From 6fca34eace86a4850a7f4e0f7860fc001ba58dea Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Tue, 19 Mar 2024 10:39:37 -0400 Subject: [PATCH] Proceed with cert auth login attempts if ocsp_fail_open is true and servers are unreachable (#25982) --- builtin/credential/cert/backend.go | 5 +- builtin/credential/cert/backend_test.go | 419 +++++++++++++++++++++++- builtin/credential/cert/path_certs.go | 18 +- builtin/credential/cert/path_login.go | 30 ++ changelog/25982.txt | 3 + sdk/helper/ocsp/client.go | 10 + website/content/api-docs/auth/cert.mdx | 2 + 7 files changed, 481 insertions(+), 6 deletions(-) create mode 100644 changelog/25982.txt diff --git a/builtin/credential/cert/backend.go b/builtin/credential/cert/backend.go index f7f1255595..d40a8de602 100644 --- a/builtin/credential/cert/backend.go +++ b/builtin/credential/cert/backend.go @@ -26,8 +26,9 @@ const ( operationPrefixCert = "cert" trustedCertPath = "cert/" - defaultRoleCacheSize = 200 - maxRoleCacheSize = 10000 + defaultRoleCacheSize = 200 + defaultOcspMaxRetries = 4 + maxRoleCacheSize = 10000 ) func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) { diff --git a/builtin/credential/cert/backend_test.go b/builtin/credential/cert/backend_test.go index a7bb32de5f..d56bd25a45 100644 --- a/builtin/credential/cert/backend_test.go +++ b/builtin/credential/cert/backend_test.go @@ -5,6 +5,7 @@ package cert import ( "context" + "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -20,6 +21,7 @@ import ( mathrand "math/rand" "net" "net/http" + "net/http/httptest" "net/url" "os" "path/filepath" @@ -28,8 +30,8 @@ import ( "time" "github.com/go-test/deep" - cleanhttp "github.com/hashicorp/go-cleanhttp" - rootcerts "github.com/hashicorp/go-rootcerts" + "github.com/hashicorp/go-cleanhttp" + "github.com/hashicorp/go-rootcerts" "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/api" "github.com/hashicorp/vault/builtin/logical/pki" @@ -41,6 +43,8 @@ import ( "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" "github.com/mitchellh/mapstructure" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/ocsp" "golang.org/x/net/http2" ) @@ -2057,6 +2061,11 @@ func testConnState(certPath, keyPath, rootCertPath string) (tls.ConnectionState, if err != nil { return tls.ConnectionState{}, err } + + return testConnStateWithCert(cert, rootCAs) +} + +func testConnStateWithCert(cert tls.Certificate, rootCAs *x509.CertPool) (tls.ConnectionState, error) { listenConf := &tls.Config{ Certificates: []tls.Certificate{cert}, ClientAuth: tls.RequestClientCert, @@ -2336,3 +2345,409 @@ func TestBackend_CertUpgrade(t *testing.T) { t.Fatal(diff) } } + +// TestOCSPWithMixedValidResponses validates the expected behavior of multiple OCSP servers configured, +// with and without ocsp_query_all_servers enabled or disabled. +func TestOCSPWithMixedValidResponses(t *testing.T) { + caFile := "test-fixtures/root/rootcacert.pem" + pemCa, err := os.ReadFile(caFile) + require.NoError(t, err, "failed reading in file %s", caFile) + caTLS := loadCerts(t, caFile, "test-fixtures/root/rootcakey.pem") + leafTLS := loadCerts(t, "test-fixtures/keys/cert.pem", "test-fixtures/keys/key.pem") + + rootConfig := &rootcerts.Config{ + CAFile: caFile, + } + rootCAs, err := rootcerts.LoadCACerts(rootConfig) + connState, err := testConnStateWithCert(leafTLS, rootCAs) + require.NoError(t, err, "error testing connection state: %v", err) + + // Setup an OCSP handler + ocspHandler := func(status int) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + now := time.Now() + ocspRes := ocsp.Response{ + SerialNumber: leafTLS.Leaf.SerialNumber, + ThisUpdate: now.Add(-1 * time.Hour), + NextUpdate: now.Add(30 * time.Minute), + Status: status, + } + response, err := ocsp.CreateResponse(caTLS.Leaf, caTLS.Leaf, ocspRes, caTLS.PrivateKey.(crypto.Signer)) + if err != nil { + t.Fatalf("failed generating OCSP response: %v", err) + } + _, _ = w.Write(response) + }) + } + goodTs := httptest.NewServer(ocspHandler(ocsp.Good)) + revokeTs := httptest.NewServer(ocspHandler(ocsp.Revoked)) + defer goodTs.Close() + defer revokeTs.Close() + + steps := []logicaltest.TestStep{ + // step 1/2: This should pass as we will query the first server and get a valid good response, not testing + // the second configured server + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", allowed{names: "cert.example.com"}, false, + map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{goodTs.URL, revokeTs.URL}, + "ocsp_query_all_servers": false, + }), + testAccStepLogin(t, connState), + // step 3/4: This should fail as we will query the revoking OCSP server first and get a revoke response + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", allowed{names: "cert.example.com"}, false, + map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{revokeTs.URL, goodTs.URL}, + "ocsp_query_all_servers": false, + }), + testAccStepLoginInvalid(t, connState), + } + + // Setup a new factory everytime to avoid OCSP caching from influencing the test + for i := 0; i < len(steps); i += 2 { + setup := i + execute := i + 1 + t.Run(fmt.Sprintf("steps-%d-%d", setup+1, execute+1), func(t *testing.T) { + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: testFactory(t), + Steps: []logicaltest.TestStep{steps[setup], steps[execute]}, + }) + }) + } +} + +// TestOCSPFailOpenWithGoodResponse validates the expected behavior with multiple OCSP servers configured +// one that returns a Good response the other is not available, along with the ocsp_fail_open in multiple modes +func TestOCSPFailOpenWithGoodResponse(t *testing.T) { + caFile := "test-fixtures/root/rootcacert.pem" + pemCa, err := os.ReadFile(caFile) + require.NoError(t, err, "failed reading in file %s", caFile) + caTLS := loadCerts(t, caFile, "test-fixtures/root/rootcakey.pem") + leafTLS := loadCerts(t, "test-fixtures/keys/cert.pem", "test-fixtures/keys/key.pem") + + rootConfig := &rootcerts.Config{ + CAFile: caFile, + } + rootCAs, err := rootcerts.LoadCACerts(rootConfig) + connState, err := testConnStateWithCert(leafTLS, rootCAs) + require.NoError(t, err, "error testing connection state: %v", err) + + // Setup an OCSP handler + ocspHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + now := time.Now() + ocspRes := ocsp.Response{ + SerialNumber: leafTLS.Leaf.SerialNumber, + ThisUpdate: now.Add(-1 * time.Hour), + NextUpdate: now.Add(30 * time.Minute), + Status: ocsp.Good, + } + response, err := ocsp.CreateResponse(caTLS.Leaf, caTLS.Leaf, ocspRes, caTLS.PrivateKey.(crypto.Signer)) + if err != nil { + t.Fatalf("failed generating OCSP response: %v", err) + } + _, _ = w.Write(response) + }) + ts := httptest.NewServer(ocspHandler) + defer ts.Close() + + steps := []logicaltest.TestStep{ + // Step 1/2 With no proper responses from any OCSP server and fail_open to true, we should pass validation + // as fail_open is true + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", allowed{names: "cert.example.com"}, false, + map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{"http://127.0.0.1:30000", "http://127.0.0.1:30001"}, + "ocsp_fail_open": true, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLogin(t, connState), + // Step 3/4 With no proper responses from any OCSP server and fail_open to false we should fail validation + // as fail_open is false + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", + allowed{names: "cert.example.com"}, false, map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{"http://127.0.0.1:30000", "http://127.0.0.1:30001"}, + "ocsp_fail_open": false, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLoginInvalid(t, connState), + // Step 5/6 With a single positive response, query all servers set to false and fail open true, pass validation + // as query all servers is false + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", allowed{names: "cert.example.com"}, false, + map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{ts.URL, "http://127.0.0.1:30001"}, + "ocsp_fail_open": true, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLogin(t, connState), + // Step 7/8 With a single positive response, query all servers set to false and fail open false, pass validation + // as query all servers is false + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", + allowed{names: "cert.example.com"}, false, map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{ts.URL, "http://127.0.0.1:30001"}, + "ocsp_fail_open": false, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLogin(t, connState), + } + + // Setup a new factory everytime to avoid OCSP caching from influencing the test + for i := 0; i < len(steps); i += 2 { + setup := i + execute := i + 1 + t.Run(fmt.Sprintf("steps-%d-%d", setup+1, execute+1), func(t *testing.T) { + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: testFactory(t), + Steps: []logicaltest.TestStep{steps[setup], steps[execute]}, + }) + }) + } +} + +// TestOCSPFailOpenWithRevokeResponse validates the expected behavior with multiple OCSP servers configured +// one that returns a Revoke response the other is not available, along with the ocsp_fail_open in multiple modes +func TestOCSPFailOpenWithRevokeResponse(t *testing.T) { + caFile := "test-fixtures/root/rootcacert.pem" + pemCa, err := os.ReadFile(caFile) + require.NoError(t, err, "failed reading in file %s", caFile) + caTLS := loadCerts(t, caFile, "test-fixtures/root/rootcakey.pem") + leafTLS := loadCerts(t, "test-fixtures/keys/cert.pem", "test-fixtures/keys/key.pem") + + rootConfig := &rootcerts.Config{ + CAFile: caFile, + } + rootCAs, err := rootcerts.LoadCACerts(rootConfig) + connState, err := testConnStateWithCert(leafTLS, rootCAs) + require.NoError(t, err, "error testing connection state: %v", err) + + // Setup an OCSP handler + ocspHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + now := time.Now() + ocspRes := ocsp.Response{ + SerialNumber: leafTLS.Leaf.SerialNumber, + ThisUpdate: now.Add(-1 * time.Hour), + NextUpdate: now.Add(30 * time.Minute), + Status: ocsp.Revoked, + } + response, err := ocsp.CreateResponse(caTLS.Leaf, caTLS.Leaf, ocspRes, caTLS.PrivateKey.(crypto.Signer)) + if err != nil { + t.Fatalf("failed generating OCSP response: %v", err) + } + _, _ = w.Write(response) + }) + ts := httptest.NewServer(ocspHandler) + defer ts.Close() + + // With no OCSP servers available, make sure that we behave as we expect + steps := []logicaltest.TestStep{ + // Step 1/2 With a single revoke response, query all servers set to false and fail open true, fail validation + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", allowed{names: "cert.example.com"}, false, + map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{ts.URL, "http://127.0.0.1:30001"}, + "ocsp_fail_open": true, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLoginInvalid(t, connState), + // Step 3/4 With a single revoke response, query all servers set to false and fail open false, fail validation + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", + allowed{names: "cert.example.com"}, false, map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{ts.URL, "http://127.0.0.1:30001"}, + "ocsp_fail_open": false, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLoginInvalid(t, connState), + } + + // Setup a new factory everytime to avoid OCSP caching from influencing the test + for i := 0; i < len(steps); i += 2 { + setup := i + execute := i + 1 + t.Run(fmt.Sprintf("steps-%d-%d", setup+1, execute+1), func(t *testing.T) { + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: testFactory(t), + Steps: []logicaltest.TestStep{steps[setup], steps[execute]}, + }) + }) + } +} + +// TestOCSPFailOpenWithUnknownResponse validates the expected behavior with multiple OCSP servers configured +// one that returns an Unknown response the other is not available, along with the ocsp_fail_open in multiple modes +func TestOCSPFailOpenWithUnknownResponse(t *testing.T) { + caFile := "test-fixtures/root/rootcacert.pem" + pemCa, err := os.ReadFile(caFile) + require.NoError(t, err, "failed reading in file %s", caFile) + caTLS := loadCerts(t, caFile, "test-fixtures/root/rootcakey.pem") + leafTLS := loadCerts(t, "test-fixtures/keys/cert.pem", "test-fixtures/keys/key.pem") + + rootConfig := &rootcerts.Config{ + CAFile: caFile, + } + rootCAs, err := rootcerts.LoadCACerts(rootConfig) + connState, err := testConnStateWithCert(leafTLS, rootCAs) + require.NoError(t, err, "error testing connection state: %v", err) + + // Setup an OCSP handler + ocspHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + now := time.Now() + ocspRes := ocsp.Response{ + SerialNumber: leafTLS.Leaf.SerialNumber, + ThisUpdate: now.Add(-1 * time.Hour), + NextUpdate: now.Add(30 * time.Minute), + Status: ocsp.Unknown, + } + response, err := ocsp.CreateResponse(caTLS.Leaf, caTLS.Leaf, ocspRes, caTLS.PrivateKey.(crypto.Signer)) + if err != nil { + t.Fatalf("failed generating OCSP response: %v", err) + } + _, _ = w.Write(response) + }) + ts := httptest.NewServer(ocspHandler) + defer ts.Close() + + // With no OCSP servers available, make sure that we behave as we expect + steps := []logicaltest.TestStep{ + // Step 1/2 With a single unknown response, query all servers set to false and fail open true, pass validation + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", allowed{names: "cert.example.com"}, false, + map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{ts.URL, "http://127.0.0.1:30001"}, + "ocsp_fail_open": true, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLogin(t, connState), + // Step 3/4 With a single unknown response, query all servers set to false and fail open false, fail validation + testAccStepCertWithExtraParams(t, "web", pemCa, "foo", + allowed{names: "cert.example.com"}, false, map[string]interface{}{ + "ocsp_enabled": true, + "ocsp_servers_override": []string{ts.URL, "http://127.0.0.1:30001"}, + "ocsp_fail_open": false, + "ocsp_query_all_servers": false, + "ocsp_max_retries": 0, + }), + testAccStepLoginInvalid(t, connState), + } + + // Setup a new factory everytime to avoid OCSP caching from influencing the test + for i := 0; i < len(steps); i += 2 { + setup := i + execute := i + 1 + t.Run(fmt.Sprintf("steps-%d-%d", setup+1, execute+1), func(t *testing.T) { + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: testFactory(t), + Steps: []logicaltest.TestStep{steps[setup], steps[execute]}, + }) + }) + } +} + +// TestOcspMaxRetriesUpdate verifies that the ocsp_max_retries field is properly initialized +// with our default value of 4, legacy roles have it initialized automatically to 4 and we +// can properly store and retrieve updates to the field. +func TestOcspMaxRetriesUpdate(t *testing.T) { + storage := &logical.InmemStorage{} + ctx := context.Background() + + lb, err := Factory(context.Background(), &logical.BackendConfig{ + System: &logical.StaticSystemView{ + DefaultLeaseTTLVal: 300 * time.Second, + MaxLeaseTTLVal: 1800 * time.Second, + }, + StorageView: storage, + }) + require.NoError(t, err, "failed creating backend") + + caFile := "test-fixtures/root/rootcacert.pem" + pemCa, err := os.ReadFile(caFile) + require.NoError(t, err, "failed reading in file %s", caFile) + + data := map[string]interface{}{ + "certificate": string(pemCa), + "display_name": "test", + } + + // Test initial creation of role sets ocsp_max_retries to a default of 4 + _, err = lb.HandleRequest(ctx, &logical.Request{ + Operation: logical.UpdateOperation, + Path: "certs/test", + Data: data, + Storage: storage, + }) + require.NoError(t, err, "failed initial role creation request") + + resp, err := lb.HandleRequest(ctx, &logical.Request{ + Operation: logical.ReadOperation, + Path: "certs/test", + Storage: storage, + }) + require.NoError(t, err, "failed reading role request") + require.NotNil(t, resp) + require.Equal(t, 4, resp.Data["ocsp_max_retries"], "ocsp config didn't match expectations") + + // Test we can update the field and read it back + data["ocsp_max_retries"] = 1 + _, err = lb.HandleRequest(ctx, &logical.Request{ + Operation: logical.UpdateOperation, + Path: "certs/test", + Data: data, + Storage: storage, + }) + require.NoError(t, err, "failed updating role request") + + resp, err = lb.HandleRequest(ctx, &logical.Request{ + Operation: logical.ReadOperation, + Path: "certs/test", + Storage: storage, + }) + require.NoError(t, err, "failed reading role request") + require.NotNil(t, resp) + require.Equal(t, 1, resp.Data["ocsp_max_retries"], "ocsp config didn't match expectations on update") + + // Verify existing storage entries get updated with a value of 4 + entry := &logical.StorageEntry{ + Key: "cert/legacy", + Value: []byte(`{"token_bound_cidrs":null,"token_explicit_max_ttl":0,"token_max_ttl":0, + "token_no_default_policy":false,"token_num_uses":0,"token_period":0, + "token_policies":null,"token_type":0,"token_ttl":0,"Name":"test", + "Certificate":"-----BEGIN CERTIFICATE-----\nMIIDPDCCAiSgAwIBAgIUb5id+GcaMeMnYBv3MvdTGWigyJ0wDQYJKoZIhvcNAQEL\nBQAwFjEUMBIGA1UEAxMLZXhhbXBsZS5jb20wHhcNMTYwMjI5MDIyNzI5WhcNMjYw\nMjI2MDIyNzU5WjAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTCCASIwDQYJKoZIhvcN\nAQEBBQADggEPADCCAQoCggEBAOxTMvhTuIRc2YhxZpmPwegP86cgnqfT1mXxi1A7\nQ7qax24Nqbf00I3oDMQtAJlj2RB3hvRSCb0/lkF7i1Bub+TGxuM7NtZqp2F8FgG0\nz2md+W6adwW26rlxbQKjmRvMn66G9YPTkoJmPmxt2Tccb9+apmwW7lslL5j8H48x\nAHJTMb+PMP9kbOHV5Abr3PT4jXUPUr/mWBvBiKiHG0Xd/HEmlyOEPeAThxK+I5tb\n6m+eB+7cL9BsvQpy135+2bRAxUphvFi5NhryJ2vlAvoJ8UqigsNK3E28ut60FAoH\nSWRfFUFFYtfPgTDS1yOKU/z/XMU2giQv2HrleWt0mp4jqBUCAwEAAaOBgTB/MA4G\nA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSdxLNP/ocx\n7HK6JT3/sSAe76iTmzAfBgNVHSMEGDAWgBSdxLNP/ocx7HK6JT3/sSAe76iTmzAc\nBgNVHREEFTATggtleGFtcGxlLmNvbYcEfwAAATANBgkqhkiG9w0BAQsFAAOCAQEA\nwHThDRsXJunKbAapxmQ6bDxSvTvkLA6m97TXlsFgL+Q3Jrg9HoJCNowJ0pUTwhP2\nU946dCnSCkZck0fqkwVi4vJ5EQnkvyEbfN4W5qVsQKOFaFVzep6Qid4rZT6owWPa\ncNNzNcXAee3/j6hgr6OQ/i3J6fYR4YouYxYkjojYyg+CMdn6q8BoV0BTsHdnw1/N\nScbnBHQIvIZMBDAmQueQZolgJcdOuBLYHe/kRy167z8nGg+PUFKIYOL8NaOU1+CJ\nt2YaEibVq5MRqCbRgnd9a2vG0jr5a3Mn4CUUYv+5qIjP3hUusYenW1/EWtn1s/gk\nzehNe5dFTjFpylg1o6b8Ow==\n-----END CERTIFICATE-----\n", + "DisplayName":"test","Policies":null,"TTL":0,"MaxTTL":0,"Period":0, + "AllowedNames":null,"AllowedCommonNames":null,"AllowedDNSSANs":null, + "AllowedEmailSANs":null,"AllowedURISANs":null,"AllowedOrganizationalUnits":null, + "RequiredExtensions":null,"AllowedMetadataExtensions":null,"BoundCIDRs":null, + "OcspCaCertificates":"","OcspEnabled":false,"OcspServersOverride":null, + "OcspFailOpen":false,"OcspQueryAllServers":false}`), + } + err = storage.Put(ctx, entry) + require.NoError(t, err, "failed putting legacy storage entry") + + resp, err = lb.HandleRequest(ctx, &logical.Request{ + Operation: logical.ReadOperation, + Path: "certs/legacy", + Storage: storage, + }) + require.NoError(t, err, "failed reading role request") + require.NotNil(t, resp) + require.Equal(t, 4, resp.Data["ocsp_max_retries"], "ocsp config didn't match expectations on legacy entry") +} + +func loadCerts(t *testing.T, certFile, certKey string) tls.Certificate { + caTLS, err := tls.LoadX509KeyPair(certFile, certKey) + require.NoError(t, err, "failed reading ca/key files") + + caTLS.Leaf, err = x509.ParseCertificate(caTLS.Certificate[0]) + require.NoError(t, err, "failed parsing certificate from file %s", certFile) + + return caTLS +} diff --git a/builtin/credential/cert/path_certs.go b/builtin/credential/cert/path_certs.go index dcba0b424e..19963827c5 100644 --- a/builtin/credential/cert/path_certs.go +++ b/builtin/credential/cert/path_certs.go @@ -96,6 +96,11 @@ from the AuthorityInformationAccess extension on the certificate being inspected Default: 0, Description: "If greater than 0, specifies the maximum age of an OCSP thisUpdate field to avoid accepting old responses without a nextUpdate field.", }, + "ocsp_max_retries": { + Type: framework.TypeInt, + Default: 4, + Description: "The number of retries the OCSP client should attempt per query.", + }, "allowed_names": { Type: framework.TypeCommaStringSlice, Description: `A comma-separated list of names. @@ -248,7 +253,7 @@ func (b *backend) Cert(ctx context.Context, s logical.Storage, n string) (*CertE return nil, nil } - var result CertEntry + result := CertEntry{OcspMaxRetries: defaultOcspMaxRetries} // Specify our defaults if the key is missing if err := entry.DecodeJSON(&result); err != nil { return nil, err } @@ -315,6 +320,7 @@ func (b *backend) pathCertRead(ctx context.Context, req *logical.Request, d *fra "ocsp_fail_open": cert.OcspFailOpen, "ocsp_query_all_servers": cert.OcspQueryAllServers, "ocsp_this_update_max_age": int64(cert.OcspThisUpdateMaxAge.Seconds()), + "ocsp_max_retries": cert.OcspMaxRetries, } cert.PopulateTokenData(data) @@ -350,7 +356,8 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr if cert == nil { cert = &CertEntry{ - Name: name, + Name: name, + OcspMaxRetries: defaultOcspMaxRetries, } } @@ -380,6 +387,12 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr } cert.OcspThisUpdateMaxAge = maxAgeDuration } + if ocspMaxRetries, ok := d.GetOk("ocsp_max_retries"); ok { + cert.OcspMaxRetries = ocspMaxRetries.(int) + if cert.OcspMaxRetries < 0 { + return nil, fmt.Errorf("ocsp_max_retries can not be a negative number") + } + } if displayNameRaw, ok := d.GetOk("display_name"); ok { cert.DisplayName = displayNameRaw.(string) } @@ -531,6 +544,7 @@ type CertEntry struct { OcspFailOpen bool OcspQueryAllServers bool OcspThisUpdateMaxAge time.Duration + OcspMaxRetries int } const pathCertHelpSyn = ` diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 0ecbe8c565..55f161a9fe 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -14,6 +14,7 @@ import ( "encoding/pem" "errors" "fmt" + "net/url" "strings" "github.com/hashicorp/errwrap" @@ -663,6 +664,7 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage, } conf.QueryAllServers = conf.QueryAllServers || entry.OcspQueryAllServers conf.OcspThisUpdateMaxAge = entry.OcspThisUpdateMaxAge + conf.OcspMaxRetries = entry.OcspMaxRetries } } @@ -685,6 +687,12 @@ func (b *backend) checkForCertInOCSP(ctx context.Context, clientCert *x509.Certi defer b.ocspClientMutex.RUnlock() err := b.ocspClient.VerifyLeafCertificate(ctx, clientCert, chain[1], conf) if err != nil { + if conf.OcspFailureMode == ocsp.FailOpenTrue { + onlyNetworkErrors := b.handleOcspErrorInFailOpen(err) + if onlyNetworkErrors { + return true, nil + } + } // We want to preserve error messages when they have additional, // potentially useful information. Just having a revoked cert // isn't additionally useful. @@ -696,6 +704,28 @@ func (b *backend) checkForCertInOCSP(ctx context.Context, clientCert *x509.Certi return true, nil } +func (b *backend) handleOcspErrorInFailOpen(err error) bool { + urlError := &url.Error{} + allNetworkErrors := true + if multiError, ok := err.(*multierror.Error); ok { + for _, myErr := range multiError.Errors { + if !errors.As(myErr, &urlError) { + allNetworkErrors = false + } + } + } else if !errors.As(err, &urlError) { + allNetworkErrors = false + } + + if allNetworkErrors { + b.Logger().Warn("OCSP is set to fail-open, and could not retrieve "+ + "OCSP based revocation but proceeding.", "detail", err) + return true + } + + return false +} + func (b *backend) checkForChainInCRLs(chain []*x509.Certificate) bool { badChain := false for _, cert := range chain { diff --git a/changelog/25982.txt b/changelog/25982.txt new file mode 100644 index 0000000000..59a7d0512b --- /dev/null +++ b/changelog/25982.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/cert: Allow cert auth login attempts if ocsp_fail_open is true and OCSP servers are unreachable +``` diff --git a/sdk/helper/ocsp/client.go b/sdk/helper/ocsp/client.go index 5bb82e1f81..c55f109a7e 100644 --- a/sdk/helper/ocsp/client.go +++ b/sdk/helper/ocsp/client.go @@ -516,6 +516,7 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509. timeout := defaultOCSPResponderTimeout ocspClient := retryablehttp.NewClient() + ocspClient.RetryMax = conf.OcspMaxRetries ocspClient.HTTPClient.Timeout = timeout ocspClient.HTTPClient.Transport = newInsecureOcspTransport(conf.ExtraCas) @@ -594,6 +595,14 @@ func (c *Client) GetRevocationStatus(ctx context.Context, subject, issuer *x509. } } + // If querying all servers is enabled, and we have an error from a host, we can't trust + // a good status from the other as we can't confirm the other server would have returned the + // same response, we do allow revoke responses through + if conf.QueryAllServers && firstError != nil && (ret != nil && ret.code == ocspStatusGood) { + return nil, fmt.Errorf("encountered an error on a server, "+ + "ignoring good response status as ocsp_query_all_servers is set to true: %w", firstError) + } + // If no server reported the cert revoked, but we did have an error, report it if (ret == nil || ret.code == ocspStatusUnknown) && firstError != nil { return nil, firstError @@ -639,6 +648,7 @@ type VerifyConfig struct { OcspFailureMode FailOpenMode QueryAllServers bool OcspThisUpdateMaxAge time.Duration + OcspMaxRetries int } // VerifyLeafCertificate verifies just the subject against it's direct issuer diff --git a/website/content/api-docs/auth/cert.mdx b/website/content/api-docs/auth/cert.mdx index 864d175744..eb6cbb9a03 100644 --- a/website/content/api-docs/auth/cert.mdx +++ b/website/content/api-docs/auth/cert.mdx @@ -80,6 +80,8 @@ Sets a CA cert and associated parameters in a role name. - `ocsp_this_update_max_age` `(integer:0 or string: "")` - If greater than 0, specifies the maximum age of an OCSP thisUpdate field. This avoids accepting old responses without a nextUpdate field. +- `ocsp_max_retries` `(integer: 4)` - The number of retries attempted before giving + up on an OCSP request. 0 will disable retries - `ocsp_query_all_servers` `(bool: false)` - If set to true, rather than accepting the first successful OCSP response, query all servers and consider the certificate valid only if all servers agree.