From 0cc6d95ae6235f08acf3f57214dd4455c70e933b Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 4 Apr 2017 04:48:44 -0700 Subject: [PATCH 1/3] implement a no_store option for pki roles, refs #2511 --- builtin/logical/pki/path_issue_sign.go | 10 +- builtin/logical/pki/path_roles.go | 28 ++++- builtin/logical/pki/path_roles_test.go | 150 +++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 5 deletions(-) diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index 803ef7f38539..f25619761a7b 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -240,10 +240,12 @@ func (b *backend) pathIssueSignCert( resp.Secret.TTL = parsedBundle.Certificate.NotAfter.Sub(time.Now()) } - err = req.Storage.Put(&logical.StorageEntry{ - Key: "certs/" + cb.SerialNumber, - Value: parsedBundle.CertificateBytes, - }) + if !*role.NoStore { + 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: %v", err) } diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 0eadc6ff293a..6e6380b338c1 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -204,6 +204,17 @@ 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.`, }, + "no_store": &framework.FieldSchema{ + Type: framework.TypeBool, + Default: false, + Description: ` +If set, certificates issued/signed against this role will not be stored in the +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".`, + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -280,6 +291,13 @@ func (b *backend) getRole(s logical.Storage, n string) (*roleEntry, error) { modified = true } + // analogous upgrade for no_store + if result.NoStore == nil { + result.NoStore = new(bool) + *result.NoStore = false + modified = true + } + if modified { jsonEntry, err := logical.StorageEntryJSON("role/"+n, &result) if err != nil { @@ -384,9 +402,16 @@ func (b *backend) pathRoleCreate( OU: data.Get("ou").(string), Organization: data.Get("organization").(string), GenerateLease: new(bool), + NoStore: new(bool), } - *entry.GenerateLease = data.Get("generate_lease").(bool) + *entry.NoStore = data.Get("no_store").(bool) + // no_store implies generate_lease := false + if *entry.NoStore { + *entry.GenerateLease = false + } else { + *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 @@ -504,6 +529,7 @@ type roleEntry struct { 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"` + NoStore *bool `json:"no_store,omitempty" structs:"no_store,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 index 1efafecfa9a7..64b60e2b6747 100644 --- a/builtin/logical/pki/path_roles_test.go +++ b/builtin/logical/pki/path_roles_test.go @@ -124,6 +124,156 @@ func TestPki_RoleGenerateLease(t *testing.T) { } } +func TestPki_RoleNoStore(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) + } + + // no_store cannot be nil + noStore := resp.Data["no_store"].(*bool) + if noStore == nil { + t.Fatalf("no_store should not be nil") + } + + // By default, generate_lease should be `false` + if *noStore { + t.Fatalf("no_store 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.NoStore = 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 no_store + resp, err = b.HandleRequest(roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: err: %v resp: %#v", err, resp) + } + + noStore = resp.Data["no_store"].(*bool) + if noStore == nil { + t.Fatalf("no_store should not be nil") + } + + // Upgrade should set no_store to `false` + if *noStore { + t.Fatalf("no_store should be false after an upgrade") + } + + // Make sure that setting no_store to `true` works properly + roleReq.Operation = logical.UpdateOperation + roleReq.Path = "roles/testrole_nostore" + roleReq.Data["no_store"] = true + roleReq.Data["allowed_domain"] = "myvault.com" + roleReq.Data["allow_subdomains"] = true + roleReq.Data["ttl"] = "5h" + + 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) + } + + noStore = resp.Data["no_store"].(*bool) + if noStore == nil { + t.Fatalf("no_store should not be nil") + } + if !*noStore { + t.Fatalf("no_store should have been set to true") + } + + // issue a certificate and test that it's not stored + 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) + } + + issueData := map[string]interface{}{ + "common_name": "cert.myvault.com", + "format": "pem", + "ip_sans": "127.0.0.1", + "ttl": "1h", + } + issueReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "issue/testrole_nostore", + 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) + } + + // list certs + resp, err = b.HandleRequest(&logical.Request{ + Operation: logical.ListOperation, + Path: "certs", + Storage: storage, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("bad: err: %v resp: %#v", err, resp) + } + if len(resp.Data["keys"].([]string)) != 1 { + t.Fatalf("Only the CA certificate should be stored: %#v", resp) + } +} + func TestPki_CertsLease(t *testing.T) { var resp *logical.Response var err error From f4b9bac22304e35cffec1504bf471573e3223a94 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 4 Apr 2017 15:07:44 -0700 Subject: [PATCH 2/3] review fixes --- builtin/logical/pki/path_issue_sign.go | 2 +- builtin/logical/pki/path_roles.go | 14 ++---- builtin/logical/pki/path_roles_test.go | 52 ++------------------- website/source/api/secret/pki/index.html.md | 7 +++ 4 files changed, 16 insertions(+), 59 deletions(-) diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index f25619761a7b..f180d9527af0 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -240,7 +240,7 @@ func (b *backend) pathIssueSignCert( resp.Secret.TTL = parsedBundle.Certificate.NotAfter.Sub(time.Now()) } - if !*role.NoStore { + if !role.NoStore { err = req.Storage.Put(&logical.StorageEntry{ Key: "certs/" + cb.SerialNumber, Value: parsedBundle.CertificateBytes, diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 6e6380b338c1..2095fbde3c55 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -291,13 +291,6 @@ func (b *backend) getRole(s logical.Storage, n string) (*roleEntry, error) { modified = true } - // analogous upgrade for no_store - if result.NoStore == nil { - result.NoStore = new(bool) - *result.NoStore = false - modified = true - } - if modified { jsonEntry, err := logical.StorageEntryJSON("role/"+n, &result) if err != nil { @@ -402,12 +395,11 @@ func (b *backend) pathRoleCreate( OU: data.Get("ou").(string), Organization: data.Get("organization").(string), GenerateLease: new(bool), - NoStore: new(bool), + NoStore: data.Get("no_store").(bool), } - *entry.NoStore = data.Get("no_store").(bool) // no_store implies generate_lease := false - if *entry.NoStore { + if entry.NoStore { *entry.GenerateLease = false } else { *entry.GenerateLease = data.Get("generate_lease").(bool) @@ -529,7 +521,7 @@ type roleEntry struct { 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"` - NoStore *bool `json:"no_store,omitempty" structs:"no_store,omitempty"` + NoStore bool `json:"no_store" structs:"no_store" mapstructure:"no_store"` } 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 index 64b60e2b6747..82772b0cde3e 100644 --- a/builtin/logical/pki/path_roles_test.go +++ b/builtin/logical/pki/path_roles_test.go @@ -153,51 +153,12 @@ func TestPki_RoleNoStore(t *testing.T) { t.Fatalf("bad: err: %v resp: %#v", err, resp) } - // no_store cannot be nil - noStore := resp.Data["no_store"].(*bool) - if noStore == nil { - t.Fatalf("no_store should not be nil") - } - - // By default, generate_lease should be `false` - if *noStore { + // By default, no_store should be `false` + noStore := resp.Data["no_store"].(bool) + if noStore { t.Fatalf("no_store 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.NoStore = 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 no_store - resp, err = b.HandleRequest(roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("bad: err: %v resp: %#v", err, resp) - } - - noStore = resp.Data["no_store"].(*bool) - if noStore == nil { - t.Fatalf("no_store should not be nil") - } - - // Upgrade should set no_store to `false` - if *noStore { - t.Fatalf("no_store should be false after an upgrade") - } - // Make sure that setting no_store to `true` works properly roleReq.Operation = logical.UpdateOperation roleReq.Path = "roles/testrole_nostore" @@ -217,11 +178,8 @@ func TestPki_RoleNoStore(t *testing.T) { t.Fatalf("bad: err: %v resp: %#v", err, resp) } - noStore = resp.Data["no_store"].(*bool) - if noStore == nil { - t.Fatalf("no_store should not be nil") - } - if !*noStore { + noStore = resp.Data["no_store"].(bool) + if !noStore { t.Fatalf("no_store should have been set to true") } diff --git a/website/source/api/secret/pki/index.html.md b/website/source/api/secret/pki/index.html.md index e5a9e3c81949..ecfb73dfdbd6 100644 --- a/website/source/api/secret/pki/index.html.md +++ b/website/source/api/secret/pki/index.html.md @@ -728,6 +728,13 @@ that is not allowed by the CN policy in the role, the request is denied. disabled, as large amount of leases adversely affect the startup time of Vault. +- `no_store` `(bool: false)` – If set, certificates issued/signed against this +role will not be stored in the 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`. + ### Sample Payload ```json From c3b2e39aed316b36a10ccf703c79862dafce5679 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Thu, 6 Apr 2017 17:36:45 -0700 Subject: [PATCH 3/3] review fix --- builtin/logical/pki/path_issue_sign.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index f180d9527af0..429d96315b7b 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -245,9 +245,9 @@ func (b *backend) pathIssueSignCert( Key: "certs/" + cb.SerialNumber, Value: parsedBundle.CertificateBytes, }) - } - if err != nil { - return nil, fmt.Errorf("Unable to store certificate locally: %v", err) + if err != nil { + return nil, fmt.Errorf("Unable to store certificate locally: %v", err) + } } return resp, nil