Skip to content

MFA for admin actions: Add MFA prompt to API client (tctl and tsh)#30203

Merged
Joerger merged 1 commit intomasterfrom
joerger/admin-mfa-client
Aug 25, 2023
Merged

MFA for admin actions: Add MFA prompt to API client (tctl and tsh)#30203
Joerger merged 1 commit intomasterfrom
joerger/admin-mfa-client

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Aug 9, 2023

This PR updates the API client to prompt for MFA for admin actions. This can be done either by passing the call option directly, or retrying after getting the relevant access denied error.

Part of implementing RFD 131.

In follow up PRs, Admin Action API requests will be updated to

  • Client side: provide MFA through the call option when we know MFA is required
  • Server side: verify the MFA passed in the request context.
    • MFA will not be required until we flip the switch in Teleport 15, but Teleport 14 clients will be able to pass the requirement with the retry mechanism added in this PR.

This is used in tctl, tsh, and Teleport Connect, though Teleport Connect does not show a modal yet.

WebSessions do not use this retry mechanism. I follow up this PR with a solution for the WebUI if I find a way to do it.

Depends on #30578

@github-actions github-actions Bot added desktop-access size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Aug 9, 2023
@github-actions github-actions Bot requested review from codingllama and gzdunek August 9, 2023 00:00
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Reviewed commit-by-commit, so some comments may be repeated (or already addressed).

First of all thanks for the refactor, moving client-MFA to its own package is a good idea.

I think overall the PR is too big, it would be easier to review a smaller, more gradual package move, with changes separate in their own PRs. If you want to break it up, given the number of comments, it might be a good idea. (You can preemptively address the comments in future PRs.)

Comment thread lib/auth/mfa/export.go Outdated
Comment thread lib/auth/mfa/export.go
Comment thread lib/auth/mfa/mfa.go
Comment thread lib/auth/mfa/export.go Outdated
Comment thread lib/client/api.go Outdated
Comment thread api/client/mock_server_test.go Outdated
Comment thread api/client/mock_server_test.go Outdated
Comment thread lib/auth/clt.go Outdated
Comment thread api/client/client.go Outdated
Comment thread e
@codingllama
Copy link
Copy Markdown
Contributor

One more reason to break the PR is so we can backport the package move, but not the admin changes. If we don't backport the move, any eventual fixes in older code will be nasty to get to old releases.

@Joerger Joerger force-pushed the joerger/admin-mfa-client branch from 23668a4 to be6d7d5 Compare August 17, 2023 00:34
@Joerger Joerger changed the base branch from master to joerger/admin-mfa-client-base August 17, 2023 00:34
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

This is WAY simpler to review now, I didn't even recognize it at first. Many thanks for all the refactors.

Comment thread api/client/mfa.go Outdated
Comment thread api/client/mfa.go Outdated
Comment thread api/metadata/mfa.go Outdated
Comment thread api/metadata/mfa.go Outdated
Comment thread api/metadata/mfa.go Outdated
Comment thread lib/auth/clt.go Outdated
Comment thread api/client/mfa_test.go Outdated
Comment thread api/client/mfa_test.go Outdated
Comment thread api/client/mfa_test.go Outdated
@Joerger Joerger force-pushed the joerger/admin-mfa-client-base branch 2 times, most recently from a88ceaf to e74b3e2 Compare August 18, 2023 01:26
@Joerger Joerger force-pushed the joerger/admin-mfa-client branch 3 times, most recently from bac92d3 to ef11149 Compare August 18, 2023 02:53
Comment thread api/mfa/mfa.go Outdated
Comment thread api/mfa/mfa.go Outdated
Comment thread api/mfa/mfa.go Outdated
Comment thread api/mfa/mfa_test.go Outdated
Comment thread api/mfa/mfa_test.go Outdated
Comment thread api/utils/grpc/interceptors/mfa_test.go Outdated
Comment thread api/utils/grpc/interceptors/mfa_test.go Outdated
Comment thread api/utils/grpc/interceptors/mfa_test.go Outdated
Comment thread api/mfa/mfa_test.go Outdated
Comment thread api/client/client.go Outdated
@Joerger Joerger force-pushed the joerger/admin-mfa-client-base branch from e74b3e2 to 5d3ab0d Compare August 18, 2023 20:21
@Joerger Joerger force-pushed the joerger/admin-mfa-client branch from ef11149 to 22d2582 Compare August 18, 2023 20:33
Comment thread api/client/client.go Outdated
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.

Do we really want any and every client created to have this interceptor or just the tctl client? Should the client used by a node/app/db/kube/etc service have this interceptor included?

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.

I'd say yes, why would we not want it?

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.

How is the Proxy supposed to perform MFA?

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.

Oh sorry, I was thinking about the wrong PR. Ignore me.

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.

I've changed this so it is only used for tsh, tctl, and Teleport Connect.

The interceptor will always be set, but it'll essentially be a noop unless client.PromptAdminRequestMFA is set.

To be specific, it will aggregate trace.BadParameter("missing PromptAdminRequestMFA field, client cannot perform MFA ceremony") with the ErrAdminActionMFARequired passed through the interceptor.

@codingllama you mentioned before that you weren't sold on enabling/disabling the interceptor in this way, but I think it makes more sense. If the client has a method to prompt MFA, it should. If it doesn't, it shouldn't. Adding a separate flag seems redundant.

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.

How difficult would it be to create certain clients without the interceptor?

If you think it isn't worth it, or a worse solution, we can go with the nil. Thanks for calling attention to it explicitly.

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.

I think having the aggregated error is slightly better as it can help point us in the right direction with any bugs, such as if a client does not have PromptAdminRequestMFA set when we expect it to. Also adding the mfa-retry interceptor into the middle of list after the fact is a bit awkward from a code formatting perspective.

@Joerger Joerger force-pushed the joerger/admin-mfa-client branch 2 times, most recently from 5b44fe1 to 5337fc4 Compare August 22, 2023 19:48
@Joerger Joerger changed the base branch from joerger/admin-mfa-client-base to joerger/grpc-error-interceptor August 22, 2023 19:48
Comment thread api/mfa/mfa.go Outdated
Comment thread lib/client/cluster_client.go Outdated
Comment thread lib/client/mfa/prompt.go Outdated
Comment thread lib/auth/authclient/authclient.go Outdated
@Joerger Joerger requested a review from rosstimothy August 24, 2023 19:27
@Joerger Joerger force-pushed the joerger/grpc-error-interceptor branch from e0e0490 to 5f869dc Compare August 24, 2023 23:10
@Joerger Joerger force-pushed the joerger/admin-mfa-client branch from 2cd7148 to 93e7761 Compare August 24, 2023 23:23
Base automatically changed from joerger/grpc-error-interceptor to master August 24, 2023 23:54
@Joerger Joerger force-pushed the joerger/admin-mfa-client branch from 93e7761 to aa9103a Compare August 25, 2023 00:42
Comment thread lib/auth/authclient/authclient.go Outdated
Comment thread lib/auth/authclient/authclient.go Outdated
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from gzdunek August 25, 2023 13:01
@Joerger Joerger force-pushed the joerger/admin-mfa-client branch from aa9103a to 343d6d5 Compare August 25, 2023 17:13
@Joerger Joerger enabled auto-merge August 25, 2023 17:13
* Add MFA PerRPCredentials.

* Add RetryWithMFA unary interceptor.

* Add MFA prompt retry mechanism for Admin API requests.
@Joerger Joerger force-pushed the joerger/admin-mfa-client branch from aec9265 to 1b07967 Compare August 25, 2023 18:57
@Joerger Joerger added this pull request to the merge queue Aug 25, 2023
Merged via the queue into master with commit 9b29150 Aug 25, 2023
@Joerger Joerger deleted the joerger/admin-mfa-client branch August 25, 2023 19:31
@Joerger Joerger changed the title Add MFA prompt to API client for Admin Actions (tctl and tsh) MFA for Admin Actions: Add MFA prompt to API client (tctl and tsh) Oct 20, 2023
@Joerger Joerger changed the title MFA for Admin Actions: Add MFA prompt to API client (tctl and tsh) MFA for admin actions: Add MFA prompt to API client (tctl and tsh) Oct 20, 2023
@Joerger Joerger mentioned this pull request Oct 20, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants