From ebc05a52218d9f43d71e64bbf543e88dac003e7d Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Mon, 17 Jul 2023 09:50:04 -0400 Subject: [PATCH] backport of commit 366693c78dec71212ab344e8d315637722b60d25 (#21887) Co-authored-by: Laurent --- builtin/logical/pki/acme_wrappers.go | 17 +++++++----- builtin/logical/pki/path_acme_order.go | 16 ++++++++--- builtin/logical/pki/path_acme_test.go | 37 +++++++++++++++++++++++-- builtin/logical/pki/path_config_acme.go | 12 ++++++++ changelog/21702.txt | 3 ++ ui/app/models/pki/config/acme.js | 7 +++++ website/content/api-docs/secret/pki.mdx | 4 +++ 7 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 changelog/21702.txt diff --git a/builtin/logical/pki/acme_wrappers.go b/builtin/logical/pki/acme_wrappers.go index ef83bbce2..13ba1c3fd 100644 --- a/builtin/logical/pki/acme_wrappers.go +++ b/builtin/logical/pki/acme_wrappers.go @@ -398,13 +398,16 @@ func getAcmeRoleAndIssuer(sc *storageContext, data *framework.FieldData, config } } - // Override ExtKeyUsage behavior to force it to only be ServerAuth within ACME issued certs - role.ExtKeyUsage = []string{"serverauth"} - role.ExtKeyUsageOIDs = []string{} - role.ServerFlag = true - role.ClientFlag = false - role.CodeSigningFlag = false - role.EmailProtectionFlag = false + // If not allowed in configuration, override ExtKeyUsage behavior to force it to only be + // ServerAuth within ACME issued certs + if !config.AllowRoleExtKeyUsage { + role.ExtKeyUsage = []string{"serverauth"} + role.ExtKeyUsageOIDs = []string{} + role.ServerFlag = true + role.ClientFlag = false + role.CodeSigningFlag = false + role.EmailProtectionFlag = false + } return role, issuer, nil } diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index eb2ffaf9e..e95c8fdc9 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/builtin/logical/pki/path_acme_order.go @@ -582,10 +582,18 @@ func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil. return nil, "", fmt.Errorf("verification of parsed bundle failed: %w", err) } - // We only allow ServerAuth key usage from ACME issued certs. - for _, usage := range parsedBundle.Certificate.ExtKeyUsage { - if usage != x509.ExtKeyUsageServerAuth { - return nil, "", fmt.Errorf("%w: ACME certs only allow ServerAuth key usage", ErrBadCSR) + // We only allow ServerAuth key usage from ACME issued certs + // when configuration does not allow usage of ExtKeyusage field. + config, err := ac.sc.Backend.acmeState.getConfigWithUpdate(ac.sc) + if err != nil { + return nil, "", fmt.Errorf("failed to fetch ACME configuration: %w", err) + } + + if !config.AllowRoleExtKeyUsage { + for _, usage := range parsedBundle.Certificate.ExtKeyUsage { + if usage != x509.ExtKeyUsageServerAuth { + return nil, "", fmt.Errorf("%w: ACME certs only allow ServerAuth key usage", ErrBadCSR) + } } } diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go index 335200f90..7e53bb213 100644 --- a/builtin/logical/pki/path_acme_test.go +++ b/builtin/logical/pki/path_acme_test.go @@ -795,8 +795,10 @@ func TestAcmeTruncatesToIssuerExpiry(t *testing.T) { require.Equal(t, shortCa.NotAfter, acmeCert.NotAfter, "certificate times aren't the same") } -// TestAcmeIgnoresRoleExtKeyUsage -func TestAcmeIgnoresRoleExtKeyUsage(t *testing.T) { +// TestAcmeRoleExtKeyUsage verify that ACME by default ignores the role's various ExtKeyUsage flags, +// but if the ACME configuration override of allow_role_ext_key_usage is set that we then honor +// the role's flag. +func TestAcmeRoleExtKeyUsage(t *testing.T) { t.Parallel() cluster, client, _ := setupAcmeBackend(t) @@ -862,6 +864,37 @@ func TestAcmeIgnoresRoleExtKeyUsage(t *testing.T) { require.Equal(t, 1, len(acmeCert.ExtKeyUsage), "mis-match on expected ExtKeyUsages") require.ElementsMatch(t, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, acmeCert.ExtKeyUsage, "mismatch of ExtKeyUsage flags") + + // Now turn the ACME configuration allow_role_ext_key_usage and retest to make sure we get a certificate + // with them all + _, err = client.Logical().WriteWithContext(context.Background(), "pki/config/acme", map[string]interface{}{ + "enabled": true, + "eab_policy": "not-required", + "allow_role_ext_key_usage": true, + }) + require.NoError(t, err, "failed updating ACME configuration") + + t.Logf("Testing Authorize Order on %s", baseAcmeURL) + order, err = acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{ + {Type: "dns", Value: identifiers[0]}, + }) + require.NoError(t, err, "failed creating order") + + // HACK: Update authorization/challenge to completed as we can't really do it properly in this workflow test. + markAuthorizationSuccess(t, client, acmeClient, acct, order) + + certs, _, err = acmeClient.CreateOrderCert(testCtx, order.FinalizeURL, csr, true) + require.NoError(t, err, "order finalization failed") + require.GreaterOrEqual(t, len(certs), 1, "expected at least one cert in bundle") + acmeCert, err = x509.ParseCertificate(certs[0]) + require.NoError(t, err, "failed parsing acme cert") + + require.Equal(t, 4, len(acmeCert.ExtKeyUsage), "mis-match on expected ExtKeyUsages") + require.ElementsMatch(t, []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth, + x509.ExtKeyUsageCodeSigning, x509.ExtKeyUsageEmailProtection, + }, + acmeCert.ExtKeyUsage, "mismatch of ExtKeyUsage flags") } func TestIssuerRoleDirectoryAssociations(t *testing.T) { diff --git a/builtin/logical/pki/path_config_acme.go b/builtin/logical/pki/path_config_acme.go index d6818deae..c3fa53920 100644 --- a/builtin/logical/pki/path_config_acme.go +++ b/builtin/logical/pki/path_config_acme.go @@ -27,6 +27,7 @@ type acmeConfigEntry struct { Enabled bool `json:"enabled"` AllowedIssuers []string `json:"allowed_issuers="` AllowedRoles []string `json:"allowed_roles"` + AllowRoleExtKeyUsage bool `json:"allow_role_ext_key_usage"` DefaultDirectoryPolicy string `json:"default_directory_policy"` DNSResolver string `json:"dns_resolver"` EabPolicyName EabPolicyName `json:"eab_policy_name"` @@ -36,6 +37,7 @@ var defaultAcmeConfig = acmeConfigEntry{ Enabled: false, AllowedIssuers: []string{"*"}, AllowedRoles: []string{"*"}, + AllowRoleExtKeyUsage: false, DefaultDirectoryPolicy: "sign-verbatim", DNSResolver: "", EabPolicyName: eabPolicyNotRequired, @@ -98,6 +100,11 @@ func pathAcmeConfig(b *backend) *framework.Path { Description: `which roles are allowed for use with ACME; by default via '*', these will be all roles including sign-verbatim; when concrete role names are specified, any default_directory_policy role must be included to allow usage of the default acme directories under /pki/acme/directory and /pki/issuer/:issuer_id/acme/directory.`, Default: []string{"*"}, }, + "allow_role_ext_key_usage": { + Type: framework.TypeBool, + Description: `whether the ExtKeyUsage field from a role is used, defaults to false meaning that certificate will be signed with ServerAuth.`, + Default: false, + }, "default_directory_policy": { Type: framework.TypeString, Description: `the policy to be used for non-role-qualified ACME requests; by default ACME issuance will be otherwise unrestricted, equivalent to the sign-verbatim endpoint; one may also specify a role to use as this policy, as "role:", the specified role must be allowed by allowed_roles`, @@ -161,6 +168,7 @@ func genResponseFromAcmeConfig(config *acmeConfigEntry, warnings []string) *logi response := &logical.Response{ Data: map[string]interface{}{ "allowed_roles": config.AllowedRoles, + "allow_role_ext_key_usage": config.AllowRoleExtKeyUsage, "allowed_issuers": config.AllowedIssuers, "default_directory_policy": config.DefaultDirectoryPolicy, "enabled": config.Enabled, @@ -194,6 +202,10 @@ func (b *backend) pathAcmeWrite(ctx context.Context, req *logical.Request, d *fr } } + if allowRoleExtKeyUsageRaw, ok := d.GetOk("allow_role_ext_key_usage"); ok { + config.AllowRoleExtKeyUsage = allowRoleExtKeyUsageRaw.(bool) + } + if defaultDirectoryPolicyRaw, ok := d.GetOk("default_directory_policy"); ok { config.DefaultDirectoryPolicy = defaultDirectoryPolicyRaw.(string) } diff --git a/changelog/21702.txt b/changelog/21702.txt new file mode 100644 index 000000000..5475a486d --- /dev/null +++ b/changelog/21702.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Add a parameter to allow ExtKeyUsage field usage from a role within ACME. +``` diff --git a/ui/app/models/pki/config/acme.js b/ui/app/models/pki/config/acme.js index 86025ed60..9733932c7 100644 --- a/ui/app/models/pki/config/acme.js +++ b/ui/app/models/pki/config/acme.js @@ -39,6 +39,13 @@ export default class PkiConfigAcmeModel extends Model { }) allowedRoles; + @attr('boolean', { + label: 'Allow role ExtKeyUsage', + subText: + "When enabled, respect the role's ExtKeyUsage flags. Otherwise, ACME certificates are forced to ServerAuth.", + }) + allowRoleExtKeyUsage; + @attr('array', { editType: 'stringArray', subText: diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index c6c4c4e0b..e94a2fedb 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -374,6 +374,10 @@ mount. an issuer outside this list, it will be allowed. The default value `*` allows every issuer within the mount. + - `allow_role_ext_key_usage` `(bool: false)` - whether the ExtKeyUsage field + from a role is used, defaults to false meaning that certificate will be + signed with ServerAuth. + - `allowed_roles` `(list: ["*"])` - Specifies a list of roles allowed to issue certificates via explicit ACME paths. The default value `*` allows every role within the mount to be used. If the `default_directory_policy`