Skip to content

Prompt MFA just once for tctl get tokens#64751

Merged
Joerger merged 1 commit intomasterfrom
joerger/fix-get-tokens-admin-mfa-prompt
Mar 24, 2026
Merged

Prompt MFA just once for tctl get tokens#64751
Joerger merged 1 commit intomasterfrom
joerger/fix-get-tokens-admin-mfa-prompt

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Mar 18, 2026

Changelog: Fix an issue where tctl get tokens would prompt for admin MFA three times rather than once.

tctl get tokens is an edge case in the Admin MFA get requests as the resource is inherently a secret and does not require a WithSecrets option in the rpc. This edge case is now covered in the automated tests.

This edge case bug was dormant before #57772 due to the fact that tokens are the only resource effected. Before v18.1.5, this bug can still be reproduced by calling tctl get tokens,tokens

Manual Test Plan

  • tctl get tokens prompts for MFA once

mfaRequired := rc.withSecrets && slices.ContainsFunc(rc.refs, func(r services.Ref) bool {

withSecrets := rc.withSecrets || slices.ContainsFunc(rc.refs, func(r services.Ref) bool {
// tokens cannot be retrieved without secrets.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be the case even for delegated tokens?

Copy link
Copy Markdown
Contributor

@hugoShaka hugoShaka Mar 20, 2026

Choose a reason for hiding this comment

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

Yes, every token is secret today. Sadly the token kind conflates 3 things:

  • dynamic join tokens
  • user token
  • static tokens

The delegated tokens are an edge case case of the dynamic token. So we must always consider all tokens secret by default. This is what the provision token RPCs are doing, they enforce MFA on read.

On the client-side, tctl has to do the MFA, else it will be forced by the server 3 times instead of once, even for delegated tokens. The "withSecrets" part is not really the point here, we are more interested in the side effect that all secret reads require MFA.

In the future, I'd like to change the handler.RequireMFA() function to take handler.RequireMFA(verb) so we can differentiate between MFA on read (token, CA certificates, user --with-sercets, ...) and MFA on write (roles). However this won't be backported to v18, at least now, so I think @Joerger 's fix is the best thing to do here to fix the issue.

@hugoShaka hugoShaka requested a review from zmb3 March 20, 2026 12:55
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Mar 24, 2026

@kiosion @zmb3 Friendly ping to review

@Joerger Joerger added this pull request to the merge queue Mar 24, 2026
Merged via the queue into master with commit d4c9ed5 Mar 24, 2026
56 of 63 checks passed
@Joerger Joerger deleted the joerger/fix-get-tokens-admin-mfa-prompt branch March 24, 2026 20:24
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@Joerger See the table below for backport results.

Branch Result
branch/v17 Failed
branch/v18 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v18 size/sm tctl tctl - Teleport admin tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants