diff --git a/authorize/check_response.go b/authorize/check_response.go index 617468387..b5bd366a5 100644 --- a/authorize/check_response.go +++ b/authorize/check_response.go @@ -32,6 +32,12 @@ func (a *Authorize) handleResult( request *evaluator.Request, result *evaluator.Result, ) (*envoy_service_auth_v3.CheckResponse, error) { + // If a client certificate is required, but the client did not provide a + // valid certificate, deny right away. Do not redirect to authenticate. + if result.Deny.Reasons.Has(criteria.ReasonInvalidClientCertificate) { + return a.handleResultDenied(ctx, in, request, result, result.Deny.Reasons) + } + // when the user is unauthenticated it means they haven't // logged in yet, so redirect to authenticate if result.Allow.Reasons.Has(criteria.ReasonUserUnauthenticated) || diff --git a/authorize/check_response_test.go b/authorize/check_response_test.go index aa3134d97..b7e837efe 100644 --- a/authorize/check_response_test.go +++ b/authorize/check_response_test.go @@ -88,6 +88,19 @@ func TestAuthorize_handleResult(t *testing.T) { assert.NotNil(t, res.GetOkResponse()) }) }) + t.Run("invalid-client-certificate", func(t *testing.T) { + // Even if the user is unauthenticated, if a client certificate was required and no valid + // certificate was provided, access should be denied (no login redirect). + res, err := a.handleResult(context.Background(), + &envoy_service_auth_v3.CheckRequest{}, + &evaluator.Request{}, + &evaluator.Result{ + Allow: evaluator.NewRuleResult(false, criteria.ReasonUserUnauthenticated), + Deny: evaluator.NewRuleResult(true, criteria.ReasonInvalidClientCertificate), + }) + assert.NoError(t, err) + assert.Equal(t, 495, int(res.GetDeniedResponse().GetStatus().GetCode())) + }) } func TestAuthorize_okResponse(t *testing.T) { diff --git a/integration/policy_test.go b/integration/policy_test.go index f3259c8d4..be97b5ef5 100644 --- a/integration/policy_test.go +++ b/integration/policy_test.go @@ -347,9 +347,11 @@ func TestDownstreamClientCA(t *testing.T) { defer clearTimeout() t.Run("no client cert", func(t *testing.T) { - res, err := flows.Authenticate(ctx, getClient(t), - mustParseURL("https://client-cert-required.localhost.pomerium.io/"), - flows.WithEmail("user1@dogs.test")) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + "https://client-cert-required.localhost.pomerium.io/", nil) + require.NoError(t, err) + + res, err := getClient(t).Do(req) require.NoError(t, err) res.Body.Close() assert.Equal(t, httputil.StatusInvalidClientCertificate, res.StatusCode) @@ -360,9 +362,11 @@ func TestDownstreamClientCA(t *testing.T) { client, transport := getClientWithTransport(t) transport.TLSClientConfig.Certificates = []tls.Certificate{cert} - res, err := flows.Authenticate(ctx, client, - mustParseURL("https://client-cert-required.localhost.pomerium.io/"), - flows.WithEmail("user1@dogs.test")) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + "https://client-cert-required.localhost.pomerium.io/", nil) + require.NoError(t, err) + + res, err := client.Do(req) require.NoError(t, err) res.Body.Close() assert.Equal(t, httputil.StatusInvalidClientCertificate, res.StatusCode) @@ -404,7 +408,7 @@ func TestMultipleDownstreamClientCAs(t *testing.T) { // Asserts that we get a successful JSON response from the httpdetails // service, matching the given path. - assertOK := func(res *http.Response, err error, path string) { + assertOK := func(t *testing.T, res *http.Response, err error, path string) { require.NoError(t, err, "unexpected http error") defer res.Body.Close() @@ -419,17 +423,17 @@ func TestMultipleDownstreamClientCAs(t *testing.T) { t.Run("cert1", func(t *testing.T) { client := newClientWithCert("downstream-1-client") - // With cert1, we should get a valid response for the /ca1 path. + // With cert1, we should get a valid response for the /ca1 path + // (after login). res, err := flows.Authenticate(ctx, client, mustParseURL("https://client-cert-overlap.localhost.pomerium.io/ca1"), flows.WithEmail("user1@dogs.test")) - assertOK(res, err, "/ca1") + assertOK(t, res, err, "/ca1") // With cert1, we should get an HTML error page for the /ca2 path. req, err := http.NewRequestWithContext(ctx, http.MethodGet, "https://client-cert-overlap.localhost.pomerium.io/ca2", nil) require.NoError(t, err) - res, err = client.Do(req) require.NoError(t, err, "unexpected http error") res.Body.Close() @@ -439,34 +443,36 @@ func TestMultipleDownstreamClientCAs(t *testing.T) { client := newClientWithCert("downstream-2-client") // With cert2, we should get an HTML error page for the /ca1 path - // (after login). - res, err := flows.Authenticate(ctx, client, - mustParseURL("https://client-cert-overlap.localhost.pomerium.io/ca1"), - flows.WithEmail("user1@dogs.test")) + // (before login). + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + "https://client-cert-overlap.localhost.pomerium.io/ca1", nil) require.NoError(t, err) + res, err := client.Do(req) + require.NoError(t, err, "unexpected http error") res.Body.Close() assert.Equal(t, httputil.StatusInvalidClientCertificate, res.StatusCode) - // With cert2, we should get a valid response for the /ca2 path. - req, err := http.NewRequestWithContext(ctx, http.MethodGet, - "https://client-cert-overlap.localhost.pomerium.io/ca2", nil) - require.NoError(t, err, "unexpected http error") - res, err = client.Do(req) - assertOK(res, err, "/ca2") + // With cert2, we should get a valid response for the /ca2 path + // (after login). + res, err = flows.Authenticate(ctx, client, + mustParseURL("https://client-cert-overlap.localhost.pomerium.io/ca2"), + flows.WithEmail("user1@dogs.test")) + assertOK(t, res, err, "/ca2") }) t.Run("no cert", func(t *testing.T) { client := getClient(t) // Without a client certificate, both paths should return an HTML error - // page (after login). - res, err := flows.Authenticate(ctx, client, - mustParseURL("https://client-cert-overlap.localhost.pomerium.io/ca1"), - flows.WithEmail("user1@dogs.test")) + // page (no login redirect). + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + "https://client-cert-overlap.localhost.pomerium.io/ca1", nil) require.NoError(t, err) + res, err := client.Do(req) + require.NoError(t, err, "unexpected http error") res.Body.Close() assert.Equal(t, httputil.StatusInvalidClientCertificate, res.StatusCode) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, + req, err = http.NewRequestWithContext(ctx, http.MethodGet, "https://client-cert-overlap.localhost.pomerium.io/ca2", nil) require.NoError(t, err) res, err = client.Do(req)