Update documentation, some comments, make code cleaner, and make generated roots be revoked when their TTL is up

builtin/logical/pki/cert_util.go

@@ -394,6 +394,9 @@ func signCert(b *backend,
 	return parsedBundle, nil
 }
 
+// generateCreationBundle is a shared function that reads parameters supplied
+// from the various endpoints and generates a creationBundle with the
+// parameters that can be used to issue or sign
 func generateCreationBundle(b *backend,
 	role *roleEntry,
 	signingBundle *caInfoBundle,
@@ -401,140 +404,159 @@ func generateCreationBundle(b *backend,
 	req *logical.Request,
 	data *framework.FieldData) (*creationBundle, error) {
 	var err error
+	var ok bool
 
-	// Get the common name(s)
+	// Get the common name
 	var cn string
-	if csr != nil {
+	{
-		if role.UseCSRCommonName {
+		if csr != nil {
-			cn = csr.Subject.CommonName
+			if role.UseCSRCommonName {
+				cn = csr.Subject.CommonName
+			}
 		}
-	}
-	if cn == "" {
-		cn = data.Get("common_name").(string)
 		if cn == "" {
-			return nil, certutil.UserError{Err: `the common_name field is required, or must be provided in a CSR with "use_csr_common_name" set to true`}
+			cn = data.Get("common_name").(string)
+			if cn == "" {
+				return nil, certutil.UserError{Err: `the common_name field is required, or must be provided in a CSR with "use_csr_common_name" set to true`}
+			}
 		}
 	}
 
+	// Read in alternate names -- DNS and email addresses
 	dnsNames := []string{}
 	emailAddresses := []string{}
-	if strings.Contains(cn, "@") {
+	{
-		emailAddresses = append(emailAddresses, cn)
+		if strings.Contains(cn, "@") {
-	} else {
+			emailAddresses = append(emailAddresses, cn)
-		dnsNames = append(dnsNames, cn)
-	}
-	cnAltInt, ok := data.GetOk("alt_names")
-	if ok {
-		cnAlt := cnAltInt.(string)
-		if len(cnAlt) != 0 {
-			for _, v := range strings.Split(cnAlt, ",") {
-				if strings.Contains(v, "@") {
-					emailAddresses = append(emailAddresses, cn)
-				} else {
-					dnsNames = append(dnsNames, v)
-				}
-			}
-		}
-	}
-
-	// Get any IP SANs
-	ipAddresses := []net.IP{}
-	ipAltInt, ok := data.GetOk("ip_sans")
-	if ok {
-		ipAlt := ipAltInt.(string)
-		if len(ipAlt) != 0 {
-			if !role.AllowIPSANs {
-				return nil, certutil.UserError{Err: fmt.Sprintf(
-					"IP Subject Alternative Names are not allowed in this role, but was provided %s", ipAlt)}
-			}
-			for _, v := range strings.Split(ipAlt, ",") {
-				parsedIP := net.ParseIP(v)
-				if parsedIP == nil {
-					return nil, certutil.UserError{Err: fmt.Sprintf(
-						"the value '%s' is not a valid IP address", v)}
-				}
-				ipAddresses = append(ipAddresses, parsedIP)
-			}
-		}
-	}
-
-	var ttlField string
-	ttlFieldInt, ok := data.GetOk("ttl")
-	if !ok {
-		ttlField = role.TTL
-	} else {
-		ttlField = ttlFieldInt.(string)
-	}
-
-	var ttl time.Duration
-	if len(ttlField) == 0 {
-		ttl = b.System().DefaultLeaseTTL()
-	} else {
-		ttl, err = time.ParseDuration(ttlField)
-		if err != nil {
-			return nil, certutil.UserError{Err: fmt.Sprintf(
-				"invalid requested ttl: %s", err)}
-		}
-	}
-
-	var maxTTL time.Duration
-	if len(role.MaxTTL) == 0 {
-		maxTTL = b.System().MaxLeaseTTL()
-	} else {
-		maxTTL, err = time.ParseDuration(role.MaxTTL)
-		if err != nil {
-			return nil, certutil.UserError{Err: fmt.Sprintf(
-				"invalid ttl: %s", err)}
-		}
-	}
-
-	if ttl > maxTTL {
-		// Don't error if they were using system defaults, only error if
-		// they specifically chose a bad TTL
-		if len(ttlField) == 0 {
-			ttl = maxTTL
 		} else {
+			dnsNames = append(dnsNames, cn)
+		}
+		cnAltInt, ok := data.GetOk("alt_names")
+		if ok {
+			cnAlt := cnAltInt.(string)
+			if len(cnAlt) != 0 {
+				for _, v := range strings.Split(cnAlt, ",") {
+					if strings.Contains(v, "@") {
+						emailAddresses = append(emailAddresses, cn)
+					} else {
+						dnsNames = append(dnsNames, v)
+					}
+				}
+			}
+		}
+
+		// Check for bad email and/or DNS names
+		badName, err := validateNames(req, dnsNames, role)
+		if len(badName) != 0 {
 			return nil, certutil.UserError{Err: fmt.Sprintf(
-				"ttl is larger than maximum allowed (%d)", maxTTL/time.Second)}
+				"name %s not allowed by this role", badName)}
+		} else if err != nil {
+			return nil, certutil.InternalError{Err: fmt.Sprintf(
+				"error validating name %s: %s", badName, err)}
+		}
+
+		badName, err = validateNames(req, emailAddresses, role)
+		if len(badName) != 0 {
+			return nil, certutil.UserError{Err: fmt.Sprintf(
+				"email %s not allowed by this role", badName)}
+		} else if err != nil {
+			return nil, certutil.InternalError{Err: fmt.Sprintf(
+				"error validating name %s: %s", badName, err)}
 		}
 	}
 
-	if signingBundle != nil &&
+	// Get and verify any IP SANs
-		time.Now().Add(ttl).After(signingBundle.Certificate.NotAfter) {
+	ipAddresses := []net.IP{}
-		return nil, certutil.UserError{Err: fmt.Sprintf(
+	var ipAltInt interface{}
-			"cannot satisfy request, as TTL is beyond the expiration of the CA certificate")}
+	{
+		ipAltInt, ok = data.GetOk("ip_sans")
+		if ok {
+			ipAlt := ipAltInt.(string)
+			if len(ipAlt) != 0 {
+				if !role.AllowIPSANs {
+					return nil, certutil.UserError{Err: fmt.Sprintf(
+						"IP Subject Alternative Names are not allowed in this role, but was provided %s", ipAlt)}
+				}
+				for _, v := range strings.Split(ipAlt, ",") {
+					parsedIP := net.ParseIP(v)
+					if parsedIP == nil {
+						return nil, certutil.UserError{Err: fmt.Sprintf(
+							"the value '%s' is not a valid IP address", v)}
+					}
+					ipAddresses = append(ipAddresses, parsedIP)
+				}
+			}
+		}
 	}
 
-	badName, err := validateNames(req, dnsNames, role)
+	// Get the TTL and very it against the max allowed
-	if len(badName) != 0 {
+	var ttlField string
-		return nil, certutil.UserError{Err: fmt.Sprintf(
+	var ttl time.Duration
-			"name %s not allowed by this role", badName)}
+	var maxTTL time.Duration
-	} else if err != nil {
+	var ttlFieldInt interface{}
-		return nil, certutil.InternalError{Err: fmt.Sprintf(
+	{
-			"error validating name %s: %s", badName, err)}
+		ttlFieldInt, ok = data.GetOk("ttl")
-	}
+		if !ok {
-
+			ttlField = role.TTL
-	badName, err = validateNames(req, emailAddresses, role)
+		} else {
-	if len(badName) != 0 {
+			ttlField = ttlFieldInt.(string)
-		return nil, certutil.UserError{Err: fmt.Sprintf(
+		}
-			"email %s not allowed by this role", badName)}
+
-	} else if err != nil {
+		if len(ttlField) == 0 {
-		return nil, certutil.InternalError{Err: fmt.Sprintf(
+			ttl = b.System().DefaultLeaseTTL()
-			"error validating name %s: %s", badName, err)}
+		} else {
+			ttl, err = time.ParseDuration(ttlField)
+			if err != nil {
+				return nil, certutil.UserError{Err: fmt.Sprintf(
+					"invalid requested ttl: %s", err)}
+			}
+		}
+
+		if len(role.MaxTTL) == 0 {
+			maxTTL = b.System().MaxLeaseTTL()
+		} else {
+			maxTTL, err = time.ParseDuration(role.MaxTTL)
+			if err != nil {
+				return nil, certutil.UserError{Err: fmt.Sprintf(
+					"invalid ttl: %s", err)}
+			}
+		}
+
+		if ttl > maxTTL {
+			// Don't error if they were using system defaults, only error if
+			// they specifically chose a bad TTL
+			if len(ttlField) == 0 {
+				ttl = maxTTL
+			} else {
+				return nil, certutil.UserError{Err: fmt.Sprintf(
+					"ttl is larger than maximum allowed (%d)", maxTTL/time.Second)}
+			}
+		}
+
+		// If it's not self-signed, verify that the issued certificate won't be
+		// valid past the lifetime of the CA certificate
+		if signingBundle != nil &&
+			time.Now().Add(ttl).After(signingBundle.Certificate.NotAfter) {
+			return nil, certutil.UserError{Err: fmt.Sprintf(
+				"cannot satisfy request, as TTL is beyond the expiration of the CA certificate")}
+		}
 	}
 
+	// Build up usages
 	var usage certUsage
-	if role.ServerFlag {
+	{
-		usage = usage | serverUsage
+		if role.ServerFlag {
-	}
+			usage = usage | serverUsage
-	if role.ClientFlag {
+		}
-		usage = usage | clientUsage
+		if role.ClientFlag {
-	}
+			usage = usage | clientUsage
-	if role.CodeSigningFlag {
+		}
-		usage = usage | codeSigningUsage
+		if role.CodeSigningFlag {
-	}
+			usage = usage | codeSigningUsage
-	if role.EmailProtectionFlag {
+		}
-		usage = usage | emailProtectionUsage
+		if role.EmailProtectionFlag {
+			usage = usage | emailProtectionUsage
+		}
 	}
 
 	creationBundle := &creationBundle{
@@ -549,11 +571,18 @@ func generateCreationBundle(b *backend,
 		Usage:          usage,
 	}
 
+	// Don't deal with URLs or max path length if it's self-signed, as these
+	// normally come from the signing bundle
 	if signingBundle == nil {
 		return creationBundle, nil
 	}
 
+	// This will have been read in from the getURLs function
 	creationBundle.URLs = signingBundle.URLs
+
+	// If the max path length in the role is not nil, it was specified at
+	// generation time with the max_path_length parameter; otherwise derive it
+	// from the signing certificate
 	if role.MaxPathLength != nil {
 		creationBundle.MaxPathLength = *role.MaxPathLength
 	} else {

builtin/logical/pki/path_intermediate.go

@@ -201,7 +201,7 @@ func (b *backend) pathSetSignedIntermediate(
 	}
 
 	// For ease of later use, also store just the certificate at a known
-	// location, plus a fresh CRL
+	// location
 	entry.Key = "ca"
 	entry.Value = inputBundle.CertificateBytes
 	err = req.Storage.Put(entry)
@@ -209,6 +209,7 @@ func (b *backend) pathSetSignedIntermediate(
 		return nil, err
 	}
 
+	// Build a fresh CRL
 	err = buildCRL(b, req)
 
 	return nil, err

builtin/logical/pki/path_issue_sign.go

@@ -75,6 +75,8 @@ basic constraints.`,
 	return ret
 }
 
+// pathIssue issues a certificate and private key from given parameters,
+// subject to role restrictions
 func (b *backend) pathIssue(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 	roleName := data.Get("role").(string)
@@ -91,6 +93,8 @@ func (b *backend) pathIssue(
 	return b.pathIssueSignCert(req, data, role, false, false)
 }
 
+// pathSign issues a certificate from a submitted CSR, subject to role
+// restrictions
 func (b *backend) pathSign(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 	roleName := data.Get("role").(string)
@@ -107,6 +111,8 @@ func (b *backend) pathSign(
 	return b.pathIssueSignCert(req, data, role, true, false)
 }
 
+// pathSignVerbatim issues a certificate from a submitted CSR, *not* subject to
+// role restrictions
 func (b *backend) pathSignVerbatim(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 

builtin/logical/pki/path_root.go

@@ -82,7 +82,6 @@ func (b *backend) pathCAGenerateRoot(
 		role.MaxPathLength = &maxPathLength
 	}
 
-	var resp *logical.Response
 	parsedBundle, err := generateCert(b, role, nil, true, req, data)
 	if err != nil {
 		switch err.(type) {
@@ -98,17 +97,19 @@ func (b *backend) pathCAGenerateRoot(
 		return nil, fmt.Errorf("error converting raw cert bundle to cert bundle: %s", err)
 	}
 
-	resp = &logical.Response{
+	resp := b.Secret(SecretCertsType).Response(
-		Data: map[string]interface{}{
+		map[string]interface{}{
-			"serial_number": cb.SerialNumber,
 			"expiration":    int64(parsedBundle.Certificate.NotAfter.Unix()),
+			"serial_number": cb.SerialNumber,
+			"certificate":   cb.Certificate,
+			"issuing_ca":    cb.IssuingCA,
 		},
-	}
+		map[string]interface{}{
+			"serial_number": cb.SerialNumber,
+		})
 
 	switch format {
 	case "pem":
-		resp.Data["certificate"] = cb.Certificate
-		resp.Data["issuing_ca"] = cb.IssuingCA
 		if exported {
 			resp.Data["private_key"] = cb.PrivateKey
 			resp.Data["private_key_type"] = cb.PrivateKeyType
@@ -122,6 +123,9 @@ func (b *backend) pathCAGenerateRoot(
 		}
 	}
 
+	resp.Secret.TTL = parsedBundle.Certificate.NotAfter.Sub(time.Now())
+
+	// Store it as the CA bundle
 	entry, err := logical.StorageEntryJSON("config/ca_bundle", cb)
 	if err != nil {
 		return nil, err
@@ -131,8 +135,18 @@ func (b *backend) pathCAGenerateRoot(
 		return nil, err
 	}
 
+	// Also store it as just the certificate identified by serial number, so it
+	// can be revoked
+	err = req.Storage.Put(&logical.StorageEntry{
+		Key:   "certs/" + cb.SerialNumber,
+		Value: parsedBundle.CertificateBytes,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("Unable to store certificate locally")
+	}
+
 	// For ease of later use, also store just the certificate at a known
-	// location, plus a fresh CRL
+	// location
 	entry.Key = "ca"
 	entry.Value = parsedBundle.CertificateBytes
 	err = req.Storage.Put(entry)
@@ -140,6 +154,7 @@ func (b *backend) pathCAGenerateRoot(
 		return nil, err
 	}
 
+	// Build a fresh CRL
 	err = buildCRL(b, req)
 	if err != nil {
 		return nil, err
@@ -213,16 +228,14 @@ func (b *backend) pathCASignIntermediate(
 		map[string]interface{}{
 			"expiration":    int64(parsedBundle.Certificate.NotAfter.Unix()),
 			"serial_number": cb.SerialNumber,
+			"certificate":   cb.Certificate,
+			"issuing_ca":    cb.IssuingCA,
 		},
 		map[string]interface{}{
 			"serial_number": cb.SerialNumber,
 		})
 
-	switch format {
+	if format == "der" {
-	case "pem":
-		resp.Data["certificate"] = cb.Certificate
-		resp.Data["issuing_ca"] = cb.IssuingCA
-	case "der":
 		resp.Data["certificate"] = base64.StdEncoding.EncodeToString(parsedBundle.CertificateBytes)
 		resp.Data["issuing_ca"] = base64.StdEncoding.EncodeToString(parsedBundle.IssuingCABytes)
 	}

builtin/logical/pki/secret_certs.go

@@ -33,7 +33,7 @@ reference`,
 		},
 
 		DefaultDuration:    168 * time.Hour,
-		DefaultGracePeriod: 10 * time.Minute,
+		DefaultGracePeriod: time.Duration(0),
 
 		Revoke: b.secretCredsRevoke,
 	}

website/source/docs/secrets/pki/index.html.md

@@ -158,6 +158,8 @@ Now, we generate our root certificate:
 ```text
 $ vault write pki/root/generate/internal common_name=myvault.com ttl=87600h
 Key             Value
+lease_id        pki/root/generate/internal/aa959dd4-467e-e5ff-642b-371add518b40
+lease_duration  315359999
 certificate     -----BEGIN CERTIFICATE-----
 MIIDvTCCAqWgAwIBAgIUAsza+fvOw+Xh9ifYQ0gNN0ruuWcwDQYJKoZIhvcNAQEL
 BQAwFjEUMBIGA1UEAxMLbXl2YXVsdC5jb20wHhcNMTUxMTE5MTYwNDU5WhcNMjUx
@@ -1124,7 +1126,6 @@ subpath for interactive help output.
   </dd>
 </dl>
 
-
 #### DELETE
 
 <dl class="api">
@@ -1161,10 +1162,10 @@ subpath for interactive help output.
     overwrite any previously-existing private key and certificate._ If the path
     ends with `exported`, the private key will be returned in the response; if
     it is `internal` the private key will not be returned and *cannot be
-    retrieved later*. Distribution points use the values set via
+    retrieved later*. Distribution points use the values set via `config/urls`.
-    `config/urls`. <br /><br />Vault does _not_ revoke this certificate (since
+    <br /><br />As with other issued certificates, Vault will automatically
-    it could not sign the CRL with an expired certificate), however, this
+    revoke the generated root at the end of its lease period; the CA
-    endpoint does honor the maximum mount TTL.
+    certificate will sign its own CRL.
   </dd>
 
   <dt>Method</dt>
@@ -1234,9 +1235,9 @@ subpath for interactive help output.
 
     ```javascript
     {
-      "lease_id": "",
+      "lease_id": "pki/root/generate/internal/aa959dd4-467e-e5ff-642b-371add518b40",
+      "lease_duration": 315359999,
       "renewable": false,
-      "lease_duration": 21600,
       "data": {
         "certificate": "-----BEGIN CERTIFICATE-----\nMIIDzDCCAragAwIBAgIUOd0ukLcjH43TfTHFG9qE0FtlMVgwCwYJKoZIhvcNAQEL\n...\numkqeYeO30g1uYvDuWLXVA==\n-----END CERTIFICATE-----\n",
         "issuing_ca": "-----BEGIN CERTIFICATE-----\nMIIDzDCCAragAwIBAgIUOd0ukLcjH43TfTHFG9qE0FtlMVgwCwYJKoZIhvcNAQEL\n...\numkqeYeO30g1uYvDuWLXVA==\n-----END CERTIFICATE-----\n",