Perform full SSH RBAC checks in VerifiedPublicKeyCallback and only check cert/key in PublicKeyCallback.

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

Author: cthach

lib/srv/authhandlers.go

@@ -372,9 +372,44 @@ func (h *AuthHandlers) CheckPortForward(addr string, ctx *ServerContext, request
 	return nil
 }
 
-// UserKeyAuth implements SSH client authentication using public keys and is
-// called by the server every time the client connects.
-func (h *AuthHandlers) UserKeyAuth(conn ssh.ConnMetadata, key ssh.PublicKey) (ppms *ssh.Permissions, rerr error) {
+// PublicKeyCallback validates a user key or certificate for SSH public key authentication before key possession has
+// been proven.
+//
+// This method is to be used as the PublicKeyCallback in ssh.ServerConfig and performs a basic check to verify that the
+// certificate was signed by a trusted authority and that the certificate metadata is valid. It DOES NOT perform any
+// RBAC checks and is used as a preliminary step before the more in-depth VerifiedPublicKeyCallback, which performs RBAC
+// checks and is called only after the client proves possession of the key. If the certificate is valid, this callback
+// returns an empty ssh.Permissions object. If the certificate is invalid, it returns a non-nil error to reject the auth
+// attempt.
+func (h *AuthHandlers) PublicKeyCallback(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) {
+	checker := apisshutils.CertChecker{
+		CertChecker: ssh.CertChecker{
+			IsUserAuthority: h.IsUserAuthority,
+			Clock:           h.c.Clock.Now,
+		},
+		FIPS: h.c.FIPS,
+	}
+
+	if _, err := checker.Authenticate(conn, key); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	return &ssh.Permissions{}, nil
+}
+
+// VerifiedPublicKeyCallback performs full SSH user authentication and authorization after the client proves key
+// possession.
+//
+// This method is to be used as the VerifiedPublicKeyCallback in ssh.ServerConfig and performs RBAC checks to
+// determine if the user is authorized to log in. If the user is authorized, this callback returns an ssh.Permissions
+// object that includes any relevant access permits encoded in the Extensions field, which can then be used by the rest
+// of the connection handling logic to determine what actions the user is authorized to perform in their session.
+func (h *AuthHandlers) VerifiedPublicKeyCallback(
+	conn ssh.ConnMetadata,
+	key ssh.PublicKey,
+	_ *ssh.Permissions,
+	_ string,
+) (ppms *ssh.Permissions, rerr error) {
 	ctx := context.Background()
 
 	fingerprint := fmt.Sprintf("%v %v", key.Type(), sshutils.Fingerprint(key))

lib/srv/authhandlers_test.go

@@ -294,7 +294,7 @@ func TestRBAC(t *testing.T) {
 			require.NoError(t, err)
 
 			// perform public key authentication
-			_, err = ah.UserKeyAuth(&mockConnMetadata{}, cert)
+			_, err = ah.VerifiedPublicKeyCallback(&mockConnMetadata{}, cert, nil, "")
 			require.NoError(t, err)
 
 			tt.loginRBACCheck(t, lc.rbacChecked)
@@ -608,7 +608,7 @@ func TestScopedRBAC(t *testing.T) {
 			require.NoError(t, err)
 
 			// preform public key authentication
-			_, err = ah.UserKeyAuth(&mockConnMetadata{}, cert)
+			_, err = ah.VerifiedPublicKeyCallback(&mockConnMetadata{}, cert, nil, "")
 			if tt.allowed {
 				require.NoError(t, err)
 			} else {
@@ -736,11 +736,11 @@ func TestForwardingGitLocalOnly(t *testing.T) {
 	require.NoError(t, err)
 
 	// verify that authentication succeeds for local cert but is rejected categorically for remote
-	_, err = ah.UserKeyAuth(&mockConnMetadata{}, localCert)
+	_, err = ah.VerifiedPublicKeyCallback(&mockConnMetadata{}, localCert, nil, "")
 	require.NoError(t, err)
 	require.True(t, gc.rbacChecked)
 
-	_, err = ah.UserKeyAuth(&mockConnMetadata{}, remoteCert)
+	_, err = ah.VerifiedPublicKeyCallback(&mockConnMetadata{}, remoteCert, nil, "")
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "cross-cluster git forwarding is not supported")
 }
@@ -1067,7 +1067,7 @@ func TestAuthAttemptAuditEvent(t *testing.T) {
 	require.NoError(t, err)
 
 	// perform public key authentication, should fail because no login checker set
-	_, err = ah.UserKeyAuth(&mockConnMetadata{}, cert)
+	_, err = ah.VerifiedPublicKeyCallback(&mockConnMetadata{}, cert, nil, "")
 	require.Error(t, err)
 
 	// audit event (AuthAttempt) should include node's host id and hostname

lib/srv/forward/sshserver.go

@@ -579,8 +579,9 @@ func (s *Server) Serve() {
 		config    = &ssh.ServerConfig{}
 	)
 
-	// Configure callback for user certificate authentication.
-	config.PublicKeyCallback = s.authHandlers.UserKeyAuth
+	// Configure callbacks for user certificate authentication.
+	config.PublicKeyCallback = s.authHandlers.PublicKeyCallback
+	config.VerifiedPublicKeyCallback = s.authHandlers.VerifiedPublicKeyCallback
 
 	// Set host certificate the in-memory server will present to clients.
 	config.AddHostKey(s.hostCertificate)

lib/srv/git/forward.go

@@ -267,7 +267,8 @@ func (s *ForwardServer) Serve() {
 		sshutils.NewChanHandlerFunc(s.onChannel),
 		sshutils.StaticHostSigners(s.cfg.HostCertificate),
 		sshutils.AuthMethods{
-			PublicKey: s.userKeyAuth,
+			PublicKey:         s.publicKeyCallback,
+			VerifiedPublicKey: s.verifiedPublicKeyCallback,
 		},
 		sshutils.SetFIPS(s.cfg.FIPS),
 		sshutils.SetCiphers(s.cfg.Ciphers),
@@ -295,7 +296,7 @@ func (s *ForwardServer) close() {
 	}
 }
 
-func (s *ForwardServer) userKeyAuth(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) {
+func checkAndSetGitUser(conn ssh.ConnMetadata, key ssh.PublicKey) (ssh.ConnMetadata, error) {
 	cert, ok := key.(*ssh.Certificate)
 	if !ok {
 		return nil, trace.BadParameter("unsupported key type")
@@ -319,9 +320,31 @@ func (s *ForwardServer) userKeyAuth(conn ssh.ConnMetadata, key ssh.PublicKey) (*
 		conn = sshutils.NewSSHConnMetadataWithUser(conn, ident.Principals[0])
 	}
 
-	// Use auth.UserKeyAuth to verify user cert is signed by UserCA and to evaluate
-	// RBAC permissions.
-	permissions, err := s.auth.UserKeyAuth(conn, key)
+	return conn, nil
+}
+
+func (s *ForwardServer) publicKeyCallback(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) {
+	conn, err := checkAndSetGitUser(conn, key)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	permissions, err := s.auth.PublicKeyCallback(conn, key)
+	if err != nil {
+		userKeyAuthFailureCounter.Inc()
+		return nil, trace.Wrap(err)
+	}
+
+	return permissions, nil
+}
+
+func (s *ForwardServer) verifiedPublicKeyCallback(conn ssh.ConnMetadata, key ssh.PublicKey, _ *ssh.Permissions, _ string) (*ssh.Permissions, error) {
+	conn, err := checkAndSetGitUser(conn, key)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	permissions, err := s.auth.VerifiedPublicKeyCallback(conn, key, nil, "")
 	if err != nil {
 		userKeyAuthFailureCounter.Inc()
 		return nil, trace.Wrap(err)

lib/srv/regular/sshserver.go

@@ -956,7 +956,10 @@ func New(
 		component,
 		addr, s,
 		getHostSigners,
-		sshutils.AuthMethods{PublicKey: s.authHandlers.UserKeyAuth},
+		sshutils.AuthMethods{
+			PublicKey:         s.authHandlers.PublicKeyCallback,
+			VerifiedPublicKey: s.authHandlers.VerifiedPublicKeyCallback,
+		},
 		sshutils.SetLimiter(s.limiter),
 		sshutils.SetRequestHandler(s),
 		sshutils.SetNewConnHandler(s),

lib/sshutils/server.go

@@ -237,6 +237,7 @@ func NewServer(
 	}
 
 	s.cfg.PublicKeyCallback = ah.PublicKey
+	s.cfg.VerifiedPublicKeyCallback = ah.VerifiedPublicKey
 	s.cfg.PasswordCallback = ah.Password
 	s.cfg.NoClientAuth = ah.NoClient
 
@@ -723,9 +724,10 @@ func (f NewConnHandlerFunc) HandleNewConn(ctx context.Context, ccx *ConnectionCo
 }
 
 type AuthMethods struct {
-	PublicKey PublicKeyFunc
-	Password  PasswordFunc
-	NoClient  bool
+	PublicKey         PublicKeyFunc
+	VerifiedPublicKey VerifiedPublicKeyFunc
+	Password          PasswordFunc
+	NoClient          bool
 }
 
 // GetHostSignersFunc is an infallible function that returns host signers for
@@ -802,7 +804,15 @@ func validateHostSigner(fips bool, signer ssh.Signer) error {
 
 type (
 	PublicKeyFunc func(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error)
-	PasswordFunc  func(conn ssh.ConnMetadata, password []byte) (*ssh.Permissions, error)
+
+	VerifiedPublicKeyFunc func(
+		conn ssh.ConnMetadata,
+		key ssh.PublicKey,
+		permissions *ssh.Permissions,
+		signatureAlgorithm string,
+	) (*ssh.Permissions, error)
+
+	PasswordFunc func(conn ssh.ConnMetadata, password []byte) (*ssh.Permissions, error)
 )
 
 // ClusterDetails specifies information about a cluster