From 58cab5f59f890bf489016749cc849a684b512133 Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 9 Feb 2018 13:42:19 -0500 Subject: [PATCH] added a flag to make common name optional if desired (#3940) * added a flag to make common name optional if desired * Cover one more case where cn can be empty * remove skipping when empty; instead check for emptiness before calling validateNames * Add verification before adding to DNS names to also fix #3918 --- builtin/logical/pki/backend_test.go | 91 +++++++++++++++++++++ builtin/logical/pki/cert_util.go | 27 +++--- builtin/logical/pki/path_roles.go | 7 ++ website/source/api/secret/pki/index.html.md | 13 +-- 4 files changed, 123 insertions(+), 15 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index b56c9a5bdef0..f6dc61b73aa8 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -41,6 +41,97 @@ var ( parsedKeyUsageUnderTest int ) +func TestPKI_RequireCN(t *testing.T) { + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "pki": Factory, + }, + } + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + var err error + err = client.Sys().Mount("pki", &api.MountInput{ + Type: "pki", + Config: api.MountConfigInput{ + DefaultLeaseTTL: "16h", + MaxLeaseTTL: "32h", + }, + }) + if err != nil { + t.Fatal(err) + } + + resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ + "common_name": "myvault.com", + }) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("expected ca info") + } + + // Create a role which does require CN (default) + _, err = client.Logical().Write("pki/roles/example", map[string]interface{}{ + "allowed_domains": "foobar.com,zipzap.com,abc.com,xyz.com", + "allow_bare_domains": true, + "allow_subdomains": true, + "max_ttl": "2h", + }) + + // Issue a cert with require_cn set to true and with common name supplied. + // It should succeed. + resp, err = client.Logical().Write("pki/issue/example", map[string]interface{}{ + "common_name": "foobar.com", + }) + if err != nil { + t.Fatal(err) + } + + // Issue a cert with require_cn set to true and with out supplying the + // common name. It should error out. + resp, err = client.Logical().Write("pki/issue/example", map[string]interface{}{}) + if err == nil { + t.Fatalf("expected an error due to missing common_name") + } + + // Modify the role to make the common name optional + _, err = client.Logical().Write("pki/roles/example", map[string]interface{}{ + "allowed_domains": "foobar.com,zipzap.com,abc.com,xyz.com", + "allow_bare_domains": true, + "allow_subdomains": true, + "max_ttl": "2h", + "require_cn": false, + }) + + // Issue a cert with require_cn set to false and without supplying the + // common name. It should succeed. + resp, err = client.Logical().Write("pki/issue/example", map[string]interface{}{}) + if err != nil { + t.Fatal(err) + } + + if resp.Data["certificate"] == "" { + t.Fatalf("expected a cert to be generated") + } + + // Issue a cert with require_cn set to false and with a common name. It + // should succeed. + resp, err = client.Logical().Write("pki/issue/example", map[string]interface{}{}) + if err != nil { + t.Fatal(err) + } + + if resp.Data["certificate"] == "" { + t.Fatalf("expected a cert to be generated") + } +} + // Performs basic tests on CA functionality // Uses the RSA CA key func TestBackend_RSAKey(t *testing.T) { diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 1d6c5236ec5a..6f04110b2352 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -623,8 +623,8 @@ func generateCreationBundle(b *backend, } if cn == "" { cn = data.Get("common_name").(string) - if cn == "" { - return nil, errutil.UserError{Err: `the common_name field is required, or must be provided in a CSR with "use_csr_common_name" set to true`} + if cn == "" && role.RequireCN { + return nil, errutil.UserError{Err: `the common_name field is required, or must be provided in a CSR with "use_csr_common_name" set to true, unless "require_cn" is set to false`} } } @@ -633,7 +633,7 @@ func generateCreationBundle(b *backend, emailAddresses = csr.EmailAddresses } - if !data.Get("exclude_cn_from_sans").(bool) { + if cn != "" && !data.Get("exclude_cn_from_sans").(bool) { if strings.Contains(cn, "@") { // Note: emails are not disallowed if the role's email protection // flag is false, because they may well be included for @@ -642,7 +642,10 @@ func generateCreationBundle(b *backend, // used for the purpose for which they are presented emailAddresses = append(emailAddresses, cn) } else { - dnsNames = append(dnsNames, cn) + // Only add to dnsNames if it's actually a DNS name + if hostnameRegex.MatchString(cn) { + dnsNames = append(dnsNames, cn) + } } } @@ -654,7 +657,9 @@ func generateCreationBundle(b *backend, if strings.Contains(v, "@") { emailAddresses = append(emailAddresses, v) } else { - dnsNames = append(dnsNames, v) + if hostnameRegex.MatchString(cnAlt) { + dnsNames = append(dnsNames, v) + } } } } @@ -662,14 +667,16 @@ func generateCreationBundle(b *backend, // Check the CN. This ensures that the CN is checked even if it's // excluded from SANs. - badName := validateNames(req, []string{cn}, role) - if len(badName) != 0 { - return nil, errutil.UserError{Err: fmt.Sprintf( - "common name %s not allowed by this role", badName)} + if cn != "" { + badName := validateNames(req, []string{cn}, role) + if len(badName) != 0 { + return nil, errutil.UserError{Err: fmt.Sprintf( + "common name %s not allowed by this role", badName)} + } } // Check for bad email and/or DNS names - badName = validateNames(req, dnsNames, role) + badName := validateNames(req, dnsNames, role) if len(badName) != 0 { return nil, errutil.UserError{Err: fmt.Sprintf( "subject alternate name %s not allowed by this role", badName)} diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index d3e02401a83c..d3a32e9983d6 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -221,6 +221,11 @@ or revoked, so this option is recommended only for certificates that are non-sensitive, or extremely short-lived. This option implies a value of "false" for "generate_lease".`, }, + "require_cn": &framework.FieldSchema{ + Type: framework.TypeBool, + Default: true, + Description: `If set to false, makes the 'common_name' field optional while generating a certificate.`, + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -411,6 +416,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data Organization: data.Get("organization").([]string), GenerateLease: new(bool), NoStore: data.Get("no_store").(bool), + RequireCN: data.Get("require_cn").(bool), } // no_store implies generate_lease := false @@ -541,6 +547,7 @@ type roleEntry struct { Organization []string `json:"organization_list" mapstructure:"organization"` GenerateLease *bool `json:"generate_lease,omitempty"` NoStore bool `json:"no_store" mapstructure:"no_store"` + RequireCN bool `json:"require_cn" mapstructure:"require_cn"` // Used internally for signing intermediates AllowExpirationPastCA bool diff --git a/website/source/api/secret/pki/index.html.md b/website/source/api/secret/pki/index.html.md index 815b9811750d..3b704c2af55d 100644 --- a/website/source/api/secret/pki/index.html.md +++ b/website/source/api/secret/pki/index.html.md @@ -773,11 +773,14 @@ request is denied. Vault. - `no_store` `(bool: false)` – If set, certificates issued/signed against this -role will not be stored in the storage backend. This can improve performance -when issuing large numbers of certificates. However, certificates issued -in this way cannot be enumerated or revoked, so this option is recommended -only for certificates that are non-sensitive, or extremely short-lived. -This option implies a value of `false` for `generate_lease`. + role will not be stored in the storage backend. This can improve performance + when issuing large numbers of certificates. However, certificates issued in + this way cannot be enumerated or revoked, so this option is recommended only + for certificates that are non-sensitive, or extremely short-lived. This + option implies a value of `false` for `generate_lease`. + +- `require_cn` `(bool: true)` - If set to false, makes the `common_name` field + optional while generating a certificate. ### Sample Payload