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.

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

Add support for MFA for DB access #8270

Merged
merged 18 commits into from
Oct 6, 2021
Merged

Add support for MFA for DB access #8270

merged 18 commits into from
Oct 6, 2021

Conversation

smallinsky
Copy link
Contributor

@smallinsky smallinsky commented Sep 15, 2021

What

Introduce MFA per db connection.

UX Changes:

  • Add support for MFA if user role db permission requires require_session_mfa
    • db connect
    tsh db connect proddb
    Tap any security key
    
    • db login
     tsh db login proddb
     Tap any security key
    
  • tsh db connect databasename right now will call under the hood the databaseLogin function if db cert expired or if is missing. The tsh db login databasenama call is superflous if user wants to connect to the database using predefine db cli client binaries like postgresBin mysqlBin
  • removed fetching db credentails for active databases during tsh db ls command.
  • removed fetching db credentails for active databases during tsh login command.

@smallinsky smallinsky requested a review from r0mant September 15, 2021 13:51
@smallinsky smallinsky marked this pull request as ready for review September 20, 2021 08:04
@smallinsky smallinsky requested a review from Joerger September 20, 2021 14:21
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Have some code org suggestions. Also, can we test this somehow? We can't probably test real 2fa in tests but at least the business logic of requiring 2nd factor we should be able to.

lib/auth/auth.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
tool/tsh/db.go Outdated Show resolved Hide resolved
lib/auth/auth.go Outdated Show resolved Hide resolved
Comment on lines +603 to +607
// If the cert expiration time is less than 5s consider cert as expired and don't add
// it to the user profile as an active database.
if time.Until(cert.NotAfter) < 5*time.Second {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be fine but seems a little "arbitrary" TBH. What's the reasoning for this? Ideally, the comment should explain not "what" but "why".

lib/auth/auth_with_roles_test.go Outdated Show resolved Hide resolved
lib/auth/auth_with_roles_test.go Outdated Show resolved Hide resolved
lib/auth/auth_with_roles_test.go Outdated Show resolved Hide resolved
lib/auth/auth_with_roles_test.go Outdated Show resolved Hide resolved
lib/auth/auth_with_roles_test.go Outdated Show resolved Hide resolved
@smallinsky
Copy link
Contributor Author

@klizhentas Can I ask for UX changes review ?
The most significant change from the user perspective is:

tsh db connect databasename right now will call under the hood the databaseLogin function if db cert expired or if is missing. The tsh db login databasenama call is superflous if user wants to connect to the database using predefine db cli client binaries like postgresBin mysqlBin

So a user can just connect to a DB tsh db connect databasename without even login to it.

Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

@r0mant r0mant enabled auto-merge (squash) October 6, 2021 18:51
@r0mant r0mant merged commit 700f9f7 into master Oct 6, 2021
@r0mant r0mant deleted the smallinsky/dbmfa branch October 6, 2021 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants