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

Jump to bottom

MFA for App Access - tsh for cloud apps #40985

Merged
merged 24 commits into from
Jun 20, 2024
Merged

MFA for App Access - tsh for cloud apps #40985

merged 24 commits into from
Jun 20, 2024

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Apr 28, 2024 •
edited
Loading

Adds MFA for Cloud App access support with:

  1. extended TTL mfa-verified certs for proxy requests
  2. Automatic certificate renewal
  3. Automatic local CA renewal

(2) and (3) are important for users with a low max_session_ttl setting, which is common in per-session MFA setups.

Many refactors were required to obtain feature parity with tsh proxy app and avoid duplicated logins and other accumulated cruft in the previous implementation.

TODO:

  • Enable the reuse of 1-minute TTL certs for flows like tsh apps login awsconsole && tsh aws ... to avoid re-logins
  • Manual testing (before merging)
    • AWS
    • Azure
    • GCP

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Apr 28, 2024
@Joerger Joerger mentioned this pull request Apr 29, 2024
Closed
@Joerger Joerger force-pushed the joerger/app-access-mfa-tsh-proxy branch 3 times, most recently from 1d23890 to 183df95 Compare May 2, 2024 23:40
Base automatically changed from joerger/app-access-mfa-tsh-proxy to master May 6, 2024 19:46
@Joerger Joerger force-pushed the joerger/app-access-mfa-cloud branch 3 times, most recently from c142f56 to d7cceca Compare May 21, 2024 21:07
@Joerger Joerger marked this pull request as ready for review May 22, 2024 00:33
@github-actions github-actions bot requested review from strideynet and tcsc May 22, 2024 00:33
@github-actions github-actions bot added machine-id size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels May 22, 2024
@Joerger Joerger force-pushed the joerger/app-access-mfa-cloud branch from 4466b09 to bfbc259 Compare May 22, 2024 00:36
@Joerger Joerger requested a review from GavinFrazar May 22, 2024 00:36
@Joerger Joerger force-pushed the joerger/app-access-mfa-cloud branch 2 times, most recently from f7a443e to 5f03cc4 Compare May 22, 2024 00:53
@Joerger Joerger force-pushed the joerger/app-access-mfa-cloud branch from 5f03cc4 to fef5471 Compare May 22, 2024 01:24
rosstimothy
rosstimothy reviewed Jun 12, 2024
View reviewed changes
lib/client/client_store.go Outdated Show resolved Hide resolved
lib/client/client_store.go Outdated Show resolved Hide resolved
lib/client/local_proxy_middleware.go Outdated Show resolved Hide resolved
lib/client/local_proxy_middleware.go Outdated Show resolved Hide resolved
lib/client/local_proxy_middleware.go
Comment on lines +430 to +431
// Requests to IPs have no server names. Default to CA.
r.certsByHost[""] = &caTLSCert
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the blank host correlate to an IP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the TLS ServerName SNI as the name of the host, so the host name for all IP addresses is "". This is preserved from CertGenListener from which most of this logic is derived.

lib/client/local_proxy_middleware.go Show resolved Hide resolved
tool/tsh/common/app_local_proxy.go Outdated Show resolved Hide resolved
tool/tsh/common/app_local_proxy.go Outdated Show resolved Hide resolved
@Joerger
Restore RetryWithRelogin for app commands by making missing profile s…
2bd62ae 
…tatus a retryable error; cache profile status in appInfo.
@Joerger Joerger force-pushed the joerger/app-access-mfa-cloud branch from 4d3dc78 to 2bd62ae Compare June 12, 2024 20:23
@Joerger Joerger requested a review from rosstimothy June 13, 2024 00:49
@Joerger Joerger force-pushed the joerger/app-access-mfa-cloud branch from 6438bb6 to 24f4130 Compare June 13, 2024 18:44
@Joerger
Copy link
Contributor Author

Joerger commented Jun 20, 2024

@zmb3 Can I get a flaky test skip?

rosstimothy
rosstimothy approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

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

/excludeflake *

@Joerger Joerger enabled auto-merge June 20, 2024 19:20
@rosstimothy
Copy link
Contributor

/excludeflake *

@Joerger Joerger added this pull request to the merge queue Jun 20, 2024
Merged via the queue into master with commit 800e8e8 Jun 20, 2024
40 of 41 checks passed
@Joerger Joerger deleted the joerger/app-access-mfa-cloud branch June 20, 2024 20:51
@public-teleport-github-review-bot
Copy link

@Joerger See the table below for backport results.

Branch Result
branch/v16 Failed

Joerger added a commit that referenced this pull request Jun 21, 2024
@Joerger
MFA for App Access - tsh for cloud apps (#40985)
4ac28ff 
* Add generic app proxy implementation.

* Incorporate forwarding proxy in generic app proxy.

* Utilize cert checker middleware for http local proxies.

* Use generalized local proxy app for aws.

* Use generalized local proxy app for azure.

* Use generalized local proxy app for gcp.

* Refactor, simplify, and unify app logic logic for app, proxy app, and
cloud commands.

* * Load existing app certs before reissuing new certs for proxy app
commands

* Fix the same intended functionality for tsh proxy db

* Fix hardware key test for tsh app login.

* Remove unused code.

* Fix merge conflict.

* Address Gavin's comments.

* Restore RetryWithRelogin for app commands by making missing profile status a retryable error; cache profile status in appInfo.

* Address Tim's comments.

* Remove read lock.

* Add tests to CertChecker and LocalCertGenerator.

* Fix lint.
@Joerger Joerger mentioned this pull request Jun 21, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 21, 2024
@Joerger
MFA for App Access - tsh for cloud apps (#40985) (#43334)
2520a57 
* Add generic app proxy implementation.

* Incorporate forwarding proxy in generic app proxy.

* Utilize cert checker middleware for http local proxies.

* Use generalized local proxy app for aws.

* Use generalized local proxy app for azure.

* Use generalized local proxy app for gcp.

* Refactor, simplify, and unify app logic logic for app, proxy app, and
cloud commands.

* * Load existing app certs before reissuing new certs for proxy app
commands

* Fix the same intended functionality for tsh proxy db

* Fix hardware key test for tsh app login.

* Remove unused code.

* Fix merge conflict.

* Address Gavin's comments.

* Restore RetryWithRelogin for app commands by making missing profile status a retryable error; cache profile status in appInfo.

* Address Tim's comments.

* Remove read lock.

* Add tests to CertChecker and LocalCertGenerator.

* Fix lint.
@portswigger-tim portswigger-tim mentioned this pull request Jul 4, 2024
Closed
This was referenced Jul 18, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosstimothy rosstimothy rosstimothy approved these changes

@GavinFrazar GavinFrazar GavinFrazar approved these changes

Assignees
No one assigned
Labels
backport/branch/v16 machine-id no-changelog Indicates that a PR does not require a changelog entry 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.
3 participants
@Joerger @rosstimothy @GavinFrazar