Skip to content

Make tctl users add prompt for MFA just once#36997

Merged
Joerger merged 5 commits intomasterfrom
joerger/reuse-mfa-tctl-users-add
Jan 23, 2024
Merged

Make tctl users add prompt for MFA just once#36997
Joerger merged 5 commits intomasterfrom
joerger/reuse-mfa-tctl-users-add

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Jan 21, 2024

Updates tctl users add to perform the admin action MFA ceremony upfront, allowing reuse so CreateUser and CreateResetPasswordToken only require one prompt.

Based on #36996

@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Jan 21, 2024
@github-actions github-actions Bot added size/sm tctl tctl - Teleport admin tool labels Jan 21, 2024
Comment thread tool/tctl/common/user_command.go Outdated
user.SetRoles(u.allowedRoles)

// Prompt for admin action MFA if required, allowing reuse for CreateResetPasswordToken.
mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client, "CreateUser", true /*allowReuse*/)
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.

Does this mean users who do not use MFA are going to incur an additional "cost" in the form of one extra call to the cluster in order to get the MFA challenge?

I know that in the past we've been trying to avoid it. I remember Michael dealing with this around file transfer in moderated sessions. But maybe nowadays it's not that big of a deal since we want to encourage people to use MFA anyway.

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.

maybe we can also cache the challenge (chal.MFARequired)?

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.

You're absolutely right. I've updated it to check the cached auth pref to see if MFA is enforced for admin actions.

maybe we can also cache the challenge (chal.MFARequired)?

It's true that even with the change above we will sometimes check if MFA is required when we already know it is/is not. However, I'd like to keep this PR simple and not add a potentially finicky cache system. I'll consider making a follow up for this.

@Joerger Joerger force-pushed the joerger/reuse-mfa-tctl-users-add branch from 2b3715b to b64ab83 Compare January 22, 2024 19:36
@Joerger Joerger force-pushed the joerger/client-mfa-ceremony-refactor branch from 7fede37 to ac2f794 Compare January 22, 2024 19:37
@Joerger Joerger force-pushed the joerger/reuse-mfa-tctl-users-add branch from b64ab83 to 3c4c44f Compare January 22, 2024 19:39
@Joerger Joerger force-pushed the joerger/client-mfa-ceremony-refactor branch from ac2f794 to 98254ee Compare January 22, 2024 20:54
@Joerger Joerger force-pushed the joerger/reuse-mfa-tctl-users-add branch from 3c4c44f to 6f2e6fe Compare January 22, 2024 20:55
Base automatically changed from joerger/client-mfa-ceremony-refactor to master January 23, 2024 01:37
@Joerger Joerger enabled auto-merge January 23, 2024 18:07
@Joerger Joerger added this pull request to the merge queue Jan 23, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 23, 2024
@Joerger Joerger added this pull request to the merge queue Jan 23, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 23, 2024
@Joerger Joerger added this pull request to the merge queue Jan 23, 2024
Merged via the queue into master with commit 85b7d9b Jan 23, 2024
@Joerger Joerger deleted the joerger/reuse-mfa-tctl-users-add branch January 23, 2024 20:09
@public-teleport-github-review-bot
Copy link
Copy Markdown

@Joerger See the table below for backport results.

Branch Result
branch/v15 Failed

@public-teleport-github-review-bot
Copy link
Copy Markdown

@Joerger See the table below for backport results.

Branch Result
branch/v15 Failed

Joerger added a commit that referenced this pull request Jan 24, 2024
* Refactor MFA prompt logic for the API client to enable various MFA
ceremony use cases.

* Address comments.

* Prompt for MFA before creating a user with tctl.

* Check if AdminActionMFA is enforced before attempting to create an MFA auth challenge.
github-merge-queue Bot pushed a commit that referenced this pull request Jan 25, 2024
* Refactor MFA prompt logic for the API client to enable various MFA
ceremony use cases.

* Address comments.

* Prompt for MFA before creating a user with tctl.

* Check if AdminActionMFA is enforced before attempting to create an MFA auth challenge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm tctl tctl - Teleport admin tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants