Only request TLS client certificates if needed

For gRPC we can always request that client certificates are sent, even if we don't do actual certificate validation. For HTTP this is a bit more problematic, because browsers tend to show popups/prompts whenever the server asks for a certificate. This change adds an argument to NewTLSConfigFromServerConfiguration() to indicate that certificates should be sent over. We only enable this if the server has a TLS client certificate validating policy in place.
buildbarn · Jun 20, 2024 · d8269bd · d8269bd
1 parent 4d8e1ea
commit d8269bd
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 34 deletions.
diff --git a/pkg/grpc/authenticator.go b/pkg/grpc/authenticator.go
@@ -25,75 +25,79 @@ type Authenticator interface {
 
 // NewAuthenticatorFromConfiguration creates a tree of Authenticator
 // objects based on a configuration file.
-func NewAuthenticatorFromConfiguration(policy *configuration.AuthenticationPolicy, group program.Group) (Authenticator, bool, error) {
+func NewAuthenticatorFromConfiguration(policy *configuration.AuthenticationPolicy, group program.Group) (Authenticator, bool, bool, error) {
 	if policy == nil {
-		return nil, false, status.Error(codes.InvalidArgument, "Authentication policy not specified")
+		return nil, false, false, status.Error(codes.InvalidArgument, "Authentication policy not specified")
 	}
 	switch policyKind := policy.Policy.(type) {
 	case *configuration.AuthenticationPolicy_Allow:
 		authenticationMetadata, err := auth.NewAuthenticationMetadataFromProto(policyKind.Allow)
 		if err != nil {
-			return nil, false, status.Error(codes.InvalidArgument, "Failed to create authentication metadata")
+			return nil, false, false, status.Error(codes.InvalidArgument, "Failed to create authentication metadata")
 		}
-		return NewAllowAuthenticator(authenticationMetadata), false, nil
+		return NewAllowAuthenticator(authenticationMetadata), false, false, nil
 	case *configuration.AuthenticationPolicy_Any:
 		children := make([]Authenticator, 0, len(policyKind.Any.Policies))
 		needsPeerTransportCredentials := false
+		requestTLSClientCertificate := false
 		for _, childConfiguration := range policyKind.Any.Policies {
-			child, childNeedsPeerTransportCredentials, err := NewAuthenticatorFromConfiguration(childConfiguration, group)
+			child, childNeedsPeerTransportCredentials, childRequestTLSClientCertificate, err := NewAuthenticatorFromConfiguration(childConfiguration, group)
 			if err != nil {
-				return nil, false, err
+				return nil, false, false, err
 			}
 			children = append(children, child)
 			needsPeerTransportCredentials = needsPeerTransportCredentials || childNeedsPeerTransportCredentials
+			requestTLSClientCertificate = requestTLSClientCertificate || childRequestTLSClientCertificate
 		}
-		return NewAnyAuthenticator(children), needsPeerTransportCredentials, nil
+		return NewAnyAuthenticator(children), needsPeerTransportCredentials, requestTLSClientCertificate, nil
 	case *configuration.AuthenticationPolicy_All:
 		children := make([]Authenticator, 0, len(policyKind.All.Policies))
 		needsPeerTransportCredentials := false
+		requestTLSClientCertificate := false
 		for _, childConfiguration := range policyKind.All.Policies {
-			child, childNeedsPeerTransportCredentials, err := NewAuthenticatorFromConfiguration(childConfiguration, group)
+			child, childNeedsPeerTransportCredentials, childRequestTLSClientCertificate, err := NewAuthenticatorFromConfiguration(childConfiguration, group)
 			if err != nil {
-				return nil, false, err
+				return nil, false, false, err
 			}
 			children = append(children, child)
 			needsPeerTransportCredentials = needsPeerTransportCredentials || childNeedsPeerTransportCredentials
+			requestTLSClientCertificate = requestTLSClientCertificate || childRequestTLSClientCertificate
 		}
-		return NewAllAuthenticator(children), needsPeerTransportCredentials, nil
+		return NewAllAuthenticator(children), needsPeerTransportCredentials, requestTLSClientCertificate, nil
 	case *configuration.AuthenticationPolicy_Deny:
-		return NewDenyAuthenticator(policyKind.Deny), false, nil
+		return NewDenyAuthenticator(policyKind.Deny), false, false, nil
 	case *configuration.AuthenticationPolicy_TlsClientCertificate:
 		clientCAs := x509.NewCertPool()
 		if !clientCAs.AppendCertsFromPEM([]byte(policyKind.TlsClientCertificate.ClientCertificateAuthorities)) {
-			return nil, false, status.Error(codes.InvalidArgument, "Failed to parse client certificate authorities")
+			return nil, false, false, status.Error(codes.InvalidArgument, "Failed to parse client certificate authorities")
 		}
 		validator, err := jmespath.Compile(policyKind.TlsClientCertificate.ValidationJmespathExpression)
 		if err != nil {
-			return nil, false, util.StatusWrap(err, "Failed to compile validation JMESPath expression")
+			return nil, false, false, util.StatusWrap(err, "Failed to compile validation JMESPath expression")
 		}
 		metadataExtractor, err := jmespath.Compile(policyKind.TlsClientCertificate.MetadataExtractionJmespathExpression)
 		if err != nil {
-			return nil, false, util.StatusWrap(err, "Failed to compile metadata extraction JMESPath expression")
+			return nil, false, false, util.StatusWrap(err, "Failed to compile metadata extraction JMESPath expression")
 		}
 		return NewTLSClientCertificateAuthenticator(
 			clientCAs,
 			clock.SystemClock,
 			validator,
 			metadataExtractor,
-		), false, nil
+		), false, true, nil
 	case *configuration.AuthenticationPolicy_Jwt:
 		authorizationHeaderParser, err := jwt.NewAuthorizationHeaderParserFromConfiguration(policyKind.Jwt, group)
 		if err != nil {
-			return nil, false, util.StatusWrap(err, "Failed to create authorization header parser for JWT authentication policy")
+			return nil, false, false, util.StatusWrap(err, "Failed to create authorization header parser for JWT authentication policy")
 		}
-		return NewJWTAuthenticator(authorizationHeaderParser), false, nil
+		return NewJWTAuthenticator(authorizationHeaderParser), false, false, nil
 	case *configuration.AuthenticationPolicy_PeerCredentialsJmespathExpression:
 		metadataExtractor, err := jmespath.Compile(policyKind.PeerCredentialsJmespathExpression)
 		if err != nil {
-			return nil, false, util.StatusWrap(err, "Failed to compile peer credentials metadata extraction JMESPath expression")
+			return nil, false, false, util.StatusWrap(err, "Failed to compile peer credentials metadata extraction JMESPath expression")
 		}
-		return NewPeerCredentialsAuthenticator(metadataExtractor), true, nil
+		return NewPeerCredentialsAuthenticator(metadataExtractor), true, false, nil
 	default:
-		return nil, false, status.Error(codes.InvalidArgument, "Configuration did not contain an authentication policy type")
+		return nil, false, false, status.Error(codes.InvalidArgument, "Configuration did not contain an authentication policy type")
 	}
 }
diff --git a/pkg/grpc/server.go b/pkg/grpc/server.go
@@ -36,7 +36,7 @@ func init() {
 func NewServersFromConfigurationAndServe(configurations []*configuration.ServerConfiguration, registrationFunc func(grpc.ServiceRegistrar), group program.Group) error {
 	for _, configuration := range configurations {
 		// Create an authenticator for requests.
-		authenticator, needsPeerTransportCredentials, err := NewAuthenticatorFromConfiguration(configuration.AuthenticationPolicy, group)
+		authenticator, needsPeerTransportCredentials, requestTLSClientCertificate, err := NewAuthenticatorFromConfiguration(configuration.AuthenticationPolicy, group)
 		if err != nil {
 			return err
 		}
@@ -70,7 +70,7 @@ func NewServersFromConfigurationAndServe(configurations []*configuration.ServerC
 
 		// Enable TLS transport credentials if provided.
 		hasCredsOption := false
-		if tlsConfig, err := util.NewTLSConfigFromServerConfiguration(configuration.Tls); err != nil {
+		if tlsConfig, err := util.NewTLSConfigFromServerConfiguration(configuration.Tls, requestTLSClientCertificate); err != nil {
 			return err
 		} else if tlsConfig != nil {
 			hasCredsOption = true

diff --git a/pkg/http/server.go b/pkg/http/server.go
@@ -22,7 +22,10 @@ func NewServersFromConfigurationAndServe(configurations []*configuration.ServerC
 			}
 			authenticatedHandler := NewAuthenticatingHandler(handler, authenticator)
 
-			tlsConfig, err := util.NewTLSConfigFromServerConfiguration(configuration.Tls)
+			tlsConfig, err := util.NewTLSConfigFromServerConfiguration(
+				configuration.Tls,
+				/* requestClientCertificate = */ false,
+			)
 			if err != nil {
 				return err
 			}

diff --git a/pkg/util/tls.go b/pkg/util/tls.go
@@ -179,7 +179,7 @@ func NewTLSConfigFromClientConfiguration(configuration *pb.ClientConfiguration)
 // object based on parameters specified in a Protobuf message for use
 // with a TLS server. This Protobuf message is embedded in Buildbarn
 // configuration files.
-func NewTLSConfigFromServerConfiguration(configuration *pb.ServerConfiguration) (*tls.Config, error) {
+func NewTLSConfigFromServerConfiguration(configuration *pb.ServerConfiguration, requestClientCertificate bool) (*tls.Config, error) {
 	tlsPrometheusMetrics.Do(func() {
 		prometheus.MustRegister(tlsCertificateNotAfterTimeSeconds)
 		prometheus.MustRegister(tlsCertificateNotBeforeTimeSeconds)
@@ -193,7 +193,9 @@ func NewTLSConfigFromServerConfiguration(configuration *pb.ServerConfiguration)
 	if err != nil {
 		return nil, err
 	}
-	tlsConfig.ClientAuth = tls.RequestClientCert
+	if requestClientCertificate {
+		tlsConfig.ClientAuth = tls.RequestClientCert
+	}
 
 	if configuration.ServerKeyPair == nil {
 		return nil, StatusWrapWithCode(err, codes.InvalidArgument, "Missing server_key_pair configuration")

diff --git a/pkg/util/tls_test.go b/pkg/util/tls_test.go
@@ -279,7 +279,7 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 	t.Run("Disabled", func(t *testing.T) {
 		// When the TLS configuration is nil, TLS should be left
 		// disabled.
-		tlsConfig, err := util.NewTLSConfigFromServerConfiguration(nil)
+		tlsConfig, err := util.NewTLSConfigFromServerConfiguration(nil, false)
 		require.NoError(t, err)
 		require.Nil(t, tlsConfig)
 	})
@@ -297,7 +297,9 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 						},
 					},
 				},
-			})
+			},
+			/* requestClientCertificate = */ true,
+		)
 		require.NoError(t, err)
 		cert, err := tlsConfig.GetCertificate(nil)
 		require.NoError(t, err)
@@ -323,7 +325,9 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 						},
 					},
 				},
-			})
+			},
+			/* requestClientCertificate = */ true,
+		)
 		require.NoError(t, err)
 		cert, err := tlsConfig.GetCertificate(nil)
 		require.NoError(t, err)
