Skip to content

Add draft for RFD/0090 - DB MFA Sessions#16739

Merged
GavinFrazar merged 13 commits into
masterfrom
rfd/0090-db-mfa-sessions
Feb 3, 2023
Merged

Add draft for RFD/0090 - DB MFA Sessions#16739
GavinFrazar merged 13 commits into
masterfrom
rfd/0090-db-mfa-sessions

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar commented Sep 26, 2022

This RFD describes changes proposed to support per-session-mfa support with tsh proxy db --tunnel (which is necessary for use with GUI clients, so effectively this RFD is to support per-session-mfa with GUI clients).

Relevant issue: #12538

@GavinFrazar GavinFrazar added the rfd Request for Discussion label Sep 26, 2022
@GavinFrazar GavinFrazar marked this pull request as ready for review September 26, 2022 22:38
@github-actions github-actions Bot requested a review from Tener September 26, 2022 22:39
Remove extra line breaks
Comment thread rfd/0090-db-mfa-sessions.md
Comment thread rfd/0090-db-mfa-sessions.md Outdated
Comment thread rfd/0090-db-mfa-sessions.md Outdated
Comment thread rfd/0090-db-mfa-sessions.md
## Details
When per-session-MFA is enabled, we should not restrict database cert TTL to 1 minute.

Instead, database cert TTL should be restricted to `max_session_ttl`, and the cert
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that using current default behavior where cert expiration time is set based on max_session_ttl is reasonable. Alternatively the DB MFA certs expiration time can be unset allowing to run tsh proxy db forever without second MFA prompt though we will ignore basic teleport cluster restriction in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

max_session_ttl allows the user to change it as they like, although we have a maximum cert duration limit of 30 hours (defined in api/defaults/defaults.go). As long as it isn't excessively asking for MFA, I think that's fine.

I'm more concerned about how the user will be notified of the prompt, since the proxy just runs as a foreground terminal process. My yubikey blinks green when it's ready for a tap, so this hasn't been an issue for me, but whether the user gets a system notification or just a line printed in the terminal with no notification I think is up to their personal system's configuration. I think TeleportConnect will solve this? @ravicious

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, in Connect when the tsh daemon process becomes aware of this, it can notify the Electron app about it and then the app can just bring its window to the top. This is how we decided to solve it for now, not only for per session MFA but also for things like trying to connect to a proxy after your cert has expired.

Copy link
Copy Markdown
Contributor

@xinding33 xinding33 left a comment

Choose a reason for hiding this comment

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

This is a clear improvement to user experience so no objections from me. The biggest open question is definitely security, so as long as the platform security team is ok with the changes, then I'm a +1.

@GavinFrazar
Copy link
Copy Markdown
Contributor Author

@r0mant @klizhentas can you guys take a look? Marek suggested I put you two down as required approvers.

Comment thread rfd/0090-db-mfa-sessions.md
Comment thread rfd/0090-db-mfa-sessions.md Outdated
Comment thread rfd/0090-db-mfa-sessions.md
Comment thread rfd/0090-db-mfa-sessions.md
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Oct 18, 2022

See also #7568 which is a similar request but for k8s sessions instead of DB sessions.

@GavinFrazar
Copy link
Copy Markdown
Contributor Author

See also #7568 which is a similar request but for k8s sessions instead of DB sessions.

While it is a similar request, we don't have an equivalent to tsh proxy db for kube access. I think we should have a separate RFD for solving that issue.

Comment thread rfd/0090-db-mfa-sessions.md
Comment thread rfd/0090-db-mfa-sessions.md
@GavinFrazar GavinFrazar enabled auto-merge February 3, 2023 01:10
@GavinFrazar GavinFrazar added this pull request to the merge queue Feb 3, 2023
Merged via the queue into master with commit 99e9db8 Feb 3, 2023
@GavinFrazar GavinFrazar deleted the rfd/0090-db-mfa-sessions branch February 3, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rfd Request for Discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants