From c6f138bb9a08c3911a975312219df66c71a926ff Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 24 Feb 2017 12:12:40 -0500 Subject: [PATCH] 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 | 1 + builtin/logical/pki/path_issue_sign.go | 55 +++-- builtin/logical/pki/path_roles.go | 37 +++- builtin/logical/pki/path_roles_test.go | 206 ++++++++++++++++++ website/source/docs/secrets/pki/index.html.md | 13 ++ 5 files changed, 289 insertions(+), 23 deletions(-) create mode 100644 builtin/logical/pki/path_roles_test.go diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 4cb2c9acc..98763c5ef 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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 { diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index a32b6af5e..d32d9a345 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/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, diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 879952207..49bebe10a 100644 --- a/builtin/logical/pki/path_roles.go +++ b/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 " 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` diff --git a/builtin/logical/pki/path_roles_test.go b/builtin/logical/pki/path_roles_test.go new file mode 100644 index 000000000..1efafecfa --- /dev/null +++ b/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") + } +} diff --git a/website/source/docs/secrets/pki/index.html.md b/website/source/docs/secrets/pki/index.html.md index 985cbcdb3..87d5a89ae 100644 --- a/website/source/docs/secrets/pki/index.html.md +++ b/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. +
  • + generate_lease + optional + 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 ` 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. +