Change behavior of TTL in sign-intermediate (#3325)

* Fix using wrong public key in sign-self-issued

* Change behavior of TTL in sign-intermediate

This allows signing CA certs with an expiration past the signer's
NotAfter.

It also change sign-self-issued to replace the Issuer, since it's
potentially RFC legal but stacks won't validate it.

Ref: https://groups.google.com/d/msg/vault-tool/giP69-n2o20/FfhRpW1vAQAJ

builtin/logical/pki/backend_test.go

@@ -2461,6 +2461,101 @@ func TestBackend_Permitted_DNS_Domains(t *testing.T) {
 	checkIssue(true, "common_name", "host.xyz.com")
 }
 
+func TestBackend_SignIntermediate_AllowedPastCA(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("root", &api.MountInput{
+		Type: "pki",
+		Config: api.MountConfigInput{
+			DefaultLeaseTTL: "16h",
+			MaxLeaseTTL:     "32h",
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = client.Sys().Mount("int", &api.MountInput{
+		Type: "pki",
+		Config: api.MountConfigInput{
+			DefaultLeaseTTL: "4h",
+			MaxLeaseTTL:     "20h",
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Direct issuing from root
+	_, err = client.Logical().Write("root/root/generate/internal", map[string]interface{}{
+		"ttl":         "40h",
+		"common_name": "myvault.com",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = client.Logical().Write("root/roles/test", map[string]interface{}{
+		"allow_bare_domains": true,
+		"allow_subdomains":   true,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	resp, err := client.Logical().Write("int/intermediate/generate/internal", map[string]interface{}{
+		"common_name": "myint.com",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	csr := resp.Data["csr"]
+
+	_, err = client.Logical().Write("root/sign/test", map[string]interface{}{
+		"common_name": "myint.com",
+		"csr":         csr,
+		"ttl":         "60h",
+	})
+	if err == nil {
+		t.Fatal("expected error")
+	}
+
+	_, err = client.Logical().Write("root/sign-verbatim/test", map[string]interface{}{
+		"common_name": "myint.com",
+		"csr":         csr,
+		"ttl":         "60h",
+	})
+	if err == nil {
+		t.Fatal("expected error")
+	}
+
+	resp, err = client.Logical().Write("root/root/sign-intermediate", map[string]interface{}{
+		"common_name": "myint.com",
+		"csr":         csr,
+		"ttl":         "60h",
+	})
+	if err != nil {
+		t.Fatalf("got error: %v", err)
+	}
+	if resp == nil {
+		t.Fatal("got nil response")
+	}
+	if len(resp.Warnings) == 0 {
+		t.Fatal("expected warnings")
+	}
+}
+
 func TestBackend_SignSelfIssued(t *testing.T) {
 	// create the backend
 	config := logical.TestBackendConfig()
@@ -2601,8 +2696,11 @@ func TestBackend_SignSelfIssued(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !reflect.DeepEqual(newCert.Subject, newCert.Issuer) {
-		t.Fatal("expected same subject/issuer")
+	if reflect.DeepEqual(newCert.Subject, newCert.Issuer) {
+		t.Fatal("expected different subject/issuer")
+	}
+	if !reflect.DeepEqual(newCert.Issuer, signingBundle.Certificate.Subject) {
+		t.Fatalf("expected matching issuer/CA subject\n\nIssuer:\n%#v\nSubject:\n%#v\n", newCert.Issuer, signingBundle.Certificate.Subject)
 	}
 	if bytes.Equal(newCert.AuthorityKeyId, newCert.SubjectKeyId) {
 		t.Fatal("expected different authority/subject")

builtin/logical/pki/cert_util.go

@@ -767,7 +767,7 @@ func generateCreationBundle(b *backend,
 		// 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 &&
-			notAfter.After(signingBundle.Certificate.NotAfter) {
+			notAfter.After(signingBundle.Certificate.NotAfter) && !role.AllowExpirationPastCA {
 
 			return nil, errutil.UserError{Err: fmt.Sprintf(
 				"cannot satisfy request, as TTL is beyond the expiration of the CA certificate")}

builtin/logical/pki/path_roles.go

@@ -532,6 +532,9 @@ type roleEntry struct {
 	Organization          string `json:"organization" structs:"organization" mapstructure:"organization"`
 	GenerateLease         *bool  `json:"generate_lease,omitempty" structs:"generate_lease,omitempty"`
 	NoStore               bool   `json:"no_store" structs:"no_store" mapstructure:"no_store"`
+
+	// Used internally for signing intermediates
+	AllowExpirationPastCA bool
 }
 
 const pathListRolesHelpSyn = `List the existing roles in this backend`

builtin/logical/pki/path_root.go

@@ -242,12 +242,13 @@ func (b *backend) pathCASignIntermediate(
 	}
 
 	role := &roleEntry{
-		TTL:              (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
-		AllowLocalhost:   true,
-		AllowAnyName:     true,
-		AllowIPSANs:      true,
-		EnforceHostnames: false,
-		KeyType:          "any",
+		TTL:                   (time.Duration(data.Get("ttl").(int)) * time.Second).String(),
+		AllowLocalhost:        true,
+		AllowAnyName:          true,
+		AllowIPSANs:           true,
+		EnforceHostnames:      false,
+		KeyType:               "any",
+		AllowExpirationPastCA: true,
 	}
 
 	if cn := data.Get("common_name").(string); len(cn) == 0 {
@@ -304,6 +305,10 @@ func (b *backend) pathCASignIntermediate(
 		},
 	}
 
+	if signingBundle.Certificate.NotAfter.Before(parsedBundle.Certificate.NotAfter) {
+		resp.AddWarning("The expiration time for the signed certificate is after the CA's expiration time. If the new certificate is not treated as a root, validation paths with the certificate past the issuing CA's expiration time will fail.")
+	}
+
 	switch format {
 	case "pem":
 		resp.Data["certificate"] = cb.Certificate
@@ -388,7 +393,6 @@ func (b *backend) pathCASignSelfIssued(
 		return nil, fmt.Errorf("Error converting raw signing bundle to cert bundle: %s", err)
 	}
 
-	cert.AuthorityKeyId = signingBundle.Certificate.SubjectKeyId
 	urls := &urlEntries{}
 	if signingBundle.URLs != nil {
 		urls = signingBundle.URLs
@@ -397,7 +401,7 @@ func (b *backend) pathCASignSelfIssued(
 	cert.CRLDistributionPoints = urls.CRLDistributionPoints
 	cert.OCSPServer = urls.OCSPServers
 
-	newCert, err := x509.CreateCertificate(rand.Reader, cert, cert, signingBundle.PrivateKey.Public(), signingBundle.PrivateKey)
+	newCert, err := x509.CreateCertificate(rand.Reader, cert, signingBundle.Certificate, cert.PublicKey, signingBundle.PrivateKey)
 	if err != nil {
 		return nil, errwrap.Wrapf("error signing self-issued certificate: {{err}}", err)
 	}
@@ -448,5 +452,7 @@ Signs another CA's self-issued certificate.
 const pathSignSelfIssuedHelpDesc = `
 Signs another CA's self-issued certificate. This is most often used for rolling roots; unless you know you need this you probably want to use sign-intermediate instead.
 
-Note that this is a very privileged operation and should be extremely restricted in terms of who is allowed to use it. All values will be taken directly from the incoming certificate and no verification of host names, path lengths, or any other values will be performed.
+Note that this is a very privileged operation and should be extremely restricted in terms of who is allowed to use it. All values will be taken directly from the incoming certificate and only verification that it is self-issued will be performed.
+
+Configured URLs for CRLs/OCSP/etc. will be copied over and the issuer will be this mount's CA cert. Other than that, all other values will be used verbatim.
 `

website/source/api/secret/pki/index.html.md

@@ -1035,7 +1035,8 @@ verbatim.
 
 - `ttl` `(string: "")` – Specifies the requested Time To Live (after which the
   certificate will be expired). This cannot be larger than the mount max (or, if
-  not set, the system max).
+  not set, the system max). However, this can be after the expiration of the
+  signing CA.
 
 - `format` `(string: "pem")` – Specifies the format for returned data. Can be
   `pem`, `der`, or `pem_bundle`. If `der`, the output is base64 encoded. If
@@ -1111,11 +1112,13 @@ certificate (which will usually be a self-signed certificate as well).
 **_This is an extremely privileged endpoint_**. The given certificate will be
 signed as-is with only minimal validation performed (is it a CA cert, and is it
 actually self-issued). The only values that will be changed will be the
-authority key ID and, if set, any distribution points.
+authority key ID, the issuer DN, and, if set, any distribution points.
 
-This is generally only needed for root certificate rolling. If you don't know
-whether you need this endpoint, you most likely should be using a different
-endpoint (such as `sign-intermediate`).
+This is generally only needed for root certificate rolling in cases where you
+don't want/can't get access to a CSR (such as if it's a root stored in Vault
+where the key is not exposed). If you don't know whether you need this
+endpoint, you most likely should be using a different endpoint (such as
+`sign-intermediate`).
 
 This endpoint requires `sudo` capability.