fix: allow MFA response reuse during tctl acl create/update - #69430
fix: allow MFA response reuse during tctl acl create/update#69430kimlisa wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ce8eb40de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| func withResusableAdminActionMFA(ctx context.Context, client *authclient.Client) (context.Context, error) { | ||
| mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client.PerformMFACeremony, true /*allowReuse*/) | ||
| if err == nil { | ||
| ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse) |
There was a problem hiding this comment.
When admin-action MFA users have only TOTP available, PerformAdminActionMFACeremony(..., allowReuse=true) can still return a TOTP response, but TOTP codes are single-use (checkOTP rejects the previously used token). Storing that response on the shared context makes multi-RPC create flows such as tctl acl create --access-type ... --members ... send the same OTP to both CreateAccessListWithPreset and the follow-up member upsert, so the later admin-protected call fails instead of prompting for a fresh code. Only keep this preflight response for MFA methods that are actually reusable, or clear it before subsequent RPCs that need a new TOTP.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this should be okay, the admin-action is enabled when cluster is configured with second_factor: webauthn so PerformAdminActionMFACeremony will only return a webauthn response
The commands were failing when admin-action is enabled because the API call performed multiple admin-protected writes (access_list and role).
1ce8eb4 to
9a1d39e
Compare
| })) | ||
| } | ||
|
|
||
| func withReusableAdminActionMFA(ctx context.Context, client *authclient.Client) (context.Context, error) { |
There was a problem hiding this comment.
Feels like this withReusableAdminActionMFA is something the MFA package should expose rather than having to hand roll at every call site.
There was a problem hiding this comment.
yeah i see it hand rolled in quite a few places... next time 😅
kozadaev-tp
left a comment
kozadaev-tp
left a comment
There was a problem hiding this comment.
LGTM - minor readability nit (optional)
| func withReusableAdminActionMFA(ctx context.Context, client *authclient.Client) (context.Context, error) { | ||
| mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client.PerformMFACeremony, true /*allowReuse*/) | ||
| if err == nil { | ||
| ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse) | ||
| } else if !errors.Is(err, &mfa.ErrMFANotRequired) && !errors.Is(err, &mfa.ErrMFANotSupported) { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| return ctx, nil | ||
| } |
There was a problem hiding this comment.
[nit] I'd refactor the error handling for better readability. Feel free to ignore though.
| func withReusableAdminActionMFA(ctx context.Context, client *authclient.Client) (context.Context, error) { | |
| mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client.PerformMFACeremony, true /*allowReuse*/) | |
| if err == nil { | |
| ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse) | |
| } else if !errors.Is(err, &mfa.ErrMFANotRequired) && !errors.Is(err, &mfa.ErrMFANotSupported) { | |
| return nil, trace.Wrap(err) | |
| } | |
| return ctx, nil | |
| } | |
| func withReusableAdminActionMFA(ctx context.Context, client *authclient.Client) (context.Context, error) { | |
| mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client.PerformMFACeremony, true /*allowReuse*/) | |
| if err != nil { | |
| if errors.Is(err, &mfa.ErrMFANotRequired) || errors.Is(err, &mfa.ErrMFANotSupported) { | |
| return ctx, nil | |
| } | |
| return nil, trace.Wrap(err) | |
| } | |
| return mfa.ContextWithMFAResponse(ctx, mfaResponse), nil | |
| } |
4 participants
The commands were failing (see error below) when admin-action is enabled because the API call performed multiple admin-protected writes (access_list and role).
... ERROR: access denied mfa session data not foundChangelog: Fixed
tctl acl createandtctl acl updatefailures when admin-action MFA is enabled.Manual Test Plan
Test Environment
local - my user has both webauthn and otp registered
Test Cases
with admin-action enabled
without admin-action enabled