PKI: Role switch to control lease generation (#2403)

* pki: Make generation of leases optional

* pki: add tests for upgrading generate_lease

* pki: add tests for leased and non-leased certs

* docs++ pki generate_lease

* Generate lease is applicable for both issuing and signing

* pki: fix tests

* Address review feedback

* Address review feedback

builtin/logical/pki/backend_test.go

@@ -1444,6 +1444,7 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep {
 		stepCount++
 		//t.Logf("test step %d\nissue vals: %#v\n", stepCount, issueTestStep)
 		roleTestStep.Data = structs.New(roleVals).Map()
+		roleTestStep.Data["generate_lease"] = false
 		ret = append(ret, roleTestStep)
 		issueTestStep.Data = structs.New(issueVals).Map()
 		switch {

builtin/logical/pki/path_issue_sign.go

@@ -175,47 +175,43 @@ func (b *backend) pathIssueSignCert(
 		return nil, fmt.Errorf("Error converting raw cert bundle to cert bundle: %s", err)
 	}
 
-	resp := b.Secret(SecretCertsType).Response(
-		map[string]interface{}{
-			"serial_number": cb.SerialNumber,
-		},
-		map[string]interface{}{
-			"serial_number": cb.SerialNumber,
-		})
+	respData := map[string]interface{}{
+		"serial_number": cb.SerialNumber,
+	}
 
 	switch format {
 	case "pem":
-		resp.Data["issuing_ca"] = signingCB.Certificate
-		resp.Data["certificate"] = cb.Certificate
+		respData["issuing_ca"] = signingCB.Certificate
+		respData["certificate"] = cb.Certificate
 		if cb.CAChain != nil && len(cb.CAChain) > 0 {
-			resp.Data["ca_chain"] = cb.CAChain
+			respData["ca_chain"] = cb.CAChain
 		}
 		if !useCSR {
-			resp.Data["private_key"] = cb.PrivateKey
-			resp.Data["private_key_type"] = cb.PrivateKeyType
+			respData["private_key"] = cb.PrivateKey
+			respData["private_key_type"] = cb.PrivateKeyType
 		}
 
 	case "pem_bundle":
-		resp.Data["issuing_ca"] = signingCB.Certificate
-		resp.Data["certificate"] = cb.ToPEMBundle()
+		respData["issuing_ca"] = signingCB.Certificate
+		respData["certificate"] = cb.ToPEMBundle()
 		if cb.CAChain != nil && len(cb.CAChain) > 0 {
-			resp.Data["ca_chain"] = cb.CAChain
+			respData["ca_chain"] = cb.CAChain
 		}
 		if !useCSR {
-			resp.Data["private_key"] = cb.PrivateKey
-			resp.Data["private_key_type"] = cb.PrivateKeyType
+			respData["private_key"] = cb.PrivateKey
+			respData["private_key_type"] = cb.PrivateKeyType
 		}
 
 	case "der":
-		resp.Data["certificate"] = base64.StdEncoding.EncodeToString(parsedBundle.CertificateBytes)
-		resp.Data["issuing_ca"] = base64.StdEncoding.EncodeToString(signingBundle.CertificateBytes)
+		respData["certificate"] = base64.StdEncoding.EncodeToString(parsedBundle.CertificateBytes)
+		respData["issuing_ca"] = base64.StdEncoding.EncodeToString(signingBundle.CertificateBytes)
 
 		var caChain []string
 		for _, caCert := range parsedBundle.CAChain {
 			caChain = append(caChain, base64.StdEncoding.EncodeToString(caCert.Bytes))
 		}
 		if caChain != nil && len(caChain) > 0 {
-			resp.Data["ca_chain"] = caChain
+			respData["ca_chain"] = caChain
 		}
 
 		if !useCSR {
@@ -224,7 +220,24 @@ func (b *backend) pathIssueSignCert(
 		}
 	}
 
-	resp.Secret.TTL = parsedBundle.Certificate.NotAfter.Sub(time.Now())
+	var resp *logical.Response
+	switch {
+	case role.GenerateLease == nil:
+		return nil, fmt.Errorf("generate lease in role is nil")
+	case *role.GenerateLease == false:
+		// If lease generation is disabled do not populate `Secret` field in
+		// the response
+		resp = &logical.Response{
+			Data: respData,
+		}
+	default:
+		resp = b.Secret(SecretCertsType).Response(
+			respData,
+			map[string]interface{}{
+				"serial_number": cb.SerialNumber,
+			})
+		resp.Secret.TTL = parsedBundle.Certificate.NotAfter.Sub(time.Now())
+	}
 
 	err = req.Storage.Put(&logical.StorageEntry{
 		Key:   "certs/" + cb.SerialNumber,

builtin/logical/pki/path_roles.go

@@ -181,6 +181,20 @@ this value in certificates issued by this role.`,
 				Description: `If set, the O (Organization) will be set to
 this value in certificates issued by this role.`,
 			},
+
+			"generate_lease": &framework.FieldSchema{
+				Type:    framework.TypeBool,
+				Default: false,
+				Description: `
+If set, certificates issued/signed against this role will have Vault leases
+attached to them. Defaults to "false". Certificates can be added to the CRL by
+"vault revoke <lease_id>" when certificates are associated with leases.  It can
+also be done using the "pki/revoke" endpoint. However, when lease generation is
+disabled, invoking "pki/revoke" would be the only way to add the certificates
+to the CRL.  When large number of certificates are generated with long
+lifetimes, it is recommended that lease generation be disabled, as large amount of
+leases adversely affect the startup time of Vault.`,
+			},
 		},
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
@@ -247,6 +261,16 @@ func (b *backend) getRole(s logical.Storage, n string) (*roleEntry, error) {
 		modified = true
 	}
 
+	// Upgrade generate_lease in role
+	if result.GenerateLease == nil {
+		// All the new roles will have GenerateLease always set to a value. A
+		// nil value indicates that this role needs an upgrade. Set it to
+		// `true` to not alter its current behavior.
+		result.GenerateLease = new(bool)
+		*result.GenerateLease = true
+		modified = true
+	}
+
 	if modified {
 		jsonEntry, err := logical.StorageEntryJSON("role/"+n, &result)
 		if err != nil {
@@ -272,7 +296,12 @@ func (b *backend) pathRoleDelete(
 
 func (b *backend) pathRoleRead(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	role, err := b.getRole(req.Storage, data.Get("name").(string))
+	roleName := data.Get("name").(string)
+	if roleName == "" {
+		return logical.ErrorResponse("missing role name"), nil
+	}
+
+	role, err := b.getRole(req.Storage, roleName)
 	if err != nil {
 		return nil, err
 	}
@@ -344,8 +373,11 @@ func (b *backend) pathRoleCreate(
 		KeyUsage:            data.Get("key_usage").(string),
 		OU:                  data.Get("ou").(string),
 		Organization:        data.Get("organization").(string),
+		GenerateLease:       new(bool),
 	}
 
+	*entry.GenerateLease = data.Get("generate_lease").(bool)
+
 	if entry.KeyType == "rsa" && entry.KeyBits < 2048 {
 		return logical.ErrorResponse("RSA keys < 2048 bits are unsafe and not supported"), nil
 	}
@@ -456,10 +488,11 @@ type roleEntry struct {
 	UseCSRCommonName      bool   `json:"use_csr_common_name" structs:"use_csr_common_name" mapstructure:"use_csr_common_name"`
 	KeyType               string `json:"key_type" structs:"key_type" mapstructure:"key_type"`
 	KeyBits               int    `json:"key_bits" structs:"key_bits" mapstructure:"key_bits"`
-	MaxPathLength         *int   `json:",omitempty" structs:",omitempty"`
+	MaxPathLength         *int   `json:",omitempty" structs:"max_path_length,omitempty" mapstructure:"max_path_length"`
 	KeyUsage              string `json:"key_usage" structs:"key_usage" mapstructure:"key_usage"`
 	OU                    string `json:"ou" structs:"ou" mapstructure:"ou"`
 	Organization          string `json:"organization" structs:"organization" mapstructure:"organization"`
+	GenerateLease         *bool  `json:"generate_lease,omitempty" structs:"generate_lease,omitempty"`
 }
 
 const pathListRolesHelpSyn = `List the existing roles in this backend`

builtin/logical/pki/path_roles_test.go

@@ -0,0 +1,206 @@
+package pki
+
+import (
+	"testing"
+
+	"github.com/hashicorp/vault/logical"
+	"github.com/mitchellh/mapstructure"
+)
+
+func createBackendWithStorage(t *testing.T) (*backend, logical.Storage) {
+	config := logical.TestBackendConfig()
+	config.StorageView = &logical.InmemStorage{}
+
+	var err error
+	b := Backend()
+	_, err = b.Setup(config)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = b.Initialize()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return b, config.StorageView
+}
+
+func TestPki_RoleGenerateLease(t *testing.T) {
+	var resp *logical.Response
+	var err error
+	b, storage := createBackendWithStorage(t)
+
+	roleData := map[string]interface{}{
+		"allowed_domains": "myvault.com",
+		"ttl":             "5h",
+	}
+
+	roleReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "roles/testrole",
+		Storage:   storage,
+		Data:      roleData,
+	}
+
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	roleReq.Operation = logical.ReadOperation
+
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	// generate_lease cannot be nil. It either has to be set during role
+	// creation or has to be filled in by the upgrade code
+	generateLease := resp.Data["generate_lease"].(*bool)
+	if generateLease == nil {
+		t.Fatalf("generate_lease should not be nil")
+	}
+
+	// By default, generate_lease should be `false`
+	if *generateLease {
+		t.Fatalf("generate_lease should not be set by default")
+	}
+
+	// role.GenerateLease will be nil after the decode
+	var role roleEntry
+	err = mapstructure.Decode(resp.Data, &role)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Make it explicit
+	role.GenerateLease = nil
+
+	entry, err := logical.StorageEntryJSON("role/testrole", role)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := storage.Put(entry); err != nil {
+		t.Fatal(err)
+	}
+
+	// Reading should upgrade generate_lease
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	generateLease = resp.Data["generate_lease"].(*bool)
+	if generateLease == nil {
+		t.Fatalf("generate_lease should not be nil")
+	}
+
+	// Upgrade should set generate_lease to `true`
+	if !*generateLease {
+		t.Fatalf("generate_lease should be set after an upgrade")
+	}
+
+	// Make sure that setting generate_lease to `true` works properly
+	roleReq.Operation = logical.UpdateOperation
+	roleReq.Path = "roles/testrole2"
+	roleReq.Data["generate_lease"] = true
+
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	roleReq.Operation = logical.ReadOperation
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	generateLease = resp.Data["generate_lease"].(*bool)
+	if generateLease == nil {
+		t.Fatalf("generate_lease should not be nil")
+	}
+	if !*generateLease {
+		t.Fatalf("generate_lease should have been set")
+	}
+}
+
+func TestPki_CertsLease(t *testing.T) {
+	var resp *logical.Response
+	var err error
+	b, storage := createBackendWithStorage(t)
+
+	caData := map[string]interface{}{
+		"common_name": "myvault.com",
+		"ttl":         "5h",
+		"ip_sans":     "127.0.0.1",
+	}
+
+	caReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "root/generate/internal",
+		Storage:   storage,
+		Data:      caData,
+	}
+
+	resp, err = b.HandleRequest(caReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	roleData := map[string]interface{}{
+		"allowed_domains":  "myvault.com",
+		"allow_subdomains": true,
+		"ttl":              "2h",
+	}
+
+	roleReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "roles/testrole",
+		Storage:   storage,
+		Data:      roleData,
+	}
+
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	issueData := map[string]interface{}{
+		"common_name": "cert.myvault.com",
+		"format":      "pem",
+		"ip_sans":     "127.0.0.1",
+	}
+	issueReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "issue/testrole",
+		Storage:   storage,
+		Data:      issueData,
+	}
+
+	resp, err = b.HandleRequest(issueReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	if resp.Secret != nil {
+		t.Fatalf("expected a response that does not contain a secret")
+	}
+
+	// Turn on the lease generation and issue a certificate. The response
+	// should have a `Secret` object populated.
+	roleData["generate_lease"] = true
+
+	resp, err = b.HandleRequest(roleReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	resp, err = b.HandleRequest(issueReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v resp: %#v", err, resp)
+	}
+
+	if resp.Secret == nil {
+		t.Fatalf("expected a response that contains a secret")
+	}
+}

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

@@ -1235,6 +1235,19 @@ subpath for interactive help output.
         This sets the O (Organization) values in the subject field of issued
         certificates. This is a comma-separated string.
       </li>
+      <li>
+        <span class="param">generate_lease</span>
+        <span class="param-flags">optional</span>
+        If set, certificates issued/signed against this role will have Vault
+        leases attached to them. Defaults to "false". Certificates can be added
+        to the CRL by `vault revoke <lease_id>` when certificates are
+        associated with leases.  It can also be done using the `pki/revoke`
+        endpoint. However, when lease generation is disabled, invoking
+        `pki/revoke` would be the only way to add the certificates to the CRL.
+        When large number of certificates are generated with long lifetimes, it
+        is recommended that lease generation be disabled, as large amount of
+        leases adversely affect the startup time of Vault.
+      </li>
     </ul>
   </dd>