Skip to content

MFA for Admin Actions: GenerateUserCerts#35662

Merged
Joerger merged 10 commits intomasterfrom
joerger/admin-actions-generate-certs
Dec 22, 2023
Merged

MFA for Admin Actions: GenerateUserCerts#35662
Joerger merged 10 commits intomasterfrom
joerger/admin-actions-generate-certs

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Dec 12, 2023

Require MFA for GenerateUserCerts (tctl auth sign).

Part of RFD 131.

Based off #35386 to use the same test helpers.

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Dec 12, 2023
@github-actions github-actions Bot requested review from Tener and r0mant December 12, 2023 18:53
@github-actions github-actions Bot added size/sm tctl tctl - Teleport admin tool labels Dec 12, 2023
@Joerger Joerger force-pushed the joerger/admin-actions-generate-certs branch from f05f0f1 to 65d7f53 Compare December 12, 2023 18:55
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from r0mant December 13, 2023 07:57
Base automatically changed from joerger/admin-actions-users to master December 14, 2023 21:59
@Joerger Joerger force-pushed the joerger/admin-actions-generate-certs branch from 65d7f53 to 4ba5b94 Compare December 19, 2023 01:38
@Joerger Joerger enabled auto-merge December 19, 2023 20:19
@Joerger Joerger force-pushed the joerger/admin-actions-generate-certs branch 2 times, most recently from e6618aa to d21880a Compare December 20, 2023 02:07
@Joerger Joerger disabled auto-merge December 20, 2023 02:08
@Joerger Joerger requested review from Tener and rosstimothy December 20, 2023 02:08
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Dec 20, 2023

@rosstimothy @Tener I made some changes to help unit tests pass and to make GenerateUserCerts not require admin MFA when the request already includes an MFA response. I plan on merging this tomorrow but wanted to give you guys a chance to do a quick re-review.

Comment thread lib/auth/auth_with_roles.go Outdated
@Joerger Joerger requested a review from Tener December 21, 2023 03:10
@Joerger Joerger force-pushed the joerger/admin-actions-generate-certs branch from 3d5bcda to 27a24e4 Compare December 21, 2023 03:38
@Joerger Joerger enabled auto-merge December 21, 2023 17:20
@Joerger Joerger disabled auto-merge December 21, 2023 21:41
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Dec 21, 2023

A new test from Teleport Connect was failing in this PR, I found it was because MFA was being required for cert renewal, which is used in Teleport Connect Kube and Database "gateway" logic. There was also an issue with the wrong MFA prompt getting propagated to the API client. I fixed both issues in the latest 2 commits.

Notably, I decided to only require admin MFA for non-renewal cert generation requests, which encompasses both normal renewal and MFA verified reissue. So MFA will only be required for impersonation as intended. This will match the behavior of Web sessions, since we do not want to require MFA for frequent renewals.

Thanks @ravicious for your thorough Teleport Connect tests, I wouldn't have caught this issue otherwise.

Reviewers, I plan to merge this EOD as it's my last day before going OOO, but please open an issue or comment on this thread if you'd like any changes.

@Joerger Joerger enabled auto-merge December 21, 2023 22:00
@Joerger Joerger disabled auto-merge December 21, 2023 22:00
@Joerger Joerger force-pushed the joerger/admin-actions-generate-certs branch from 3b84c22 to 071b567 Compare December 22, 2023 01:36
@Joerger Joerger enabled auto-merge December 22, 2023 01:37
@Joerger Joerger added this pull request to the merge queue Dec 22, 2023
Merged via the queue into master with commit 7a98dcb Dec 22, 2023
@Joerger Joerger deleted the joerger/admin-actions-generate-certs branch December 22, 2023 02:16
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