Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always issue relogin and per-session mfa requests for the root cluster URI #42355

Merged
merged 8 commits into from
Jun 5, 2024
Merged

Always issue relogin and per-session mfa requests for the root cluster URI #42355

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
95f5beb
Always issue per-session MFA requests for a root cluster URI
gzdunek Jun 4, 2024
263c323
Always issue relogin requests for a root cluster URI
gzdunek Jun 4, 2024
68c79ec
Check if cluster URI is root cluster URI in login handlers
gzdunek Jun 4, 2024
d3ad8ef
Pass clusterUri instead of rootClusterUri to the ReAuthenticate dialog
gzdunek Jun 4, 2024
404976b
Merge branch 'refs/heads/master' into gzdunek/relogin-and-session-mfa…
gzdunek Jun 5, 2024
239d3b5
Reserve `root_cluster_uri` field too
gzdunek Jun 5, 2024
31cdaba
Merge branch 'refs/heads/master' into gzdunek/relogin-and-session-mfa…
gzdunek Jun 5, 2024
a6b7484
Resolve conflicts
gzdunek Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 59 additions & 59 deletions gen/proto/go/teleport/lib/teleterm/v1/tshd_events_service.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 13 additions & 13 deletions gen/proto/ts/teleport/lib/teleterm/v1/tshd_events_service_pb.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions lib/teleterm/apiserver/handler/handler_auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ func (s *Handler) Login(ctx context.Context, req *api.LoginRequest) (*api.EmptyR
if err != nil {
return nil, trace.Wrap(err)
}

if !cluster.URI.IsRoot() {
return nil, trace.BadParameter("cluster URI must be a root URI")
}

// The credentials + MFA login flow in the Electron app assumes that the default CLI prompt is
// used and works around that. Thus we have to remove the teleterm-specific MFAPromptConstructor
// added by daemon.Service.ResolveClusterURI.
Expand Down Expand Up @@ -83,6 +88,11 @@ func (s *Handler) LoginPasswordless(stream api.TerminalService_LoginPasswordless
if err != nil {
return trace.Wrap(err)
}

if !cluster.URI.IsRoot() {
return trace.BadParameter("cluster URI must be a root URI")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine for now, but in a completely new code it'd have been better to convert the string into uri.ResourceURI, then check if it points to a root cluster and only then call s.DaemonService.ResolveClusterURI.

}

// The passwordless login flow in the Electron app assumes that the default CLI prompt is used and
// works around that. Thus we have to remove the teleterm-specific MFAPromptConstructor added by
// daemon.Service.ResolveClusterURI.
Expand Down
6 changes: 3 additions & 3 deletions lib/teleterm/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ func (s *Service) ResolveClusterURI(uri uri.ResourceURI) (*clusters.Cluster, *cl

// Custom MFAPromptConstructor gets removed during the calls to Login and LoginPasswordless RPCs.
// Those RPCs assume that the default CLI prompt is in use.
clusterClient.MFAPromptConstructor = s.NewMFAPromptConstructor(cluster.URI.String())
clusterClient.MFAPromptConstructor = s.NewMFAPromptConstructor(cluster.URI)
return cluster, clusterClient, nil
}

Expand Down Expand Up @@ -346,7 +346,7 @@ func (s *Service) createGateway(ctx context.Context, params CreateGatewayParams)
LocalPort: params.LocalPort,
OnExpiredCert: s.reissueGatewayCerts,
KubeconfigsDir: s.cfg.KubeconfigsDir,
MFAPromptConstructor: s.NewMFAPromptConstructor(targetURI.String()),
MFAPromptConstructor: s.NewMFAPromptConstructor(targetURI),
ClusterClient: clusterClient,
}

Expand All @@ -370,7 +370,7 @@ func (s *Service) createGateway(ctx context.Context, params CreateGatewayParams)
// per-session MFA checks.
func (s *Service) reissueGatewayCerts(ctx context.Context, g gateway.Gateway) (tls.Certificate, error) {
reloginReq := &api.ReloginRequest{
RootClusterUri: g.TargetURI().GetClusterURI().String(),
RootClusterUri: g.TargetURI().GetRootClusterURI().String(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, the relogin request should always use the URI of a root cluster. Otherwise the login modal targets a leaf cluster (but through the root cluster, not directly) and I'm not sure what really happens there.

Perhaps the handlers for login should also verify if they received URI to a root cluster rather than a leaf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the handlers for login should also verify if they received URI to a root cluster rather than a leaf?

Good idea, added.

Reason: &api.ReloginRequest_GatewayCertExpired{
GatewayCertExpired: &api.GatewayCertExpired{
GatewayUri: g.URI().String(),
Expand Down
Loading
Loading