From 2398634862d327642f126263308878772fccf36c Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 5 Dec 2022 10:40:39 -0500 Subject: [PATCH] Respond with data to all writes in PKI engine (#18222) * Respond with data to all writes in PKI engine Signed-off-by: Alexander Scheel * Add changelog Signed-off-by: Alexander Scheel Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 2 +- builtin/logical/pki/crl_test.go | 2 +- builtin/logical/pki/integration_test.go | 2 +- builtin/logical/pki/ocsp_test.go | 4 ++-- builtin/logical/pki/path_config_crl.go | 13 ++++++++++++- builtin/logical/pki/path_resign_crls_test.go | 4 ++-- builtin/logical/pki/path_roles.go | 6 ++---- builtin/logical/pki/path_tidy.go | 18 +++++++++++++++++- builtin/logical/pki/storage_migrations_test.go | 4 ++-- changelog/18222.txt | 3 +++ command/patch_test.go | 8 ++++---- 11 files changed, 47 insertions(+), 19 deletions(-) create mode 100644 changelog/18222.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index c6a79dbfa..d29711879 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -5914,7 +5914,7 @@ func TestPKI_ListRevokedCerts(t *testing.T) { "allow_subdomains": "true", "max_ttl": "1h", }) - requireSuccessNilResponse(t, resp, err, "error setting up pki role") + requireSuccessNonNilResponse(t, resp, err, "error setting up pki role") resp, err = CBWrite(b, s, "issue/test", map[string]interface{}{ "common_name": "test1.test.com", diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index f1d3aa895..89e728ed2 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -89,7 +89,7 @@ func TestBackend_CRLConfig(t *testing.T) { "auto_rebuild": tc.autoRebuild, "auto_rebuild_grace_period": tc.autoRebuildGracePeriod, }) - requireSuccessNilResponse(t, resp, err) + requireSuccessNonNilResponse(t, resp, err) resp, err = CBRead(b, s, "config/crl") requireSuccessNonNilResponse(t, resp, err) diff --git a/builtin/logical/pki/integration_test.go b/builtin/logical/pki/integration_test.go index 6749593c8..c2bdffbde 100644 --- a/builtin/logical/pki/integration_test.go +++ b/builtin/logical/pki/integration_test.go @@ -286,7 +286,7 @@ func TestIntegration_SetSignedWithBackwardsPemBundles(t *testing.T) { MountPoint: "pki-int/", }) require.NoError(t, err, "failed setting up role example") - require.Nil(t, resp, "got non-nil response from setting up role example: %#v", resp) + require.NotNil(t, resp, "got nil response from setting up role example: %#v", resp) // Issue cert resp, err = intBackend.HandleRequest(context.Background(), &logical.Request{ diff --git a/builtin/logical/pki/ocsp_test.go b/builtin/logical/pki/ocsp_test.go index 8dc6070d8..f7bf69ac5 100644 --- a/builtin/logical/pki/ocsp_test.go +++ b/builtin/logical/pki/ocsp_test.go @@ -40,7 +40,7 @@ func TestOcsp_Disabled(t *testing.T) { resp, err := CBWrite(b, s, "config/crl", map[string]interface{}{ "ocsp_disable": "true", }) - requireSuccessNilResponse(t, resp, err) + requireSuccessNonNilResponse(t, resp, err) resp, err = SendOcspRequest(t, b, s, localTT.reqType, testEnv.leafCertIssuer1, testEnv.issuer1, crypto.SHA1) require.NoError(t, err) requireFieldsSetInResp(t, resp, "http_content_type", "http_status_code", "http_raw_body") @@ -538,7 +538,7 @@ func setupOcspEnvWithCaKeyConfig(t *testing.T, keyType string, caKeyBits int, ca "issuer_ref": issuerId, "key_type": keyType, }) - requireSuccessNilResponse(t, resp, err, "roles/test"+strconv.FormatInt(int64(i), 10)) + requireSuccessNonNilResponse(t, resp, err, "roles/test"+strconv.FormatInt(int64(i), 10)) resp, err = CBWrite(b, s, "issue/test"+strconv.FormatInt(int64(i), 10), map[string]interface{}{ "common_name": "test.foobar.com", diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index 30b8047b5..6875684d9 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -227,7 +227,18 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra } } - return nil, nil + return &logical.Response{ + Data: map[string]interface{}{ + "expiry": config.Expiry, + "disable": config.Disable, + "ocsp_disable": config.OcspDisable, + "ocsp_expiry": config.OcspExpiry, + "auto_rebuild": config.AutoRebuild, + "auto_rebuild_grace_period": config.AutoRebuildGracePeriod, + "enable_delta": config.EnableDelta, + "delta_rebuild_interval": config.DeltaRebuildInterval, + }, + }, nil } const pathConfigCRLHelpSyn = ` diff --git a/builtin/logical/pki/path_resign_crls_test.go b/builtin/logical/pki/path_resign_crls_test.go index 4b3d9be03..65ffcf95e 100644 --- a/builtin/logical/pki/path_resign_crls_test.go +++ b/builtin/logical/pki/path_resign_crls_test.go @@ -426,14 +426,14 @@ func setupResignCrlMounts(t *testing.T, b1 *backend, s1 logical.Storage, b2 *bac "allow_subdomains": "true", "max_ttl": "1h", }) - requireSuccessNilResponse(t, resp, err, "error setting up pki role on backend 1") + requireSuccessNonNilResponse(t, resp, err, "error setting up pki role on backend 1") resp, err = CBWrite(b2, s2, "roles/test", map[string]interface{}{ "allowed_domains": "test.com", "allow_subdomains": "true", "max_ttl": "1h", }) - requireSuccessNilResponse(t, resp, err, "error setting up pki role on backend 2") + requireSuccessNonNilResponse(t, resp, err, "error setting up pki role on backend 2") // Issue and revoke a cert in backend 1 resp, err = CBWrite(b1, s1, "issue/test", map[string]interface{}{ diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 89a899a8c..93d0a53ba 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -745,9 +745,6 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data return nil, err } if warning != "" { - if resp == nil { - resp = &logical.Response{} - } resp.AddWarning(warning) } if resp.IsError() { @@ -767,7 +764,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data } func validateRole(b *backend, entry *roleEntry, ctx context.Context, s logical.Storage) (*logical.Response, error) { - var resp *logical.Response + resp := &logical.Response{} var err error if entry.MaxTTL > 0 && entry.TTL > entry.MaxTTL { @@ -828,6 +825,7 @@ func validateRole(b *backend, entry *roleEntry, ctx context.Context, s logical.S return nil, errutil.UserError{Err: err.Error()} } + resp.Data = entry.ToResponseData() return resp, nil } diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index ae5f160ec..9ee18251c 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -736,7 +736,23 @@ func (b *backend) pathConfigAutoTidyWrite(ctx context.Context, req *logical.Requ return logical.ErrorResponse("Auto-tidy enabled but no tidy operations were requested. Enable at least one tidy operation to be run (tidy_cert_store / tidy_revoked_certs / tidy_revoked_cert_issuer_associations)."), nil } - return nil, sc.writeAutoTidyConfig(config) + if err := sc.writeAutoTidyConfig(config); err != nil { + return nil, err + } + + return &logical.Response{ + Data: map[string]interface{}{ + "enabled": config.Enabled, + "interval_duration": int(config.Interval / time.Second), + "tidy_cert_store": config.CertStore, + "tidy_revoked_certs": config.RevokedCerts, + "tidy_revoked_cert_issuer_associations": config.IssuerAssocs, + "tidy_expired_issuers": config.ExpiredIssuers, + "safety_buffer": int(config.SafetyBuffer / time.Second), + "issuer_safety_buffer": int(config.IssuerSafetyBuffer / time.Second), + "pause_duration": config.PauseDuration.String(), + }, + }, nil } func (b *backend) tidyStatusStart(config *tidyConfig) { diff --git a/builtin/logical/pki/storage_migrations_test.go b/builtin/logical/pki/storage_migrations_test.go index 65678f418..5535d1af6 100644 --- a/builtin/logical/pki/storage_migrations_test.go +++ b/builtin/logical/pki/storage_migrations_test.go @@ -385,7 +385,7 @@ func TestExpectedOpsWork_PreMigration(t *testing.T) { MountPoint: "pki/", }) require.NoError(t, err, "error from creating role") - require.Nil(t, resp, "got non-nil response object from creating role") + require.NotNil(t, resp, "got nil response object from creating role") // List roles resp, err = b.HandleRequest(context.Background(), &logical.Request{ @@ -471,7 +471,7 @@ func TestExpectedOpsWork_PreMigration(t *testing.T) { MountPoint: "pki/", }) require.NoError(t, err, "error setting CRL config") - require.Nil(t, resp, "got non-nil response setting CRL config") + require.NotNil(t, resp, "got nil response setting CRL config") // Set URL config resp, err = b.HandleRequest(context.Background(), &logical.Request{ diff --git a/changelog/18222.txt b/changelog/18222.txt new file mode 100644 index 000000000..a7f822aa7 --- /dev/null +++ b/changelog/18222.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Respond with written data to `config/auto-tidy`, `config/crl`, and `roles/:role`. +``` diff --git a/command/patch_test.go b/command/patch_test.go index 2b3807c52..b4bdd7a62 100644 --- a/command/patch_test.go +++ b/command/patch_test.go @@ -44,13 +44,13 @@ func TestPatchCommand_Run(t *testing.T) { { "force_kvs", []string{"-force", "pki/roles/example"}, - "Success!", + "allow_localhost", 0, }, { "force_f_kvs", []string{"-f", "pki/roles/example"}, - "Success!", + "allow_localhost", 0, }, { @@ -62,13 +62,13 @@ func TestPatchCommand_Run(t *testing.T) { { "single_value", []string{"pki/roles/example", "allow_localhost=true"}, - "Success!", + "allow_localhost", 0, }, { "multi_value", []string{"pki/roles/example", "allow_localhost=true", "allowed_domains=true"}, - "Success!", + "allow_localhost", 0, }, }