From d49390578bc28da48891a51c989cab8716b6ae34 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 14 Aug 2017 18:49:18 -0400 Subject: [PATCH 1/4] Make PKI root generation idempotent-ish and add delete endpoint. Fixes #3086 --- builtin/logical/pki/backend.go | 3 +- builtin/logical/pki/backend_test.go | 78 +++++++++++++++++++++ builtin/logical/pki/path_config_ca.go | 4 +- builtin/logical/pki/path_fetch.go | 4 +- builtin/logical/pki/path_root.go | 38 +++++++++- website/source/api/secret/pki/index.html.md | 28 ++++++-- 6 files changed, 144 insertions(+), 11 deletions(-) diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index 061ad0ea487e..e9c47fef99c5 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -45,9 +45,10 @@ func Backend() *backend { pathListRoles(&b), pathRoles(&b), pathGenerateRoot(&b), + pathSignIntermediate(&b), + pathDeleteRoot(&b), pathGenerateIntermediate(&b), pathSetSignedIntermediate(&b), - pathSignIntermediate(&b), pathConfigCA(&b), pathConfigCRL(&b), pathConfigURLs(&b), diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index d50a0f40de12..09bc654ddce4 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -22,10 +22,13 @@ import ( "time" "github.com/fatih/structs" + "github.com/hashicorp/vault/api" "github.com/hashicorp/vault/helper/certutil" "github.com/hashicorp/vault/helper/strutil" + vaulthttp "github.com/hashicorp/vault/http" "github.com/hashicorp/vault/logical" logicaltest "github.com/hashicorp/vault/logical/testing" + "github.com/hashicorp/vault/vault" "github.com/mitchellh/mapstructure" ) @@ -2156,6 +2159,81 @@ func TestBackend_SignVerbatim(t *testing.T) { } } +func TestBackend_Root_Idempotentcy(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("pki", &api.MountInput{ + Type: "pki", + Config: api.MountConfigInput{ + DefaultLeaseTTL: "16h", + MaxLeaseTTL: "32h", + }, + }) + if err != nil { + t.Fatal(err) + } + + resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ + "common_name": "myvault.com", + }) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("expected ca info") + } + resp, err = client.Logical().Read("pki/cert/ca_chain") + r1Data := resp.Data + + // Try again, make sure it's a 204 and same CA + resp, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ + "common_name": "myvault.com", + }) + if err != nil { + t.Fatal(err) + } + if resp != nil { + t.Fatal("expected no ca info") + } + resp, err = client.Logical().Read("pki/cert/ca_chain") + r2Data := resp.Data + if !reflect.DeepEqual(r1Data, r2Data) { + t.Fatal("got different ca certs") + } + + resp, err = client.Logical().Delete("pki/root") + if err != nil { + t.Fatal(err) + } + if resp != nil { + t.Fatal("expected nil response") + } + // Make sure it behaves the same + resp, err = client.Logical().Delete("pki/root") + if err != nil { + t.Fatal(err) + } + if resp != nil { + t.Fatal("expected nil response") + } + + _, err = client.Logical().Read("pki/cert/ca_chain") + if err == nil { + t.Fatal("expected error") + } +} + const ( rsaCAKey string = `-----BEGIN RSA PRIVATE KEY----- MIIEogIBAAKCAQEAmPQlK7xD5p+E8iLQ8XlVmll5uU2NKMxKY3UF5tbh+0vkc+Fy diff --git a/builtin/logical/pki/path_config_ca.go b/builtin/logical/pki/path_config_ca.go index c182553d54b8..347ac0105a67 100644 --- a/builtin/logical/pki/path_config_ca.go +++ b/builtin/logical/pki/path_config_ca.go @@ -16,9 +16,7 @@ func pathConfigCA(b *backend) *framework.Path { "pem_bundle": &framework.FieldSchema{ Type: framework.TypeString, Description: `PEM-format, concatenated unencrypted -secret key and certificate, or, if a -CSR was generated with the "generate" -endpoint, just the signed certificate.`, +secret key and certificate.`, }, }, diff --git a/builtin/logical/pki/path_fetch.go b/builtin/logical/pki/path_fetch.go index ed60e755d355..32a9a47646be 100644 --- a/builtin/logical/pki/path_fetch.go +++ b/builtin/logical/pki/path_fetch.go @@ -159,7 +159,7 @@ func (b *backend) pathFetchRead(req *logical.Request, data *framework.FieldData) caInfo, err := fetchCAInfo(req) switch err.(type) { case errutil.UserError: - response = logical.ErrorResponse(funcErr.Error()) + response = logical.ErrorResponse(err.Error()) goto reply case errutil.InternalError: retErr = err @@ -244,6 +244,8 @@ reply: } case retErr != nil: response = nil + case response.IsError(): + return response, nil default: response.Data["certificate"] = string(certificate) response.Data["revocation_time"] = revocationTime diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index d02953133cb5..5ed49ebf4f75 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -28,6 +28,21 @@ func pathGenerateRoot(b *backend) *framework.Path { return ret } +func pathDeleteRoot(b *backend) *framework.Path { + ret := &framework.Path{ + Pattern: "root", + + Callbacks: map[logical.Operation]framework.OperationFunc{ + logical.DeleteOperation: b.pathCADeleteRoot, + }, + + HelpSynopsis: pathDeleteRootHelpSyn, + HelpDescription: pathDeleteRootHelpDesc, + } + + return ret +} + func pathSignIntermediate(b *backend) *framework.Path { ret := &framework.Path{ Pattern: "root/sign-intermediate", @@ -66,10 +81,23 @@ the non-repudiation flag.`, return ret } +func (b *backend) pathCADeleteRoot( + req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + return nil, req.Storage.Delete("config/ca_bundle") +} + func (b *backend) pathCAGenerateRoot( req *logical.Request, data *framework.FieldData) (*logical.Response, error) { var err error + entry, err := req.Storage.Get("config/ca_bundle") + if err != nil { + return nil, err + } + if entry != nil { + return nil, nil + } + exported, format, role, errorResp := b.getGenerationParams(data) if errorResp != nil { return errorResp, nil @@ -133,7 +161,7 @@ func (b *backend) pathCAGenerateRoot( } // Store it as the CA bundle - entry, err := logical.StorageEntryJSON("config/ca_bundle", cb) + entry, err = logical.StorageEntryJSON("config/ca_bundle", cb) if err != nil { return nil, err } @@ -299,6 +327,14 @@ const pathGenerateRootHelpDesc = ` See the API documentation for more information. ` +const pathDeleteRootHelpSyn = ` +Deletes the root CA key to allow a new one to be generated. +` + +const pathDeleteRootHelpDesc = ` +See the API documentation for more information. +` + const pathSignIntermediateHelpSyn = ` Issue an intermediate CA certificate based on the provided CSR. ` diff --git a/website/source/api/secret/pki/index.html.md b/website/source/api/secret/pki/index.html.md index c90f0ab4ddc0..218185628bc9 100644 --- a/website/source/api/secret/pki/index.html.md +++ b/website/source/api/secret/pki/index.html.md @@ -47,8 +47,9 @@ update your API calls accordingly. ## Read CA Certificate This endpoint retrieves the CA certificate *in raw DER-encoded form*. This is a -bare endpoint that does not return a standard Vault data structure. If `/pem` is -added to the endpoint, the CA certificate is returned in PEM format. +bare endpoint that does not return a standard Vault data structure and cannot +be read by the Vault CLI. If `/pem` is added to the endpoint, the CA +certificate is returned in PEM format. This is an unauthenticated endpoint. @@ -73,7 +74,7 @@ $ curl \ This endpoint retrieves the CA certificate chain, including the CA _in PEM format_. This is a bare endpoint that does not return a standard Vault data -structure. +structure and cannot be read by the Vault CLI. This is an unauthenticated endpoint. @@ -460,8 +461,6 @@ $ curl \ https://vault.rocks/v1/pki/intermediate/generate/internal ``` -### Sample Response - ```json { "lease_id": "", @@ -974,6 +973,25 @@ $ curl \ } ``` +## Delete Root + +This endpoint deletes the current CA key (the old CA certificate will still be +accessible for reading until a new certificate/key are generated or uploaded). + +| Method | Path | Produces | +| :------- | :--------------------------- | :--------------------- | +| `DELETE` | `/pki/root` | `204 (empty body)` | + + +### Sample Request + +``` +$ curl \ + --header "X-Vault-Token: ..." \ + --request DELETE \ + https://vault.rocks/v1/pki/root +``` + ## Sign Intermediate This endpoint uses the configured CA certificate to issue a certificate with From 14efd4ee7109e94368fa20bd023819eb564c4115 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 14 Aug 2017 20:57:55 -0400 Subject: [PATCH 2/4] Fix tests and update docs --- CHANGELOG.md | 7 +++++ builtin/logical/pki/backend.go | 4 +++ builtin/logical/pki/backend_test.go | 31 ++++++++++++++----- builtin/logical/pki/path_fetch.go | 5 ++- website/source/api/secret/pki/index.html.md | 20 +++++++----- .../source/api/secret/rabbitmq/index.html.md | 3 +- 6 files changed, 52 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 464a029b7659..fb1ba29d9ea4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ ## 0.8.1 (Unreleased) +DEPRECATIONS/CHANGES: + + * PKI Root Generation: Calling `pki/root/generate` when a CA cert/key already + exists will now return a `204` instead of overwriting an existing root. If + you want to recreate the root, first run a delete operation on `pki/root` + (requires `sudo` capability), then generate it again. + IMPROVEMENTS: * plugins: Send logs through Vault's logger rather than stdout [GH-3142] diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index e9c47fef99c5..9d06b5129897 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -39,6 +39,10 @@ func Backend() *backend { "crl", "certs/", }, + + Root: []string{ + "root", + }, }, Paths: []*framework.Path{ diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 09bc654ddce4..001bf7d2b7b0 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -651,6 +651,11 @@ func generateCSRSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s ErrorOk: true, }, + logicaltest.TestStep{ + Operation: logical.DeleteOperation, + Path: "root", + }, + logicaltest.TestStep{ Operation: logical.UpdateOperation, Path: "root/generate/exported", @@ -868,6 +873,11 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int }, // Test a bunch of generation stuff + logicaltest.TestStep{ + Operation: logical.DeleteOperation, + Path: "root", + }, + logicaltest.TestStep{ Operation: logical.UpdateOperation, Path: "root/generate/exported", @@ -1000,6 +1010,11 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int }, // Do it all again, with EC keys and DER format + logicaltest.TestStep{ + Operation: logical.DeleteOperation, + Path: "root", + }, + logicaltest.TestStep{ Operation: logical.UpdateOperation, Path: "root/generate/exported", @@ -1221,7 +1236,7 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int Operation: logical.ReadOperation, PreFlight: setSerialUnderTest, Check: func(resp *logical.Response) error { - if resp.Data["error"] != nil && resp.Data["error"].(string) != "" { + if resp != nil && resp.Data["error"] != nil && resp.Data["error"].(string) != "" { return fmt.Errorf("got an error: %s", resp.Data["error"].(string)) } @@ -1235,7 +1250,7 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int Operation: logical.ReadOperation, PreFlight: setSerialUnderTest, Check: func(resp *logical.Response) error { - if resp.Data["error"] != nil && resp.Data["error"].(string) != "" { + if resp != nil && resp.Data["error"] != nil && resp.Data["error"].(string) != "" { return fmt.Errorf("got an error: %s", resp.Data["error"].(string)) } @@ -1293,7 +1308,7 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int Operation: logical.ReadOperation, PreFlight: setSerialUnderTest, Check: func(resp *logical.Response) error { - if resp.Data["error"] != nil && resp.Data["error"].(string) != "" { + if resp != nil && resp.Data["error"] != nil && resp.Data["error"].(string) != "" { return fmt.Errorf("got an error: %s", resp.Data["error"].(string)) } @@ -1307,7 +1322,7 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int Operation: logical.ReadOperation, PreFlight: setSerialUnderTest, Check: func(resp *logical.Response) error { - if resp.Data["error"] != nil && resp.Data["error"].(string) != "" { + if resp != nil && resp.Data["error"] != nil && resp.Data["error"].(string) != "" { return fmt.Errorf("got an error: %s", resp.Data["error"].(string)) } @@ -1333,8 +1348,8 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int Operation: logical.ReadOperation, PreFlight: setSerialUnderTest, Check: func(resp *logical.Response) error { - if resp.Data["error"] == nil || resp.Data["error"].(string) == "" { - return fmt.Errorf("didn't get an expected error") + if resp != nil { + return fmt.Errorf("expected no response") } serialUnderTest = "cert/" + reqdata["ec_int_serial_number"].(string) @@ -1347,8 +1362,8 @@ func generateCATestingSteps(t *testing.T, caCert, caKey, otherCaCert string, int Operation: logical.ReadOperation, PreFlight: setSerialUnderTest, Check: func(resp *logical.Response) error { - if resp.Data["error"] == nil || resp.Data["error"].(string) == "" { - return fmt.Errorf("didn't get an expected error") + if resp != nil { + return fmt.Errorf("expected no response") } serialUnderTest = "cert/" + reqdata["rsa_int_serial_number"].(string) diff --git a/builtin/logical/pki/path_fetch.go b/builtin/logical/pki/path_fetch.go index 32a9a47646be..cf71b4c35c69 100644 --- a/builtin/logical/pki/path_fetch.go +++ b/builtin/logical/pki/path_fetch.go @@ -189,7 +189,7 @@ func (b *backend) pathFetchRead(req *logical.Request, data *framework.FieldData) } } if certEntry == nil { - response = logical.ErrorResponse(fmt.Sprintf("certificate with serial %s not found", serial)) + response = nil goto reply } @@ -244,6 +244,9 @@ reply: } case retErr != nil: response = nil + return + case response == nil: + return case response.IsError(): return response, nil default: diff --git a/website/source/api/secret/pki/index.html.md b/website/source/api/secret/pki/index.html.md index 218185628bc9..2df20d7e1262 100644 --- a/website/source/api/secret/pki/index.html.md +++ b/website/source/api/secret/pki/index.html.md @@ -39,6 +39,7 @@ update your API calls accordingly. * [List Roles](#list-roles) * [Delete Role](#delete-role) * [Generate Root](#generate-root) +* [Delete Root](#delete-root) * [Sign Intermediate](#sign-intermediate) * [Sign Certificate](#sign-certificate) * [Sign Verbatim](#sign-verbatim) @@ -881,14 +882,18 @@ $ curl \ ## Generate Root -This endpoint generates a new self-signed CA certificate and private key. _This -will 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 `config/urls`. +This endpoint generates a new self-signed CA certificate and private key. 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 `config/urls`. -As with other issued certificates, Vault will automatically revoke the generated -root at the end of its lease period; the CA certificate will sign its own CRL. +As with other issued certificates, Vault will automatically revoke the +generated root at the end of its lease period; the CA certificate will sign its +own CRL. + +As of Vault 0.8.1, if a CA cert/key already exists within the backend, this +function will return a 204 and will not overwrite it. Previous versions of +Vault would overwrite the existing cert/key with new values. | Method | Path | Produces | | :------- | :--------------------------- | :--------------------- | @@ -977,6 +982,7 @@ $ curl \ This endpoint deletes the current CA key (the old CA certificate will still be accessible for reading until a new certificate/key are generated or uploaded). +_This endpoint requires sudo/root privileges._ | Method | Path | Produces | | :------- | :--------------------------- | :--------------------- | diff --git a/website/source/api/secret/rabbitmq/index.html.md b/website/source/api/secret/rabbitmq/index.html.md index 3948928141a9..e3605ddc2e93 100644 --- a/website/source/api/secret/rabbitmq/index.html.md +++ b/website/source/api/secret/rabbitmq/index.html.md @@ -61,8 +61,7 @@ $ curl \ ## Configure Lease -This endpoint configures the lease settings for generated credentials. This is -endpoint requires sudo privileges. +This endpoint configures the lease settings for generated credentials. | Method | Path | Produces | | :------- | :--------------------------- | :--------------------- | From 95560c2ee47a1b2ac47ec30072e4c827423b09ef Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 15 Aug 2017 00:08:15 -0400 Subject: [PATCH 3/4] Bump Travis wait time --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 88ae62202045..58b5939e380e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,5 +20,5 @@ branches: script: - make bootstrap - - travis_wait 30 make test - - travis_wait 30 make testrace + - travis_wait 45 make test + - travis_wait 45 make testrace From 29a34dbfa89765408de3c0ed224d2973731ab861 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 15 Aug 2017 10:22:03 -0400 Subject: [PATCH 4/4] Add some more tests --- builtin/logical/pki/backend_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 001bf7d2b7b0..47d2f6d7aca0 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2247,6 +2247,21 @@ func TestBackend_Root_Idempotentcy(t *testing.T) { if err == nil { t.Fatal("expected error") } + + resp, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{ + "common_name": "myvault.com", + }) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("expected ca info") + } + + _, err = client.Logical().Read("pki/cert/ca_chain") + if err != nil { + t.Fatal(err) + } } const (