fix: Issue DeviceWebToken during Github authn (#44656)

* Move trusted device mode calculation into CreateWebSessionFromReq * Add the GithubAuthRequest.ClientUserAgent field * Update generated protos * Issue DeviceWebToken during Github authn * Issue DeviceWebToken as part of CreateWebSessionFromReq * nit: Move methods to sessions.go
gravitational · Jul 30, 2024 · a03245b · a03245b
1 parent ca24d48
commit a03245b
Show file tree

Hide file tree

Showing 8 changed files with 1,846 additions and 1,752 deletions.
diff --git a/api/proto/teleport/legacy/types/types.proto b/api/proto/teleport/legacy/types/types.proto
@@ -4755,6 +4755,9 @@ message GithubAuthRequest {
   teleport.attestation.v1.AttestationStatement attestation_statement = 16 [(gogoproto.jsontag) = "attestation_statement,omitempty"];
   // ClientLoginIP specifies IP address of the client for login, it will be written to the user's certificates.
   string ClientLoginIP = 17 [(gogoproto.jsontag) = "client_login_ip,omitempty"];
+  // ClientUserAgent is the user agent of the Web browser, used for issuing
+  // a DeviceWebToken.
+  string ClientUserAgent = 18 [(gogoproto.jsontag) = "client_user_agent,omitempty"];
 }
 
 // SSOWarnings conveys a user-facing main message along with auxiliary warnings.

diff --git a/api/types/types.pb.go b/api/types/types.pb.go
diff --git a/lib/auth/auth.go b/lib/auth/auth.go
@@ -4092,7 +4092,7 @@ func (a *Server) ExtendWebSession(ctx context.Context, req authclient.WebSession
 	}
 
 	sessionTTL := utils.ToTTL(a.clock, expiresAt)
-	sess, err := a.newWebSession(ctx, NewWebSessionRequest{
+	sess, _, err := a.newWebSession(ctx, NewWebSessionRequest{
 		User:                 req.User,
 		LoginIP:              identity.LoginIP,
 		Roles:                roles,

diff --git a/lib/auth/auth_test.go b/lib/auth/auth_test.go
@@ -3012,7 +3012,7 @@ func TestNewWebSession(t *testing.T) {
 	}
 	bearerTokenTTL := min(req.SessionTTL, defaults.BearerTokenTTL)
 
-	ws, err := p.a.newWebSession(ctx, req, nil /* opts */)
+	ws, _, err := p.a.newWebSession(ctx, req, nil /* opts */)
 	require.NoError(t, err)
 	require.Equal(t, user.GetName(), ws.GetUser())
 	require.Equal(t, duration, ws.GetIdleTimeout())

diff --git a/lib/auth/github.go b/lib/auth/github.go
@@ -713,13 +713,15 @@ func (a *Server) validateGithubAuthCallback(ctx context.Context, diagCtx *SSODia
 	// If the request is coming from a browser, create a web session.
 	if req.CreateWebSession {
 		session, err := a.CreateWebSessionFromReq(ctx, NewWebSessionRequest{
-			User:             userState.GetName(),
-			Roles:            userState.GetRoles(),
-			Traits:           userState.GetTraits(),
-			SessionTTL:       params.SessionTTL,
-			LoginTime:        a.clock.Now().UTC(),
-			LoginIP:          req.ClientLoginIP,
-			AttestWebSession: true,
+			User:                 userState.GetName(),
+			Roles:                userState.GetRoles(),
+			Traits:               userState.GetTraits(),
+			SessionTTL:           params.SessionTTL,
+			LoginTime:            a.clock.Now().UTC(),
+			LoginIP:              req.ClientLoginIP,
+			LoginUserAgent:       req.ClientUserAgent,
+			AttestWebSession:     true,
+			CreateDeviceWebToken: true,
 		})
 		if err != nil {
 			return nil, trace.Wrap(err, "Failed to create web session.")

diff --git a/lib/auth/methods.go b/lib/auth/methods.go
@@ -30,17 +30,14 @@ import (
 
 	"github.com/gravitational/teleport/api/client/proto"
 	"github.com/gravitational/teleport/api/constants"
-	devicepb "github.com/gravitational/teleport/api/gen/proto/go/teleport/devicetrust/v1"
 	mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1"
 	"github.com/gravitational/teleport/api/types"
 	apievents "github.com/gravitational/teleport/api/types/events"
 	"github.com/gravitational/teleport/lib/auth/authclient"
 	wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
 	"github.com/gravitational/teleport/lib/authz"
 	"github.com/gravitational/teleport/lib/defaults"
-	dtconfig "github.com/gravitational/teleport/lib/devicetrust/config"
 	"github.com/gravitational/teleport/lib/events"
-	"github.com/gravitational/teleport/lib/modules"
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/utils"
 )
@@ -630,89 +627,22 @@ func (a *Server) AuthenticateWebUser(ctx context.Context, req authclient.Authent
 	}
 
 	sess, err := a.CreateWebSessionFromReq(ctx, NewWebSessionRequest{
-		User:             user.GetName(),
-		LoginIP:          loginIP,
-		Roles:            user.GetRoles(),
-		Traits:           user.GetTraits(),
-		LoginTime:        a.clock.Now().UTC(),
-		AttestWebSession: true,
+		User:                 user.GetName(),
+		LoginIP:              loginIP,
+		LoginUserAgent:       userAgent,
+		Roles:                user.GetRoles(),
+		Traits:               user.GetTraits(),
+		LoginTime:            a.clock.Now().UTC(),
+		AttestWebSession:     true,
+		CreateDeviceWebToken: true,
 	})
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	// Create the device trust DeviceWebToken.
-	// We only get a token if the server is enabled for Device Trust and the user
-	// has a suitable trusted device.
-	if loginIP != "" && userAgent != "" {
-		webToken, err := a.createDeviceWebToken(ctx, &devicepb.DeviceWebToken{
-			WebSessionId:     sess.GetName(),
-			BrowserUserAgent: userAgent,
-			BrowserIp:        loginIP,
-			User:             sess.GetUser(),
-		})
-		switch {
-		case err != nil:
-			log.WithError(err).Warn("Failed to create DeviceWebToken for user")
-		case webToken != nil: // May be nil even if err==nil.
-			sess.SetDeviceWebToken(&types.DeviceWebToken{
-				Id:    webToken.Id,
-				Token: webToken.Token,
-			})
-		}
-	}
-
-	// Calculate the trusted device requirement for the session. Helps inform the
-	// frontend if the user might run into access problems without a trusted
-	// device.
-	trustedDeviceRequirement, err := a.calculateTrustedDeviceMode(ctx, func() ([]types.Role, error) {
-		// TODO(codingllama): Levegare the checker inside CreateWebSessionFromReq to
-		//  avoid duplicate work here.
-		roles, err := services.FetchRoles(user.GetRoles(), a, user.GetTraits())
-		if err != nil {
-			return nil, trace.Wrap(err)
-		}
-		return roles, nil
-	})
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-	sess.SetTrustedDeviceRequirement(trustedDeviceRequirement)
-
 	return sess, nil
 }
 
-func (a *Server) calculateTrustedDeviceMode(ctx context.Context, getRoles func() ([]types.Role, error)) (types.TrustedDeviceRequirement, error) {
-	const unspecified = types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_UNSPECIFIED
-
-	// Don't evaluate for OSS.
-	if !modules.GetModules().IsEnterpriseBuild() {
-		return unspecified, nil
-	}
-
-	// Required by cluster mode?
-	ap, err := a.GetAuthPreference(ctx)
-	if err != nil {
-		return unspecified, trace.Wrap(err)
-	}
-	if dtconfig.GetEffectiveMode(ap.GetDeviceTrust()) == constants.DeviceTrustModeRequired {
-		return types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_REQUIRED, nil
-	}
-
-	// Required by roles?
-	roles, err := getRoles()
-	if err != nil {
-		return unspecified, trace.Wrap(err)
-	}
-	for _, role := range roles {
-		if role.GetOptions().DeviceTrustMode == constants.DeviceTrustModeRequired {
-			return types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_REQUIRED, nil
-		}
-	}
-
-	return types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_NOT_REQUIRED, nil
-}
-
 // AuthenticateSSHUser authenticates an SSH user and returns SSH and TLS
 // certificates for the public key in req.
 func (a *Server) AuthenticateSSHUser(ctx context.Context, req authclient.AuthenticateSSHRequest) (*authclient.SSHLoginResponse, error) {

diff --git a/lib/auth/sessions.go b/lib/auth/sessions.go
@@ -26,13 +26,16 @@ import (
 
 	"github.com/gravitational/teleport"
 	"github.com/gravitational/teleport/api/client/proto"
+	"github.com/gravitational/teleport/api/constants"
 	apidefaults "github.com/gravitational/teleport/api/defaults"
+	devicepb "github.com/gravitational/teleport/api/gen/proto/go/teleport/devicetrust/v1"
 	mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1"
 	"github.com/gravitational/teleport/api/types"
 	apievents "github.com/gravitational/teleport/api/types/events"
 	"github.com/gravitational/teleport/api/utils/keys"
 	"github.com/gravitational/teleport/lib/auth/native"
 	"github.com/gravitational/teleport/lib/defaults"
+	dtconfig "github.com/gravitational/teleport/lib/devicetrust/config"
 	"github.com/gravitational/teleport/lib/events"
 	"github.com/gravitational/teleport/lib/jwt"
 	"github.com/gravitational/teleport/lib/modules"
@@ -48,6 +51,9 @@ type NewWebSessionRequest struct {
 	User string
 	// LoginIP is an observed IP of the client, it will be embedded into certificates.
 	LoginIP string
+	// LoginUserAgent is the user agent of the client's browser, as captured by
+	// the Proxy.
+	LoginUserAgent string
 	// Roles optionally lists additional user roles
 	Roles []string
 	// Traits optionally lists role traits
@@ -69,6 +75,13 @@ type NewWebSessionRequest struct {
 	// This should be provided when extending an attested web session in order to maintain the
 	// session attested status.
 	PrivateKey *keys.PrivateKey
+	// CreateDeviceWebToken informs Auth to issue a DeviceWebToken when creating
+	// this session.
+	// A DeviceWebToken must only be issued for users that have been authenticated
+	// in the same RPC.
+	// May only be set internally by Auth (and Auth-related logic), not allowed
+	// for external requests.
+	CreateDeviceWebToken bool
 }
 
 // CheckAndSetDefaults validates the request and sets defaults.
@@ -89,7 +102,7 @@ func (r *NewWebSessionRequest) CheckAndSetDefaults() error {
 }
 
 func (a *Server) CreateWebSessionFromReq(ctx context.Context, req NewWebSessionRequest) (types.WebSession, error) {
-	session, err := a.newWebSession(ctx, req, nil /* opts */)
+	session, sessionChecker, err := a.newWebSession(ctx, req, nil /* opts */)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -99,9 +112,96 @@ func (a *Server) CreateWebSessionFromReq(ctx context.Context, req NewWebSessionR
 		return nil, trace.Wrap(err)
 	}
 
+	// Issue and assign the DeviceWebToken, but never persist it with the
+	// session.
+	if req.CreateDeviceWebToken {
+		if err := a.augmentSessionForDeviceTrust(ctx, session, req.LoginIP, req.LoginUserAgent); err != nil {
+			return nil, trace.Wrap(err)
+		}
+	}
+
+	// Assign the TrustedDeviceRequirement to the session, but do not persist it,
+	// so only the initial session gets it.
+	// This avoids persisting a possibly stale value.
+	if tdr, err := a.calculateTrustedDeviceMode(ctx, func() ([]types.Role, error) {
+		return sessionChecker.Roles(), nil
+	}); err != nil {
+		log.
+			WithError(err).
+			Warnf("Failed to calculate trusted device mode for session")
+	} else {
+		session.SetTrustedDeviceRequirement(tdr)
+	}
+
 	return session, nil
 }
 
+func (a *Server) augmentSessionForDeviceTrust(
+	ctx context.Context,
+	session types.WebSession,
+	loginIP, userAgent string,
+) error {
+	// IP and user agent are mandatory for device web authentication.
+	if loginIP == "" || userAgent == "" {
+		return nil
+	}
+
+	// Create the device trust DeviceWebToken.
+	// We only get a token if the server is enabled for Device Trust and the user
+	// has a suitable trusted device.
+	webToken, err := a.createDeviceWebToken(ctx, &devicepb.DeviceWebToken{
+		WebSessionId:     session.GetName(),
+		BrowserUserAgent: userAgent,
+		BrowserIp:        loginIP,
+		User:             session.GetUser(),
+	})
+	switch {
+	case err != nil:
+		log.WithError(err).Warn("Failed to create DeviceWebToken for user")
+	case webToken != nil: // May be nil even if err==nil.
+		session.SetDeviceWebToken(&types.DeviceWebToken{
+			Id:    webToken.Id,
+			Token: webToken.Token,
+		})
+	}
+
+	return nil
+}
+
+func (a *Server) calculateTrustedDeviceMode(
+	ctx context.Context,
+	getRoles func() ([]types.Role, error),
+) (types.TrustedDeviceRequirement, error) {
+	const unspecified = types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_UNSPECIFIED
+
+	// Don't evaluate for OSS.
+	if !modules.GetModules().IsEnterpriseBuild() {
+		return unspecified, nil
+	}
+
+	// Required by cluster mode?
+	ap, err := a.GetAuthPreference(ctx)
+	if err != nil {
+		return unspecified, trace.Wrap(err)
+	}
+	if dtconfig.GetEffectiveMode(ap.GetDeviceTrust()) == constants.DeviceTrustModeRequired {
+		return types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_REQUIRED, nil
+	}
+
+	// Required by roles?
+	roles, err := getRoles()
+	if err != nil {
+		return unspecified, trace.Wrap(err)
+	}
+	for _, role := range roles {
+		if role.GetOptions().DeviceTrustMode == constants.DeviceTrustModeRequired {
+			return types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_REQUIRED, nil
+		}
+	}
+
+	return types.TrustedDeviceRequirement_TRUSTED_DEVICE_REQUIREMENT_NOT_REQUIRED, nil
+}
+
 // newWebSessionOpts are WebSession creation options exclusive to Auth.
 // These options complement [types.NewWebSessionRequest].
 // See [Server.newWebSession].
@@ -117,10 +217,10 @@ func (a *Server) newWebSession(
 	ctx context.Context,
 	req NewWebSessionRequest,
 	opts *newWebSessionOpts,
-) (types.WebSession, error) {
+) (types.WebSession, services.AccessChecker, error) {
 	userState, err := a.GetUserOrLoginState(ctx, req.User)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 	if req.LoginIP == "" {
 		// TODO(antonam): consider turning this into error after all use cases are covered (before v14.0 testplan)
@@ -129,7 +229,7 @@ func (a *Server) newWebSession(
 
 	clusterName, err := a.GetClusterName()
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 
 	checker, err := services.NewAccessChecker(&services.AccessInfo{
@@ -138,18 +238,18 @@ func (a *Server) newWebSession(
 		AllowedResourceIDs: req.RequestedResourceIDs,
 	}, clusterName.GetClusterName(), a)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 
 	idleTimeout, err := a.getWebIdleTimeout(ctx)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 
 	if req.PrivateKey == nil {
 		req.PrivateKey, err = native.GeneratePrivateKey()
 		if err != nil {
-			return nil, trace.Wrap(err)
+			return nil, nil, trace.Wrap(err)
 		}
 	}
 
@@ -188,15 +288,15 @@ func (a *Server) newWebSession(
 
 	certs, err := a.generateUserCert(ctx, certReq)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 	token, err := utils.CryptoRandomHex(defaults.SessionTokenBytes)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 	bearerToken, err := utils.CryptoRandomHex(defaults.SessionTokenBytes)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
 	bearerTokenTTL := min(sessionTTL, defaults.BearerTokenTTL)
 
@@ -221,9 +321,9 @@ func (a *Server) newWebSession(
 
 	sess, err := types.NewWebSession(token, types.KindWebSession, sessionSpec)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, nil, trace.Wrap(err)
 	}
-	return sess, nil
+	return sess, checker, nil
 }
 
 func (a *Server) getWebIdleTimeout(ctx context.Context) (time.Duration, error) {
@@ -352,6 +452,10 @@ func (a *Server) CreateAppSessionFromReq(ctx context.Context, req NewAppSessionR
 			"this Teleport cluster is not licensed for application access, please contact the cluster administrator")
 	}
 
+	if req.CreateDeviceWebToken {
+		return nil, trace.BadParameter("parameter CreateDeviceWebToken disallowed for App Sessions")
+	}
+
 	user, err := a.GetUserOrLoginState(ctx, req.User)
 	if err != nil {
 		return nil, trace.Wrap(err)