authorize: do not rely on Envoy client cert validation (#4438)

Partially revert #4374: do not record the peerCertificateValidated() result as reported by Envoy, as this does not work correctly for resumed TLS sessions. Instead always record the certificate chain as presented by the client. Remove the corresponding ClientCertificateInfo Validated field, and update affected code accordingly. Skip the CRL integration test case for now.
pomerium · Aug 3, 2023 · e91600c · e91600c
1 parent 465de43
commit e91600c
Show file tree

Hide file tree

Showing 9 changed files with 12 additions and 69 deletions.
diff --git a/authorize/evaluator/evaluator.go b/authorize/evaluator/evaluator.go
@@ -63,22 +63,14 @@ func NewRequestHTTP(
 // ClientCertificateInfo contains information about the certificate presented
 // by the client (if any).
 type ClientCertificateInfo struct {
-	// Presented is true if the client presented any certificate at all.
+	// Presented is true if the client presented a certificate.
 	Presented bool `json:"presented"`
 
-	// Validated is true if the client presented a valid certificate with a
-	// trust chain rooted at any of the CAs configured within the Envoy
-	// listener. If any routes define a tls_downstream_client_ca, additional
-	// validation is required (for all routes).
-	Validated bool `json:"validated"`
-
-	// Leaf contains the leaf client certificate, provided that the certificate
-	// validated successfully.
+	// Leaf contains the leaf client certificate (unvalidated).
 	Leaf string `json:"leaf,omitempty"`
 
 	// Intermediates contains the remainder of the client certificate chain as
-	// it was originally presented by the client, provided that the client
-	// certificate validated successfully.
+	// it was originally presented by the client (unvalidated).
 	Intermediates string `json:"intermediates,omitempty"`
 }
 

diff --git a/authorize/evaluator/evaluator_test.go b/authorize/evaluator/evaluator_test.go
@@ -123,7 +123,6 @@ func TestEvaluator(t *testing.T) {
 
 	validCertInfo := ClientCertificateInfo{
 		Presented: true,
-		Validated: true,
 		Leaf:      testValidCert,
 	}
 
@@ -167,23 +166,12 @@ func TestEvaluator(t *testing.T) {
 			require.NoError(t, err)
 			assert.Equal(t, NewRuleResult(true, criteria.ReasonClientCertificateRequired), res.Deny)
 		})
-		t.Run("invalid (Envoy)", func(t *testing.T) {
-			res, err := eval(t, options, nil, &Request{
-				Policy: &policies[10],
-				HTTP: RequestHTTP{
-					ClientCertificate: ClientCertificateInfo{Presented: true},
-				},
-			})
-			require.NoError(t, err)
-			assert.Equal(t, NewRuleResult(true, criteria.ReasonInvalidClientCertificate), res.Deny)
-		})
-		t.Run("invalid (authorize)", func(t *testing.T) {
+		t.Run("invalid", func(t *testing.T) {
 			res, err := eval(t, options, nil, &Request{
 				Policy: &policies[10],
 				HTTP: RequestHTTP{
 					ClientCertificate: ClientCertificateInfo{
 						Presented: true,
-						Validated: true,
 						Leaf:      testUnsignedCert,
 					},
 				},

diff --git a/authorize/evaluator/functions.go b/authorize/evaluator/functions.go
@@ -21,7 +21,7 @@ func isValidClientCertificate(ca string, certInfo ClientCertificateInfo) (bool,
 
 	cert := certInfo.Leaf
 
-	if !certInfo.Validated || cert == "" {
+	if cert == "" {
 		return false, nil
 	}
 

diff --git a/authorize/evaluator/functions_test.go b/authorize/evaluator/functions_test.go
@@ -107,25 +107,14 @@ func Test_isValidClientCertificate(t *testing.T) {
 	t.Run("valid cert", func(t *testing.T) {
 		valid, err := isValidClientCertificate(testCA, ClientCertificateInfo{
 			Presented: true,
-			Validated: true,
 			Leaf:      testValidCert,
 		})
 		assert.NoError(t, err, "should not return an error")
 		assert.True(t, valid, "should return true")
 	})
-	t.Run("cert not externally validated", func(t *testing.T) {
-		valid, err := isValidClientCertificate(testCA, ClientCertificateInfo{
-			Presented: true,
-			Validated: false,
-			Leaf:      testValidCert,
-		})
-		assert.NoError(t, err, "should not return an error")
-		assert.False(t, valid, "should return false")
-	})
 	t.Run("unsigned cert", func(t *testing.T) {
 		valid, err := isValidClientCertificate(testCA, ClientCertificateInfo{
 			Presented: true,
-			Validated: true,
 			Leaf:      testUnsignedCert,
 		})
 		assert.NoError(t, err, "should not return an error")
@@ -134,7 +123,6 @@ func Test_isValidClientCertificate(t *testing.T) {
 	t.Run("not a cert", func(t *testing.T) {
 		valid, err := isValidClientCertificate(testCA, ClientCertificateInfo{
 			Presented: true,
-			Validated: true,
 			Leaf:      "WHATEVER!",
 		})
 		assert.Error(t, err, "should return an error")

diff --git a/authorize/grpc.go b/authorize/grpc.go
@@ -186,7 +186,6 @@ func getClientCertificateInfo(
 		return c
 	}
 	c.Presented = metadata.Fields["presented"].GetBoolValue()
-	c.Validated = metadata.Fields["validated"].GetBoolValue()
 	escapedChain := metadata.Fields["chain"].GetStringValue()
 	if escapedChain == "" {
 		// No validated client certificate.

diff --git a/authorize/grpc_test.go b/authorize/grpc_test.go
@@ -80,7 +80,6 @@ func Test_getEvaluatorRequest(t *testing.T) {
 						"com.pomerium.client-certificate-info": {
 							Fields: map[string]*structpb.Value{
 								"presented": structpb.NewBoolValue(true),
-								"validated": structpb.NewBoolValue(true),
 								"chain":     structpb.NewStringValue(url.QueryEscape(certPEM)),
 							},
 						},
@@ -107,7 +106,6 @@ func Test_getEvaluatorRequest(t *testing.T) {
 			},
 			evaluator.ClientCertificateInfo{
 				Presented:     true,
-				Validated:     true,
 				Leaf:          certPEM[1:] + "\n",
 				Intermediates: "",
 			},
@@ -202,49 +200,33 @@ fYCZHo3CID0gRSemaQ/jYMgyeBFrHIr6icZh
 	cases := []struct {
 		label       string
 		presented   bool
-		validated   bool
 		chain       string
 		expected    evaluator.ClientCertificateInfo
 		expectedLog string
 	}{
 		{
 			"not presented",
 			false,
-			false,
 			"",
 			evaluator.ClientCertificateInfo{},
 			"",
 		},
 		{
-			"presented but invalid",
-			true,
-			false,
-			"",
-			evaluator.ClientCertificateInfo{
-				Presented: true,
-			},
-			"",
-		},
-		{
-			"validated",
-			true,
+			"presented",
 			true,
 			url.QueryEscape(leafPEM),
 			evaluator.ClientCertificateInfo{
 				Presented: true,
-				Validated: true,
 				Leaf:      leafPEM,
 			},
 			"",
 		},
 		{
-			"validated with intermediates",
-			true,
+			"presented with intermediates",
 			true,
 			url.QueryEscape(leafPEM + intermediatePEM + rootPEM),
 			evaluator.ClientCertificateInfo{
 				Presented:     true,
-				Validated:     true,
 				Leaf:          leafPEM,
 				Intermediates: intermediatePEM + rootPEM,
 			},
@@ -253,7 +235,6 @@ fYCZHo3CID0gRSemaQ/jYMgyeBFrHIr6icZh
 		{
 			"invalid chain URL encoding",
 			false,
-			false,
 			"invalid%URL%encoding",
 			evaluator.ClientCertificateInfo{},
 			`{"level":"warn","chain":"invalid%URL%encoding","error":"invalid URL escape \"%UR\"","message":"received unexpected client certificate \"chain\" value"}
@@ -262,11 +243,9 @@ fYCZHo3CID0gRSemaQ/jYMgyeBFrHIr6icZh
 		{
 			"invalid chain PEM encoding",
 			true,
-			true,
 			"not valid PEM data",
 			evaluator.ClientCertificateInfo{
 				Presented: true,
-				Validated: true,
 			},
 			`{"level":"warn","chain":"not valid PEM data","message":"received unexpected client certificate \"chain\" value (no PEM block found)"}
 `,
@@ -285,7 +264,6 @@ fYCZHo3CID0gRSemaQ/jYMgyeBFrHIr6icZh
 			metadata := &structpb.Struct{
 				Fields: map[string]*structpb.Value{
 					"presented": structpb.NewBoolValue(c.presented),
-					"validated": structpb.NewBoolValue(c.validated),
 					"chain":     structpb.NewStringValue(c.chain),
 				},
 			}

diff --git a/config/envoyconfig/luascripts/set-client-certificate-metadata.lua b/config/envoyconfig/luascripts/set-client-certificate-metadata.lua
@@ -3,12 +3,8 @@ function envoy_on_request(request_handle)
     local ssl = request_handle:streamInfo():downstreamSslConnection()
     metadata:set("com.pomerium.client-certificate-info", "presented",
                  ssl:peerCertificatePresented())
-    local validated = ssl:peerCertificateValidated()
-    metadata:set("com.pomerium.client-certificate-info", "validated", validated)
-    if validated then
-        metadata:set("com.pomerium.client-certificate-info", "chain",
-                     ssl:urlEncodedPemEncodedPeerCertificateChain())
-    end
+    metadata:set("com.pomerium.client-certificate-info", "chain",
+                 ssl:urlEncodedPemEncodedPeerCertificateChain())
 end
 
 function envoy_on_response(response_handle) end
diff --git a/config/envoyconfig/testdata/main_http_connection_manager_filter.json b/config/envoyconfig/testdata/main_http_connection_manager_filter.json
@@ -38,7 +38,7 @@
         "typedConfig": {
           "@type": "type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua",
           "defaultSourceCode": {
-            "inlineString": "function envoy_on_request(request_handle)\n    local metadata = request_handle:streamInfo():dynamicMetadata()\n    local ssl = request_handle:streamInfo():downstreamSslConnection()\n    metadata:set(\"com.pomerium.client-certificate-info\", \"presented\",\n                 ssl:peerCertificatePresented())\n    local validated = ssl:peerCertificateValidated()\n    metadata:set(\"com.pomerium.client-certificate-info\", \"validated\", validated)\n    if validated then\n        metadata:set(\"com.pomerium.client-certificate-info\", \"chain\",\n                     ssl:urlEncodedPemEncodedPeerCertificateChain())\n    end\nend\n\nfunction envoy_on_response(response_handle) end\n"
+            "inlineString": "function envoy_on_request(request_handle)\n    local metadata = request_handle:streamInfo():dynamicMetadata()\n    local ssl = request_handle:streamInfo():downstreamSslConnection()\n    metadata:set(\"com.pomerium.client-certificate-info\", \"presented\",\n                 ssl:peerCertificatePresented())\n    metadata:set(\"com.pomerium.client-certificate-info\", \"chain\",\n                 ssl:urlEncodedPemEncodedPeerCertificateChain())\nend\n\nfunction envoy_on_response(response_handle) end\n"
           }
         }
       },

diff --git a/integration/policy_test.go b/integration/policy_test.go
@@ -393,6 +393,8 @@ func TestDownstreamClientCA(t *testing.T) {
 		assert.Equal(t, "/", result.Path)
 	})
 	t.Run("revoked client cert", func(t *testing.T) {
+		t.Skip("CRL support must be reimplemented first")
+
 		// Configure an http.Client with a revoked client certificate.
 		cert := loadCertificate(t, "downstream-1-client-revoked")
 		client, transport := getClientWithTransport(t)