@@ -346,7 +350,9 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 						},
 					},
 				},
-			})
+			},
+			/* requestClientCertificate = */ false,
+		)
 		testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Failed to configure server TLS: Invalid certificate or private key: tls: failed to find any PEM data in certificate input"), err)
 	})
 
@@ -362,7 +368,9 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 						},
 					},
 				},
-			})
+			},
+			/* requestClientCertificate = */ false,
+		)
 		testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Failed to configure server TLS: Failed to initialize certificate: Invalid certificate file or private key file: tls: failed to find any PEM data in certificate input"), err)
 	})
 
@@ -378,7 +386,9 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 						},
 					},
 				},
-			})
+			},
+			/* requestClientCertificate = */ false,
+		)
 		testutil.RequirePrefixedStatus(t, status.Error(codes.InvalidArgument, "Failed to configure server TLS: Failed to initialize certificate: Failed to read certificate file: open /missing-cert.pem: no such file or directory"), err)
 	})
 
@@ -400,15 +410,16 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 					"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256",
 					"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
 				},
-			})
+			},
+			/* requestClientCertificate = */ false,
+		)
 		require.NoError(t, err)
 		cert, err := tlsConfig.GetCertificate(nil)
 		require.NoError(t, err)
 		require.Len(t, cert.Certificate, 1)
 		tlsConfig.GetCertificate = nil
 		require.Equal(t, &tls.Config{
 			MinVersion: tls.VersionTLS12,
-			ClientAuth: tls.RequestClientCert,
 			CipherSuites: []uint16{
 				tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
 				tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
@@ -432,7 +443,9 @@ func TestTLSConfigFromServerConfiguration(t *testing.T) {
 				CipherSuites: []string{
 					"TLS_ECDHE_ECDSA_WITH_AES_257_GCM_SHA385",
 				},
-			})
+			},
+			/* requestClientCertificate = */ false,
+		)
 		testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Unsupported cipher suite: \"TLS_ECDHE_ECDSA_WITH_AES_257_GCM_SHA385\""), err)
 	})
 }