authorize: do not redirect if invalid client cert (#4344)

If an authorization policy requires a client certificate, but an
incoming request does not include a valid certificate, we should serve a
deny error page right away, regardless of whether the user is
authenticated via the identity provider or not. Do not redirect to the
identity provider login page in this case.

Update the existing integration tests accordingly, and add a unit test
case for this scenario.

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) ||

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) {

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)