From 9bf3d115fc756e9f30c477c6b82746db79fa105a Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 29 Nov 2024 10:22:09 -0500 Subject: [PATCH] Add an option to allow cert-auth to return metadata about client cert that fails login (#29044) * Add an option to allow cert-auth to return metadata about client certs that fail login * Add cl * Update SPDX header for sdk/logical/response_test.go --- builtin/credential/cert/backend_test.go | 34 +++++++++ builtin/credential/cert/path_config.go | 10 +++ builtin/credential/cert/path_login.go | 64 +++++++++++++++- builtin/credential/cert/path_login_test.go | 88 +++++++++++++++++++++- changelog/29044.txt | 3 + sdk/logical/response.go | 9 +++ sdk/logical/response_test.go | 24 ++++++ vault/identity_store_entities.go | 11 +-- website/content/api-docs/auth/cert.mdx | 3 + 9 files changed, 232 insertions(+), 14 deletions(-) create mode 100644 changelog/29044.txt create mode 100644 sdk/logical/response_test.go diff --git a/builtin/credential/cert/backend_test.go b/builtin/credential/cert/backend_test.go index e4affa3b5296..0840362f0bdc 100644 --- a/builtin/credential/cert/backend_test.go +++ b/builtin/credential/cert/backend_test.go @@ -1317,6 +1317,8 @@ func TestBackend_ext_singleCert(t *testing.T) { testAccStepLoginWithMetadata(t, connState, "web", map[string]string{"2-1-1-1": "A UTF8String Extension"}, true), testAccStepCert(t, "web", ca, "foo", allowed{metadata_ext: "1.2.3.45"}, false), testAccStepLoginWithMetadata(t, connState, "web", map[string]string{}, true), + testAccStepSetConfig(t, config{EnableMetadataOnFailures: true}, connState), + testAccStepReadConfig(t, config{EnableMetadataOnFailures: true}, connState), }, }) } @@ -1728,6 +1730,7 @@ func testAccStepSetConfig(t *testing.T, conf config, connState tls.ConnectionSta ConnState: &connState, Data: map[string]interface{}{ "enable_identity_alias_metadata": conf.EnableIdentityAliasMetadata, + "enable_metadata_on_failures": conf.EnableMetadataOnFailures, }, } } @@ -1752,6 +1755,20 @@ func testAccStepReadConfig(t *testing.T, conf config, connState tls.ConnectionSt t.Fatalf("bad: expected enable_identity_alias_metadata to be %t, got %t", conf.EnableIdentityAliasMetadata, b) } + metaValueRaw, ok := resp.Data["enable_metadata_on_failures"] + if !ok { + t.Fatalf("enable_metadata_on_failures not found in response") + } + + metaValue, ok := metaValueRaw.(bool) + if !ok { + t.Fatalf("bad: expected enable_metadata_on_failures to be a bool") + } + + if metaValue != conf.EnableMetadataOnFailures { + t.Fatalf("bad: expected enable_metadata_on_failures to be %t, got %t", conf.EnableMetadataOnFailures, metaValue) + } + return nil }, } @@ -1936,6 +1953,23 @@ func testAccStepCert(t *testing.T, name string, cert []byte, policies string, te return testAccStepCertWithExtraParams(t, name, cert, policies, testData, expectError, nil) } +func testStepEnableMetadataFailures() logicaltest.TestStep { + return logicaltest.TestStep{ + Operation: logical.UpdateOperation, + Path: "config", + ErrorOk: false, + Data: map[string]interface{}{ + "enable_metadata_on_failures": true, + }, + Check: func(resp *logical.Response) error { + if resp != nil && resp.IsError() { + return fmt.Errorf("expected nil response got a response error: %v", resp) + } + return nil + }, + } +} + func testAccStepCertWithExtraParams(t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool, extraParams map[string]interface{}) logicaltest.TestStep { data := map[string]interface{}{ "certificate": string(cert), diff --git a/builtin/credential/cert/path_config.go b/builtin/credential/cert/path_config.go index 1183775f6bc4..c5c32be97d69 100644 --- a/builtin/credential/cert/path_config.go +++ b/builtin/credential/cert/path_config.go @@ -42,6 +42,11 @@ func pathConfig(b *backend) *framework.Path { Default: defaultRoleCacheSize, Description: `The size of the in memory role cache`, }, + "enable_metadata_on_failures": { + Type: framework.TypeBool, + Default: false, + Description: `If set, metadata of the client certificate will be returned on authentication failures.`, + }, }, Operations: map[logical.Operation]framework.OperationHandler{ @@ -87,6 +92,9 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, dat } config.RoleCacheSize = cacheSize } + if enableMetadataOnFailures, ok := data.GetOk("enable_metadata_on_failures"); ok { + config.EnableMetadataOnFailures = enableMetadataOnFailures.(bool) + } if err := b.storeConfig(ctx, req.Storage, config); err != nil { return nil, err } @@ -104,6 +112,7 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, d *f "enable_identity_alias_metadata": cfg.EnableIdentityAliasMetadata, "ocsp_cache_size": cfg.OcspCacheSize, "role_cache_size": cfg.RoleCacheSize, + "enable_metadata_on_failures": cfg.EnableMetadataOnFailures, } return &logical.Response{ @@ -133,4 +142,5 @@ type config struct { EnableIdentityAliasMetadata bool `json:"enable_identity_alias_metadata"` OcspCacheSize int `json:"ocsp_cache_size"` RoleCacheSize int `json:"role_cache_size"` + EnableMetadataOnFailures bool `json:"enable_metadata_on_failures"` } diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 53571b26185e..d770b1b9da3a 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -70,12 +70,20 @@ func (b *backend) loginPathWrapper(wrappedOp func(ctx context.Context, req *logi } func (b *backend) pathLoginResolveRole(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + config, err := b.Config(ctx, req.Storage) + if err != nil { + return nil, err + } + if b.configUpdated.Load() { + b.updatedConfig(config) + } + var matched *ParsedCert if verifyResp, resp, err := b.verifyCredentials(ctx, req, data); err != nil { return nil, err } else if resp != nil { - return resp, nil + return certAuthLoginFailureResponse(config, resp, req), nil } else { matched = verifyResp } @@ -118,7 +126,7 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra if verifyResp, resp, err := b.verifyCredentials(ctx, req, data); err != nil { return nil, err } else if resp != nil { - return resp, nil + return certAuthLoginFailureResponse(config, resp, req), nil } else { matched = verifyResp } @@ -181,6 +189,56 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra }, nil } +func certAuthLoginFailureResponse(config *config, resp *logical.Response, req *logical.Request) *logical.Response { + if !config.EnableMetadataOnFailures || !resp.IsError() { + return resp + } + var initialErrMsg string + if err := resp.Error(); err != nil { + initialErrMsg = err.Error() + } + + clientCert, exists := getClientCert(req) + if !exists { + return logical.ErrorResponse("no client certificate found\n" + initialErrMsg) + } + + // Trim these values as they can be anything from any sort of failed certificate + // and we don't want to expose audit entries to randomly large strings. + const maxChars = 100 + metadata := map[string]string{ + "common_name": trimToMaxChars(clientCert.Subject.CommonName, maxChars), + "serial_number": trimToMaxChars(clientCert.SerialNumber.String(), maxChars), + "subject_key_id": trimToMaxChars(certutil.GetHexFormatted(clientCert.SubjectKeyId, ":"), maxChars), + "authority_key_id": trimToMaxChars(certutil.GetHexFormatted(clientCert.AuthorityKeyId, ":"), maxChars), + } + + return logical.ErrorResponseWithData(metadata, initialErrMsg) +} + +func getClientCert(req *logical.Request) (*x509.Certificate, bool) { + if req == nil || req.Connection == nil || req.Connection.ConnState == nil || req.Connection.ConnState.PeerCertificates == nil { + return nil, false + } + clientCerts := req.Connection.ConnState.PeerCertificates + if len(clientCerts) == 0 { + return nil, false + } + clientCert := clientCerts[0] + if clientCert == nil || clientCert.IsCA { + return nil, false + } + return clientCert, true +} + +func trimToMaxChars(formatted string, maxSize int) string { + if len(formatted) > maxSize { + return formatted[:maxSize-3] + "..." + } + + return formatted +} + func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { config, err := b.Config(ctx, req.Storage) if err != nil { @@ -195,7 +253,7 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f if verifyResp, resp, err := b.verifyCredentials(ctx, req, d); err != nil { return nil, err } else if resp != nil { - return resp, nil + return certAuthLoginFailureResponse(config, resp, req), nil } else { matched = verifyResp } diff --git a/builtin/credential/cert/path_login_test.go b/builtin/credential/cert/path_login_test.go index ad1030464f35..9db8e7e6e19d 100644 --- a/builtin/credential/cert/path_login_test.go +++ b/builtin/credential/cert/path_login_test.go @@ -24,6 +24,7 @@ import ( logicaltest "github.com/hashicorp/vault/helper/testhelpers/logical" "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/logical" + "github.com/stretchr/testify/require" "golang.org/x/crypto/ocsp" ) @@ -203,6 +204,51 @@ func testAccStepResolveRoleExpectRoleResolutionToFail(t *testing.T, connState tl t.Fatal("Error not part of response.") } + if _, dataKeyExists := resp.Data["data"]; dataKeyExists { + t.Fatal("metadata key 'data' existed in response without feature enabled") + } + + if !strings.Contains(errString, certAuthFailMsg) { + t.Fatalf("Error was not due to invalid role name. Error: %s", errString) + } + return nil + }, + Data: map[string]interface{}{ + "name": certName, + }, + } +} + +func testAccStepResolveRoleExpectRoleResolutionToFailWithData(t *testing.T, connState tls.ConnectionState, certName string) logicaltest.TestStep { + return logicaltest.TestStep{ + Operation: logical.ResolveRoleOperation, + Path: "login", + Unauthenticated: true, + ConnState: &connState, + ErrorOk: true, + Check: func(resp *logical.Response) error { + if resp == nil && !resp.IsError() { + t.Fatalf("Response was not an error: resp:%#v", resp) + } + + errString, ok := resp.Data["error"].(string) + if !ok { + t.Fatal("Error not part of response.") + } + + dataKeysRaw, dataKeyExists := resp.Data["data"] + if !dataKeyExists { + t.Fatal("metadata key 'data' did not exist in response feature enabled") + } + dataKeys, ok := dataKeysRaw.(map[string]string) + if !ok { + t.Fatalf("the 'data' field was not a map: %T", dataKeysRaw) + } + + for _, key := range []string{"common_name", "serial_number", "authority_key_id", "subject_key_id"} { + require.Contains(t, dataKeys, key, "response metadata key %s was missing in response: %v", key, resp) + } + if !strings.Contains(errString, certAuthFailMsg) { t.Fatalf("Error was not due to invalid role name. Error: %s", errString) } @@ -420,6 +466,44 @@ func TestCert_RoleResolveOCSP(t *testing.T) { } } -func serialFromBigInt(serial *big.Int) string { - return strings.TrimSpace(certutil.GetHexFormatted(serial.Bytes(), ":")) +// TestCert_MetadataOnFailure verifies that we return the cert metadata +// in the response on failures if the configuration option is enabled. +func TestCert_MetadataOnFailure(t *testing.T) { + certTemplate := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "example.com", + }, + DNSNames: []string{"example.com"}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + ExtKeyUsage: []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, + x509.ExtKeyUsageClientAuth, + }, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment | x509.KeyUsageKeyAgreement, + SerialNumber: big.NewInt(mathrand.Int63()), + NotBefore: time.Now().Add(-30 * time.Second), + NotAfter: time.Now().Add(262980 * time.Hour), + } + + tempDir, connState, err := generateTestCertAndConnState(t, certTemplate) + if tempDir != "" { + defer os.RemoveAll(tempDir) + } + if err != nil { + t.Fatalf("error testing connection state: %v", err) + } + ca, err := ioutil.ReadFile(filepath.Join(tempDir, "ca_cert.pem")) + if err != nil { + t.Fatalf("err: %v", err) + } + + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: testFactory(t), + Steps: []logicaltest.TestStep{ + testStepEnableMetadataFailures(), + testAccStepCert(t, "web", ca, "foo", allowed{dns: "example.com"}, false), + testAccStepLoginWithName(t, connState, "web"), + testAccStepResolveRoleExpectRoleResolutionToFailWithData(t, connState, "notweb"), + }, + }) } diff --git a/changelog/29044.txt b/changelog/29044.txt new file mode 100644 index 000000000000..a4e54a2f7967 --- /dev/null +++ b/changelog/29044.txt @@ -0,0 +1,3 @@ +```release-note:improvement +auth/cert: Add new configuration option `enable_metadata_on_failures` to add client cert metadata on login failures to audit log and response +``` diff --git a/sdk/logical/response.go b/sdk/logical/response.go index 721618c76c17..d598c1675c3f 100644 --- a/sdk/logical/response.go +++ b/sdk/logical/response.go @@ -141,6 +141,15 @@ func ErrorResponse(text string, vargs ...interface{}) *Response { } } +// ErrorResponseWithData is used to format an error response with additional data returned +// within the "data" sub-field of the Data field. Useful to return additional information to the client +// and or appear within audited responses. +func ErrorResponseWithData(data interface{}, text string, vargs ...interface{}) *Response { + resp := ErrorResponse(text, vargs...) + resp.Data["data"] = data + return resp +} + // ListResponse is used to format a response to a list operation. func ListResponse(keys []string) *Response { resp := &Response{ diff --git a/sdk/logical/response_test.go b/sdk/logical/response_test.go new file mode 100644 index 000000000000..033c4fbb70f1 --- /dev/null +++ b/sdk/logical/response_test.go @@ -0,0 +1,24 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package logical + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestResponse_ErrorResponse validates that our helper functions produce responses +// that we consider errors. +func TestResponse_ErrorResponse(t *testing.T) { + simpleResp := ErrorResponse("a test %s", "error") + assert.True(t, simpleResp.IsError()) + + dataMap := map[string]string{ + "test1": "testing", + } + + withDataResp := ErrorResponseWithData(dataMap, "a test %s", "error") + assert.True(t, withDataResp.IsError()) +} diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index f179d0b5a629..5c6e091bb24a 100644 --- a/vault/identity_store_entities.go +++ b/vault/identity_store_entities.go @@ -10,7 +10,7 @@ import ( "strings" "github.com/golang/protobuf/ptypes" - memdb "github.com/hashicorp/go-memdb" + "github.com/hashicorp/go-memdb" "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/strutil" "github.com/hashicorp/vault/helper/identity" @@ -286,14 +286,7 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { return logical.ErrorResponse(userErr.Error()), nil } // Alias clash error, so include additional details - resp := &logical.Response{ - Data: map[string]interface{}{ - "error": userErr.Error(), - "data": aliases, - }, - } - - return resp, nil + return logical.ErrorResponseWithData(aliases, userErr.Error()), nil } if intErr != nil { return nil, intErr diff --git a/website/content/api-docs/auth/cert.mdx b/website/content/api-docs/auth/cert.mdx index 25deca159e15..8581ff41b303 100644 --- a/website/content/api-docs/auth/cert.mdx +++ b/website/content/api-docs/auth/cert.mdx @@ -367,6 +367,9 @@ Configuration options for the method. that this cache is used for all configured certificates. - `role_cache_size` `(int: 200)` - The size of the role cache. Use `-1` to disable role caching. +- `enable_metadata_on_failures` `(boolean: false)` - If set, metadata of the client + certificate such as common name, serial, subject key id and authority key id will + be returned on authentication failures and appear in auditing records. ### Sample payload