Skip to content

fix tsh db connect with hardware backed key#20806

Merged
GavinFrazar merged 4 commits intomasterfrom
gavinfrazar/fix-tsh-db-connect-with-piv
Jan 30, 2023
Merged

fix tsh db connect with hardware backed key#20806
GavinFrazar merged 4 commits intomasterfrom
gavinfrazar/fix-tsh-db-connect-with-piv

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar commented Jan 27, 2023

Fixes #20799

Leaving this in draft until I've added test cases, but from manually trying out the fix it seems to have done the trick.

Test by building with PIV=yes and enabling PIV with require_session_mfa: hardware_key # or hardware_key_touch and then connecting to a database. The issue describes configurations with the problem.

@Joerger Joerger mentioned this pull request Jan 27, 2023
Merged
@GavinFrazar GavinFrazar marked this pull request as ready for review January 28, 2023 00:47
@github-actions github-actions Bot added size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Jan 28, 2023
@GavinFrazar
Copy link
Copy Markdown
Contributor Author

GavinFrazar commented Jan 28, 2023

I added test cases for login or whenever a new client is created. This way if, for example, the user has an old profile (such that TeleportClient is created before trying to use their private key), then login should always update TeleportClient.PrivateKeyPolicy.

I also verified that the following scenarios are handled:

  1. tsh db connect --user=alice --db-user=foo --db-name=bar somedb will trigger a login flow if the user has not done tsh login before and will detect the key policy
  2. tsh db connect.. will trigger relogin and detect key policy if the profile has expired
  3. tsh db connect ... will always setup a proxy tunnel if hardware key policy is in effect

tsh db connect should now work with PIV regardless of config/protocol.

codingllama
codingllama approved these changes Jan 30, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer having a the test separate from TestTeleportClient_Login_local, but LGTM.

Edit: not applicable anymore, LGTM.

Comment thread lib/client/api_login_test.go Outdated
Comment thread tool/tsh/db.go Outdated
Comment thread tool/tsh/db.go Outdated
Comment thread tool/tsh/proxy.go Outdated
Joerger
Joerger reviewed Jan 30, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Joerger Joerger left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread lib/client/api.go Outdated
Comment thread lib/client/api.go Outdated
@GavinFrazar GavinFrazar enabled auto-merge January 30, 2023 21:15
@GavinFrazar GavinFrazar disabled auto-merge January 30, 2023 21:20
@GavinFrazar GavinFrazar enabled auto-merge January 30, 2023 21:20
@GavinFrazar GavinFrazar added this pull request to the merge queue Jan 30, 2023
@GavinFrazar GavinFrazar removed this pull request from the merge queue due to a manual request Jan 30, 2023
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/fix-tsh-db-connect-with-piv branch from da2a86c to bcb81fc Compare January 30, 2023 21:40
@GavinFrazar GavinFrazar enabled auto-merge January 30, 2023 21:40
@GavinFrazar GavinFrazar disabled auto-merge January 30, 2023 21:54
@GavinFrazar GavinFrazar enabled auto-merge January 30, 2023 21:54
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/fix-tsh-db-connect-with-piv branch from bcb81fc to 893ff07 Compare January 30, 2023 21:55
@GavinFrazar GavinFrazar disabled auto-merge January 30, 2023 23:15
@GavinFrazar GavinFrazar enabled auto-merge January 30, 2023 23:15
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/fix-tsh-db-connect-with-piv branch from 893ff07 to 439688b Compare January 30, 2023 23:16
@GavinFrazar GavinFrazar added this pull request to the merge queue Jan 30, 2023
Merged via the queue into master with commit 3df617d Jan 30, 2023
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Jan 30, 2023

@GavinFrazar See the table below for backport results.

Branch Result
branch/v11 Failed
branch/v12 Create PR

@GavinFrazar GavinFrazar mentioned this pull request Jan 31, 2023
Merged
@GavinFrazar GavinFrazar deleted the gavinfrazar/fix-tsh-db-connect-with-piv branch January 31, 2023 02:38
GavinFrazar added a commit that referenced this pull request Jan 31, 2023
@GavinFrazar
fix tsh db connect with hardware backed key (#20806)
14e013f 
* Check key policy in tsh db connect flow
* Load key policy from identity file
@GavinFrazar GavinFrazar mentioned this pull request Jan 31, 2023
Merged
GavinFrazar added a commit that referenced this pull request Feb 4, 2023
@GavinFrazar
fix tsh db connect with hardware backed key (#20806)
0695125 
* Check key policy in tsh db connect flow
* Load key policy from identity file
GavinFrazar added a commit that referenced this pull request Feb 4, 2023
@GavinFrazar
fix tsh db connect with hardware backed key (#20806) (#21042)
a6dc7c2 
* Check key policy in tsh db connect flow
* Load key policy from identity file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codingllama codingllama codingllama approved these changes

@Joerger Joerger Joerger approved these changes

Assignees

No one assigned

Labels

size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tsh db connect fails when PIV is enabled

3 participants

@GavinFrazar @codingllama @Joerger