Always issue relogin and per-session mfa requests for the root cluster URI #42355
Conversation
gzdunek
added
teleport-connect
| @@ -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(), | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Perhaps the handlers for login should also verify if they received URI to a root cluster rather than a leaf?
Good idea, added.
lib/teleterm/daemon/mfaprompt.go
Outdated
| @@ -125,7 +126,7 @@ func (p *mfaPrompt) promptWebauthn(ctx context.Context, chal *proto.MFAAuthentic | |||
|
|
|||
| func (p *mfaPrompt) promptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge, runOpts libmfa.RunOpts) (*proto.MFAAuthenticateResponse, error) { | |||
| resp, err := p.promptAppMFA(ctx, &api.PromptMFARequest{ | |||
| RootClusterUri: p.clusterURI, | |||
| RootClusterUri: p.resourceURI.GetRootClusterURI().String(), | |||
There was a problem hiding this comment.
This I'm not 100% sure about. Based on the modal implementation, this field is used purely for display only, unlike cluster URI in the login modal.
We might ask the user to authenticate an action that's related to a leaf resource. It's also good to know which cluster the given resource belongs to.
OTOH, the authentication step always happens in the root cluster. As in, we shouldn't make the user think that there's a separate set of devices for authenticating in root clusters and a separate set for leaf clusters. Also, we always show the modal after the user performs an action. It shouldn't ever catch someone by surprise. So I suppose they'll have a good idea as to which resource they're trying to access.
I think it'd be best if the modal said something like:
Verify your identity on [root cluster]
MFA is required to access Kubernetes cluster "minikube" from leaf cluster [leaf cluster]
This would require passing ClusterUri instead and adding some conditional logic to the modal. However, I think for now it's fine if we always show the root cluster name in the title of the modal.
For reference, this is how the modal looks like:
There was a problem hiding this comment.
OTOH, the authentication step always happens in the root cluster. As in, we shouldn't make the user think that there's a separate set of devices for authenticating in root clusters and a separate set for leaf clusters.
Yeah, that was my reasoning.
Also, we always show the modal after the user performs an action. It shouldn't ever catch someone by surprise. So I suppose they'll have a good idea as to which resource they're trying to access.
Actually, it may catch you by surprise. If you had a few tabs open that required per-session MFA , and you reopen the previous session, you will see an MFA pop-up for each of them, one after another. So perhaps I will change rootClusterUri to clusterUri in the proto message and handle it in the dialog? It doesn't seem to be too much work.
| @@ -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") | |||
There was a problem hiding this comment.
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.
…-for-root-cluster # Conflicts: # gen/proto/go/teleport/lib/teleterm/v1/tshd_events_service.pb.go
…-for-root-cluster # Conflicts: # gen/proto/go/teleport/lib/teleterm/v1/tshd_events_service.pb.go
…r URI (#42355) * Always issue per-session MFA requests for a root cluster URI * Always issue relogin requests for a root cluster URI * Check if cluster URI is root cluster URI in login handlers * Pass clusterUri instead of rootClusterUri to the ReAuthenticate dialog * Reserve `root_cluster_uri` field too * Resolve conflicts (cherry picked from commit 282f6f0)
…r URI (#42355) * Always issue per-session MFA requests for a root cluster URI * Always issue relogin requests for a root cluster URI * Check if cluster URI is root cluster URI in login handlers * Pass clusterUri instead of rootClusterUri to the ReAuthenticate dialog * Reserve `root_cluster_uri` field too * Resolve conflicts (cherry picked from commit 282f6f0)
…r URI (#42355) (#42605) * Always issue per-session MFA requests for a root cluster URI * Always issue relogin requests for a root cluster URI * Check if cluster URI is root cluster URI in login handlers * Pass clusterUri instead of rootClusterUri to the ReAuthenticate dialog * Reserve `root_cluster_uri` field too * Resolve conflicts (cherry picked from commit 282f6f0)
…cluster URI (#42606) * Always issue relogin and per-session mfa requests for the root cluster URI (#42355) * Always issue per-session MFA requests for a root cluster URI * Always issue relogin requests for a root cluster URI * Check if cluster URI is root cluster URI in login handlers * Pass clusterUri instead of rootClusterUri to the ReAuthenticate dialog * Reserve `root_cluster_uri` field too * Resolve conflicts (cherry picked from commit 282f6f0) * Remove unnecessary type casts and add imports
I noticed that when we show the relogin dialog for a leaf resource, we say
Log in to <leaf cluster>. I don't think it is correct behavior, since the user should log in to the root cluster, not the leaf one.This doesn't seem to be only a visual thing, because if I continue and log in to that leaf, I get:
So I think we actually logged in to the leaf cluster and then tried to fetch leaf clusters for it.
What's more, we do the same thing for per-session MFA prompts. They say
Verify your identity on <leaf cluster>. Although per-session MFA is controlled by a cluster setting (so it can be disabled on root and enabled on leaf) I think we should still show root cluster name there (and the proto field name suggests that as well). @Joerger, am I correct?