From 024716421ef6e4b4cdab2ea5198723e9725b0e95 Mon Sep 17 00:00:00 2001 From: kitography Date: Fri, 20 May 2022 13:34:55 -0400 Subject: [PATCH] Vault 5917 allow patch operations to pki roles issuers (#15510) * Add a warning when Issuing Certificate set on a role does not resolve. * Ivanka's requests - add a warning on deleting issuer or changing it's name. * Fix nil checks; reduce number of roles to iterate through; only verify roles after migration. * Fix semgrep failure, ignore roles deleted behind our back. * Patch functionality for roles * Make Patch Roles work again, add back patch issuers. * Add changelog. * Fix nil-reversion on empty response. * Panics are bad. don't do that. --- builtin/logical/pki/path_fetch_issuers.go | 156 ++++++++++++++++++ builtin/logical/pki/path_roles.go | 191 ++++++++++++++++++++-- changelog/15510.txt | 3 + 3 files changed, 334 insertions(+), 16 deletions(-) create mode 100644 changelog/15510.txt diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 0e6a7b9bd..92a5c97b7 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -127,6 +127,12 @@ and always set.`, ForwardPerformanceStandby: true, ForwardPerformanceSecondary: true, }, + logical.PatchOperation: &framework.PathOperation{ + Callback: b.pathPatchIssuer, + // Read more about why these flags are set in backend.go. + ForwardPerformanceStandby: true, + ForwardPerformanceSecondary: true, + }, }, HelpSynopsis: pathGetIssuerHelpSyn, @@ -319,6 +325,156 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da return response, err } +func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + // Since we're planning on updating issuers here, grab the lock so we've + // got a consistent view. + b.issuersLock.Lock() + defer b.issuersLock.Unlock() + + if b.useLegacyBundleCaStorage() { + return logical.ErrorResponse("Can not patch issuer until migration has completed"), nil + } + + // First we fetch the issuer + issuerName := getIssuerRef(data) + if len(issuerName) == 0 { + return logical.ErrorResponse("missing issuer reference"), nil + } + + ref, err := resolveIssuerReference(ctx, req.Storage, issuerName) + if err != nil { + return nil, err + } + if ref == "" { + return logical.ErrorResponse("unable to resolve issuer id for reference: " + issuerName), nil + } + + issuer, err := fetchIssuerById(ctx, req.Storage, ref) + if err != nil { + return nil, err + } + + // Now We are Looking at What (Might) Have Changed + modified := false + + // Name Changes First + _, ok := data.GetOk("issuer_name") // Don't check for conflicts if we aren't updating the name + var oldName string + var newName string + if ok { + newName, err = getIssuerName(ctx, req.Storage, data) + if err != nil && err != errIssuerNameInUse { + // If the error is name already in use, and the new name is the + // old name for this issuer, we're not actually updating the + // issuer name (or causing a conflict) -- so don't err out. Other + // errs should still be surfaced, however. + return logical.ErrorResponse(err.Error()), nil + } + if err == errIssuerNameInUse && issuer.Name != newName { + // When the new name is in use but isn't this name, throw an error. + return logical.ErrorResponse(err.Error()), nil + } + if newName != issuer.Name { + oldName = issuer.Name + issuer.Name = newName + modified = true + } + } + + // Leaf Not After Changes + rawLeafBehaviorData, ok := data.GetOk("leaf_not_after_behaivor") + if ok { + rawLeafBehavior := rawLeafBehaviorData.(string) + var newLeafBehavior certutil.NotAfterBehavior + switch rawLeafBehavior { + case "err": + newLeafBehavior = certutil.ErrNotAfterBehavior + case "truncate": + newLeafBehavior = certutil.TruncateNotAfterBehavior + case "permit": + newLeafBehavior = certutil.PermitNotAfterBehavior + default: + return logical.ErrorResponse("Unknown value for field `leaf_not_after_behavior`. Possible values are `err`, `truncate`, and `permit`."), nil + } + if newLeafBehavior != issuer.LeafNotAfterBehavior { + issuer.LeafNotAfterBehavior = newLeafBehavior + modified = true + } + } + + // Usage Changes + rawUsageData, ok := data.GetOk("usage") + if ok { + rawUsage := rawUsageData.([]string) + newUsage, err := NewIssuerUsageFromNames(rawUsage) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("Unable to parse specified usages: %v - valid values are %v", rawUsage, AllIssuerUsages.Names())), nil + } + if newUsage != issuer.Usage { + issuer.Usage = newUsage + modified = true + } + } + + // Manual Chain Changes + newPathData, ok := data.GetOk("manual_chain") + if ok { + newPath := newPathData.([]string) + var updateChain bool + var constructedChain []issuerID + for index, newPathRef := range newPath { + // Allow self for the first entry. + if index == 0 && newPathRef == "self" { + newPathRef = string(ref) + } + + resolvedId, err := resolveIssuerReference(ctx, req.Storage, newPathRef) + if err != nil { + return nil, err + } + + if index == 0 && resolvedId != ref { + return logical.ErrorResponse(fmt.Sprintf("expected first cert in chain to be a self-reference, but was: %v/%v", newPathRef, resolvedId)), nil + } + + constructedChain = append(constructedChain, resolvedId) + if len(issuer.ManualChain) < len(constructedChain) || constructedChain[index] != issuer.ManualChain[index] { + updateChain = true + } + } + + if len(issuer.ManualChain) != len(constructedChain) { + updateChain = true + } + + if updateChain { + issuer.ManualChain = constructedChain + + // Building the chain will write the issuer to disk; no need to do it + // twice. + modified = false + err := rebuildIssuersChains(ctx, req.Storage, issuer) + if err != nil { + return nil, err + } + } + } + + if modified { + err := writeIssuer(ctx, req.Storage, issuer) + if err != nil { + return nil, err + } + } + + response, err := respondReadIssuer(issuer) + if newName != oldName { + addWarningOnDereferencing(oldName, response, ctx, req.Storage) + } + + return response, err +} + func (b *backend) pathGetRawIssuer(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { if b.useLegacyBundleCaStorage() { return logical.ErrorResponse("Can not get issuer until migration has completed"), nil diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 4100f4347..c057c9aa8 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -118,6 +118,7 @@ See the documentation for more information.`, the common name to be issued, conforming to RFC 6125's Section 6.4.3; e.g., "*.example.net" or "b*z.example.net". See the documentation for more information.`, + Default: true, }, "allow_any_name": { @@ -431,6 +432,12 @@ serviced by this role.`, ForwardPerformanceStandby: true, ForwardPerformanceSecondary: true, }, + logical.PatchOperation: &framework.PathOperation{ + Callback: b.pathRolePatch, + // Read more about why these flags are set in backend.go. + ForwardPerformanceStandby: true, + ForwardPerformanceSecondary: true, + }, }, HelpSynopsis: pathRoleHelpSyn, @@ -611,7 +618,6 @@ func (b *backend) pathRoleList(ctx context.Context, req *logical.Request, _ *fra func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { var err error - var resp *logical.Response name := data.Get("name").(string) entry := &roleEntry{ @@ -671,17 +677,56 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } entry.AllowedOtherSANs = allowedOtherSANs + allowWildcardCertificates, present := data.GetOk("allow_wildcard_certificates") + if !present { + // While not the most secure default, when AllowWildcardCertificates isn't + // explicitly specified in the request, we automatically set it to true to + // preserve compatibility with previous versions of Vault. + allowWildcardCertificates = true + } + *entry.AllowWildcardCertificates = allowWildcardCertificates.(bool) + + warning := "" // no_store implies generate_lease := false if entry.NoStore { *entry.GenerateLease = false if data.Get("generate_lease").(bool) { - resp = &logical.Response{} - resp.AddWarning("mutually exclusive values no_store=true and generate_lease=true were both specified; no_store=true takes priority") + warning = "mutually exclusive values no_store=true and generate_lease=true were both specified; no_store=true takes priority" } } else { *entry.GenerateLease = data.Get("generate_lease").(bool) } + resp, err := validateRole(b, entry, ctx, req.Storage) + if err != nil { + return nil, err + } + if warning != "" { + if resp == nil { + resp = &logical.Response{} + } + resp.AddWarning(warning) + } + if resp.IsError() { + return resp, nil + } + + // Store it + jsonEntry, err := logical.StorageEntryJSON("role/"+name, entry) + if err != nil { + return nil, err + } + if err := req.Storage.Put(ctx, jsonEntry); err != nil { + return nil, err + } + + return resp, nil +} + +func validateRole(b *backend, entry *roleEntry, ctx context.Context, s logical.Storage) (*logical.Response, error) { + var resp *logical.Response + var err error + if entry.MaxTTL > 0 && entry.TTL > entry.MaxTTL { return logical.ErrorResponse( `"ttl" value must be less than "max_ttl" value`, @@ -710,15 +755,6 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } } - allowWildcardCertificates, present := data.GetOk("allow_wildcard_certificates") - if !present { - // While not the most secure default, when AllowWildcardCertificates isn't - // explicitly specified in the request, we automatically set it to true to - // preserve compatibility with previous versions of Vault. - allowWildcardCertificates = true - } - *entry.AllowWildcardCertificates = allowWildcardCertificates.(bool) - // Ensure issuers ref is set to a non-empty value. Note that we never // resolve the reference (to an issuerId) at role creation time; instead, // resolve it at use time. This allows values such as `default` or other @@ -728,12 +764,10 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } // Check that the issuers reference set resolves to something if !b.useLegacyBundleCaStorage() { - issuerId, err := resolveIssuerReference(ctx, req.Storage, entry.Issuer) + issuerId, err := resolveIssuerReference(ctx, s, entry.Issuer) if err != nil { if issuerId == IssuerRefNotFound { - if resp == nil { - resp = &logical.Response{} - } + resp = &logical.Response{} if entry.Issuer == defaultRef { resp.AddWarning("Issuing Certificate was set to default, but no default issuing certificate (configurable at /config/issuers) is currently set") } else { @@ -746,6 +780,131 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } + return resp, nil +} + +func getWithExplicitDefault(data *framework.FieldData, field string, defaultValue interface{}) interface{} { + assignedValue, ok := data.GetOk(field) + if ok { + return assignedValue + } + return defaultValue +} + +func getTimeWithExplicitDefault(data *framework.FieldData, field string, defaultValue time.Duration) time.Duration { + assignedValue, ok := data.GetOk(field) + if ok { + return time.Duration(assignedValue.(int)) * time.Second + } + return defaultValue +} + +func (b *backend) pathRolePatch(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + name := data.Get("name").(string) + + oldEntry, err := b.getRole(ctx, req.Storage, name) + if err != nil { + return nil, err + } + if oldEntry == nil { + return logical.ErrorResponse("Unable to fetch role entry to patch"), nil + } + + entry := &roleEntry{ + MaxTTL: getTimeWithExplicitDefault(data, "max_ttl", oldEntry.MaxTTL), + TTL: getTimeWithExplicitDefault(data, "ttl", oldEntry.TTL), + AllowLocalhost: getWithExplicitDefault(data, "allow_localhost", oldEntry.AllowLocalhost).(bool), + AllowedDomains: getWithExplicitDefault(data, "allowed_domains", oldEntry.AllowedDomains).([]string), + AllowedDomainsTemplate: getWithExplicitDefault(data, "allowed_domains_template", oldEntry.AllowedDomainsTemplate).(bool), + AllowBareDomains: getWithExplicitDefault(data, "allow_bare_domains", oldEntry.AllowBareDomains).(bool), + AllowSubdomains: getWithExplicitDefault(data, "allow_subdomains", oldEntry.AllowSubdomains).(bool), + AllowGlobDomains: getWithExplicitDefault(data, "allow_glob_domains", oldEntry.AllowGlobDomains).(bool), + AllowWildcardCertificates: new(bool), // Handled specially below + AllowAnyName: getWithExplicitDefault(data, "allow_any_name", oldEntry.AllowAnyName).(bool), + AllowedURISANsTemplate: getWithExplicitDefault(data, "allowed_uri_sans_template", oldEntry.AllowedURISANsTemplate).(bool), + EnforceHostnames: getWithExplicitDefault(data, "enforce_hostnames", oldEntry.EnforceHostnames).(bool), + AllowIPSANs: getWithExplicitDefault(data, "allow_ip_sans", oldEntry.AllowIPSANs).(bool), + AllowedURISANs: getWithExplicitDefault(data, "allowed_uri_sans", oldEntry.AllowedURISANs).([]string), + ServerFlag: getWithExplicitDefault(data, "server_flag", oldEntry.ServerFlag).(bool), + ClientFlag: getWithExplicitDefault(data, "client_flag", oldEntry.ClientFlag).(bool), + CodeSigningFlag: getWithExplicitDefault(data, "code_signing_flag", oldEntry.CodeSigningFlag).(bool), + EmailProtectionFlag: getWithExplicitDefault(data, "email_protection_flag", oldEntry.EmailProtectionFlag).(bool), + KeyType: getWithExplicitDefault(data, "key_type", oldEntry.KeyType).(string), + KeyBits: getWithExplicitDefault(data, "key_bits", oldEntry.KeyBits).(int), + SignatureBits: getWithExplicitDefault(data, "signature_bits", oldEntry.SignatureBits).(int), + UseCSRCommonName: getWithExplicitDefault(data, "use_csr_common_name", oldEntry.UseCSRCommonName).(bool), + UseCSRSANs: getWithExplicitDefault(data, "use_csr_sans", oldEntry.UseCSRSANs).(bool), + KeyUsage: getWithExplicitDefault(data, "key_usage", oldEntry.KeyUsage).([]string), + ExtKeyUsage: getWithExplicitDefault(data, "ext_key_usage", oldEntry.ExtKeyUsage).([]string), + ExtKeyUsageOIDs: getWithExplicitDefault(data, "ext_key_usage_oids", oldEntry.ExtKeyUsageOIDs).([]string), + OU: getWithExplicitDefault(data, "ou", oldEntry.OU).([]string), + Organization: getWithExplicitDefault(data, "organization", oldEntry.Organization).([]string), + Country: getWithExplicitDefault(data, "country", oldEntry.Country).([]string), + Locality: getWithExplicitDefault(data, "locality", oldEntry.Locality).([]string), + Province: getWithExplicitDefault(data, "province", oldEntry.Province).([]string), + StreetAddress: getWithExplicitDefault(data, "street_address", oldEntry.StreetAddress).([]string), + PostalCode: getWithExplicitDefault(data, "postal_code", oldEntry.PostalCode).([]string), + GenerateLease: new(bool), + NoStore: getWithExplicitDefault(data, "no_store", oldEntry.NoStore).(bool), + RequireCN: getWithExplicitDefault(data, "require_cn", oldEntry.RequireCN).(bool), + AllowedSerialNumbers: getWithExplicitDefault(data, "allowed_serial_numbers", oldEntry.AllowedSerialNumbers).([]string), + PolicyIdentifiers: getWithExplicitDefault(data, "policy_identifiers", oldEntry.PolicyIdentifiers).([]string), + BasicConstraintsValidForNonCA: getWithExplicitDefault(data, "basic_constraints_valid_for_non_ca", oldEntry.BasicConstraintsValidForNonCA).(bool), + NotBeforeDuration: getTimeWithExplicitDefault(data, "not_before_duration", oldEntry.NotBeforeDuration), + NotAfter: getWithExplicitDefault(data, "not_after", oldEntry.NotAfter).(string), + Issuer: getWithExplicitDefault(data, "issuer_ref", oldEntry.Issuer).(string), + } + + allowedOtherSANsData, wasSet := data.GetOk("allowed_other_sans") + if wasSet { + allowedOtherSANs := allowedOtherSANsData.([]string) + switch { + case len(allowedOtherSANs) == 0: + case len(allowedOtherSANs) == 1 && allowedOtherSANs[0] == "*": + default: + _, err := parseOtherSANs(allowedOtherSANs) + if err != nil { + return logical.ErrorResponse(fmt.Errorf("error parsing allowed_other_sans: %w", err).Error()), nil + } + } + entry.AllowedOtherSANs = allowedOtherSANs + } else { + entry.AllowedOtherSANs = oldEntry.AllowedOtherSANs + } + + allowWildcardCertificates, present := data.GetOk("allow_wildcard_certificates") + if !present { + allowWildcardCertificates = *oldEntry.AllowWildcardCertificates + } + *entry.AllowWildcardCertificates = allowWildcardCertificates.(bool) + + warning := "" + generateLease, ok := data.GetOk("generate_lease") + // no_store implies generate_lease := false + if entry.NoStore { + *entry.GenerateLease = false + if ok && generateLease.(bool) || !ok && (*oldEntry.GenerateLease == true) { + warning = "mutually exclusive values no_store=true and generate_lease=true were both specified; no_store=true takes priority" + } + } else { + if ok { + *entry.GenerateLease = data.Get("generate_lease").(bool) + } else { + entry.GenerateLease = oldEntry.GenerateLease + } + } + + resp, err := validateRole(b, entry, ctx, req.Storage) + if err != nil { + return nil, err + } + if warning != "" { + resp.AddWarning(warning) + } + if resp.IsError() { + return resp, nil + } + // Store it jsonEntry, err := logical.StorageEntryJSON("role/"+name, entry) if err != nil { diff --git a/changelog/15510.txt b/changelog/15510.txt new file mode 100644 index 000000000..b6b6ab94d --- /dev/null +++ b/changelog/15510.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Enable Patch Functionality for Roles and Issuers (API only) +``` \ No newline at end of file