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

@@ -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) {

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,22 +657,26 @@ 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)
+						}
 					}
 				}
 			}
 		}
 
 		// 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)}

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

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