From cad909ed024245fe444c51ffe131d15b9d9285cc Mon Sep 17 00:00:00 2001 From: kitography Date: Mon, 16 May 2022 12:52:40 -0400 Subject: [PATCH 1/9] Add a warning when Issuing Certificate set on a role does not resolve. --- builtin/logical/pki/path_roles.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 60bccf0a6c21..8a07324b4049 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -726,6 +726,18 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data if len(entry.Issuer) == 0 { entry.Issuer = defaultRef } + // Check that the issuers reference set resolves to something + issuerId, err := resolveIssuerReference(ctx, req.Storage, entry.Issuer) + if issuerId == IssuerRefNotFound { + if resp == nil { + 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 { + resp.AddWarning(fmt.Sprintf("Issuing Certificate was set to %s but no issuing certificate currently has that name", entry.Issuer)) + } + } // Store it jsonEntry, err := logical.StorageEntryJSON("role/"+name, entry) From 9e6d5256f278ba482985f74c346401c012798628 Mon Sep 17 00:00:00 2001 From: kitography Date: Tue, 17 May 2022 12:00:09 -0400 Subject: [PATCH 2/9] Ivanka's requests - add a warning on deleting issuer or changing it's name. --- builtin/logical/pki/path_fetch_issuers.go | 37 ++++++++++++++++++++--- builtin/logical/pki/storage.go | 36 ++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 2e4756761fb2..efcc0969a4dd 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -248,7 +248,9 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da modified := false + var oldName string if newName != issuer.Name { + oldName = issuer.Name issuer.Name = newName modified = true } @@ -309,7 +311,12 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da } } - return respondReadIssuer(issuer) + 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) { @@ -399,15 +406,22 @@ func (b *backend) pathDeleteIssuer(ctx context.Context, req *logical.Request, da return logical.ErrorResponse("unable to resolve issuer id for reference: " + issuerName), nil } + var response *logical.Response + response = &logical.Response{} + + issuer, err := fetchIssuerById(ctx, req.Storage, ref) + if issuer.Name != "" { + addWarningOnDereferencing(issuer.Name, response, ctx, req.Storage) + } + addWarningOnDereferencing(string(issuer.ID), response, ctx, req.Storage) + wasDefault, err := deleteIssuer(ctx, req.Storage, ref) if err != nil { return nil, err } - - var response *logical.Response if wasDefault { - response = &logical.Response{} response.AddWarning(fmt.Sprintf("Deleted issuer %v (via issuer_ref %v); this was configured as the default issuer. Operations without an explicit issuer will not work until a new default is configured.", ref, issuerName)) + addWarningOnDereferencing(defaultRef, response, ctx, req.Storage) } // Since we've deleted an issuer, the chains might've changed. Call the @@ -422,6 +436,21 @@ func (b *backend) pathDeleteIssuer(ctx context.Context, req *logical.Request, da return response, nil } +func addWarningOnDereferencing(name string, resp *logical.Response, ctx context.Context, s logical.Storage) { + timeout, inUseBy, err := checkForRolesReferencing(name, ctx, s) + if err != nil || timeout { + if inUseBy == 0 { + resp.AddWarning(fmt.Sprint("Unable to check if any roles referenced this issuer by ", name)) + } else { + resp.AddWarning(fmt.Sprint("The name ", name, " was in use by at least ", inUseBy, " roles")) + } + } else { + if inUseBy > 0 { + resp.AddWarning(fmt.Sprint(inUseBy, " roles reference ", name)) + } + } +} + const ( pathGetIssuerHelpSyn = `Fetch a single issuer certificate.` pathGetIssuerHelpDesc = ` diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index a2f628c39b57..7d10f12d76a1 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -27,6 +27,9 @@ const ( // Used as a quick sanity check for a reference id lookups... uuidLength = 36 + + maxRolesToScanOnIssuerChange = 10000 + maxRolesToFindOnIssuerChange = 10 ) type keyID string @@ -889,3 +892,36 @@ func isKeyInUse(keyId string, ctx context.Context, s logical.Storage) (inUse boo return false, "", nil } + +func checkForRolesReferencing(issuerId string, ctx context.Context, storage logical.Storage) (timeout bool, inUseBy int32, err error) { + roleEntries, err := storage.List(ctx, "role/") + if err != nil { + return false, 0, err + } + + inUseBy = 0 + checkedRoles := 0 + + for _, roleName := range roleEntries { + entry, err := storage.Get(ctx, "role/"+roleName) + if err != nil { + return false, 0, err + } + var role roleEntry + if err := entry.DecodeJSON(&role); err != nil { + return false, inUseBy, err + } + if role.Issuer == issuerId { + inUseBy = inUseBy + 1 + if inUseBy >= maxRolesToFindOnIssuerChange { + return true, inUseBy, nil + } + } + checkedRoles = checkedRoles + 1 + if checkedRoles >= maxRolesToScanOnIssuerChange { + return true, inUseBy, nil + } + } + + return false, inUseBy, nil +} From 55ee31bcf7b62431613289250d111cf49d672577 Mon Sep 17 00:00:00 2001 From: kitography Date: Wed, 18 May 2022 08:16:56 -0400 Subject: [PATCH 3/9] Fix nil checks; reduce number of roles to iterate through; only verify roles after migration. --- builtin/logical/pki/path_fetch_issuers.go | 3 +++ builtin/logical/pki/path_roles.go | 21 +++++++++++++-------- builtin/logical/pki/storage.go | 5 +++-- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index efcc0969a4dd..9215fb57f4a9 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -410,6 +410,9 @@ func (b *backend) pathDeleteIssuer(ctx context.Context, req *logical.Request, da response = &logical.Response{} issuer, err := fetchIssuerById(ctx, req.Storage, ref) + if err != nil { + return nil, err + } if issuer.Name != "" { addWarningOnDereferencing(issuer.Name, response, ctx, req.Storage) } diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 8a07324b4049..70863b100fcc 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -727,15 +727,20 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data entry.Issuer = defaultRef } // Check that the issuers reference set resolves to something - issuerId, err := resolveIssuerReference(ctx, req.Storage, entry.Issuer) - if issuerId == IssuerRefNotFound { - if resp == nil { - resp = &logical.Response{} + if !b.useLegacyBundleCaStorage() { + issuerId, err := resolveIssuerReference(ctx, req.Storage, entry.Issuer) + if err != nil { + return nil, err } - 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 { - resp.AddWarning(fmt.Sprintf("Issuing Certificate was set to %s but no issuing certificate currently has that name", entry.Issuer)) + if issuerId == IssuerRefNotFound { + if resp == nil { + 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 { + resp.AddWarning(fmt.Sprintf("Issuing Certificate was set to %s but no issuing certificate currently has that name", entry.Issuer)) + } } } diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index 7d10f12d76a1..81b3c60a6a3a 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -28,7 +28,7 @@ const ( // Used as a quick sanity check for a reference id lookups... uuidLength = 36 - maxRolesToScanOnIssuerChange = 10000 + maxRolesToScanOnIssuerChange = 100 maxRolesToFindOnIssuerChange = 10 ) @@ -908,7 +908,8 @@ func checkForRolesReferencing(issuerId string, ctx context.Context, storage logi return false, 0, err } var role roleEntry - if err := entry.DecodeJSON(&role); err != nil { + err = entry.DecodeJSON(&role) + if err != nil { return false, inUseBy, err } if role.Issuer == issuerId { From a947d266dc81461d5ae37f1f2f991ce96a1e6543 Mon Sep 17 00:00:00 2001 From: kitography Date: Wed, 18 May 2022 11:41:36 -0400 Subject: [PATCH 4/9] Fix semgrep failure, ignore roles deleted behind our back. --- builtin/logical/pki/path_roles.go | 20 +++++++++++--------- builtin/logical/pki/storage.go | 20 +++++++++++--------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 70863b100fcc..ad351a194b76 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -730,18 +730,20 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data if !b.useLegacyBundleCaStorage() { issuerId, err := resolveIssuerReference(ctx, req.Storage, entry.Issuer) if err != nil { - return nil, err - } - if issuerId == IssuerRefNotFound { - if resp == nil { - 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") + if issuerId == IssuerRefNotFound { + if resp == nil { + 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 { + resp.AddWarning(fmt.Sprintf("Issuing Certificate was set to %s but no issuing certificate currently has that name", entry.Issuer)) + } } else { - resp.AddWarning(fmt.Sprintf("Issuing Certificate was set to %s but no issuing certificate currently has that name", entry.Issuer)) + return nil, err } } + } // Store it diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index 81b3c60a6a3a..836b2a0bbc6a 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -907,15 +907,17 @@ func checkForRolesReferencing(issuerId string, ctx context.Context, storage logi if err != nil { return false, 0, err } - var role roleEntry - err = entry.DecodeJSON(&role) - if err != nil { - return false, inUseBy, err - } - if role.Issuer == issuerId { - inUseBy = inUseBy + 1 - if inUseBy >= maxRolesToFindOnIssuerChange { - return true, inUseBy, nil + if entry != nil { // If nil, someone deleted an entry since we haven't taken a lock here so just continue + var role roleEntry + err = entry.DecodeJSON(&role) + if err != nil { + return false, inUseBy, err + } + if role.Issuer == issuerId { + inUseBy = inUseBy + 1 + if inUseBy >= maxRolesToFindOnIssuerChange { + return true, inUseBy, nil + } } } checkedRoles = checkedRoles + 1 From 7a4045e8abb27f7868f04461522257ec4ee3fa09 Mon Sep 17 00:00:00 2001 From: kitography Date: Thu, 19 May 2022 10:35:54 -0400 Subject: [PATCH 5/9] Patch functionality for roles --- builtin/logical/pki/path_roles.go | 180 +++++++++++++++++++++++++++--- 1 file changed, 164 insertions(+), 16 deletions(-) diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index ad351a194b76..35c050e38624 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -611,7 +611,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 +670,53 @@ 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 != "" { + 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) { + 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 +745,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 +754,9 @@ 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{} - } 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 +769,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.PolicyIdentifiers).(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") + allowedOtherSANs := allowedOtherSANsData.([]string) + if wasSet { + 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 { From 1c39aa3e474ce0fb2aa8c1caf2595765148462cb Mon Sep 17 00:00:00 2001 From: kitography Date: Thu, 19 May 2022 10:55:21 -0400 Subject: [PATCH 6/9] Make Patch Roles work again, add back patch issuers. --- builtin/logical/pki/path_fetch_issuers.go | 156 ++++++++++++++++++++++ builtin/logical/pki/path_roles.go | 7 + 2 files changed, 163 insertions(+) diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 9215fb57f4a9..cbaadb8994f9 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 35c050e38624..32f4167ceac0 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, From 95ebf06b51ab5cd9a562352dc1ae856ae23b8dc8 Mon Sep 17 00:00:00 2001 From: kitography Date: Thu, 19 May 2022 10:58:47 -0400 Subject: [PATCH 7/9] Add changelog. --- changelog/15510.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/15510.txt diff --git a/changelog/15510.txt b/changelog/15510.txt new file mode 100644 index 000000000000..b6b6ab94d587 --- /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 From bc76eafb18a2698e653598361a6e2af616780cf2 Mon Sep 17 00:00:00 2001 From: kitography Date: Thu, 19 May 2022 11:23:55 -0400 Subject: [PATCH 8/9] Fix nil-reversion on empty response. --- builtin/logical/pki/path_roles.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 32f4167ceac0..4689a85b637b 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -702,6 +702,9 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data return nil, err } if warning != "" { + if resp == nil { + resp = &logical.Response{} + } resp.AddWarning(warning) } if resp.IsError() { @@ -721,7 +724,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } func validateRole(b *backend, entry *roleEntry, ctx context.Context, s logical.Storage) (*logical.Response, error) { - resp := &logical.Response{} + var resp *logical.Response var err error if entry.MaxTTL > 0 && entry.TTL > entry.MaxTTL { @@ -764,6 +767,7 @@ func validateRole(b *backend, entry *roleEntry, ctx context.Context, s logical.S issuerId, err := resolveIssuerReference(ctx, s, entry.Issuer) if err != nil { if issuerId == IssuerRefNotFound { + 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 { From c1993ec5408a14aae00d943c17ac285ded1c8f45 Mon Sep 17 00:00:00 2001 From: kitography Date: Thu, 19 May 2022 14:05:58 -0400 Subject: [PATCH 9/9] Panics are bad. don't do that. --- builtin/logical/pki/path_roles.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 4689a85b637b..1b86455b1121 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -849,15 +849,15 @@ func (b *backend) pathRolePatch(ctx context.Context, req *logical.Request, data 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.PolicyIdentifiers).(bool), + 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") - allowedOtherSANs := allowedOtherSANsData.([]string) if wasSet { + allowedOtherSANs := allowedOtherSANsData.([]string) switch { case len(allowedOtherSANs) == 0: case len(allowedOtherSANs) == 1 && allowedOtherSANs[0] == "*": @@ -874,7 +874,7 @@ func (b *backend) pathRolePatch(ctx context.Context, req *logical.Request, data allowWildcardCertificates, present := data.GetOk("allow_wildcard_certificates") if !present { - allowWildcardCertificates = oldEntry.AllowWildcardCertificates + allowWildcardCertificates = *oldEntry.AllowWildcardCertificates } *entry.AllowWildcardCertificates = allowWildcardCertificates.(bool)