Skip to content

Add per-session mfa support to connection testers#22528

Merged
kimlisa merged 4 commits intomasterfrom
lisa/mfa-check-conntest
Mar 9, 2023
Merged

Add per-session mfa support to connection testers#22528
kimlisa merged 4 commits intomasterfrom
lisa/mfa-check-conntest

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented Mar 2, 2023

part of #16702

Description

Implements the backend part of RFD 0111

  • Adds a MFAAuthenticateResponse field to GenerateUserCerts request
  • Define a new field for test connection request that accepts MFA response
  • Adds a new web endpoint that allows checking if target resource requires MFA

@github-actions github-actions Bot requested review from codingllama and hatched March 2, 2023 04:02
@kimlisa kimlisa requested review from marcoandredinis and removed request for hatched March 2, 2023 04:04
@kimlisa kimlisa force-pushed the lisa/mfa-check-conntest branch 2 times, most recently from 486ecd4 to ba7c8b3 Compare March 2, 2023 04:09
Comment thread lib/client/conntest/connection_tester.go Outdated
Comment thread lib/client/conntest/connection_tester.go Outdated
Comment thread lib/client/conntest/connection_tester.go Outdated
Comment thread lib/client/conntest/connection_tester.go Outdated
Comment thread lib/client/conntest/connection_tester.go Outdated
Comment thread lib/auth/auth_with_roles_test.go Outdated
Comment thread lib/auth/auth_with_roles_test.go Outdated
Comment thread lib/web/apiserver_test.go Outdated
Comment thread lib/web/apiserver_test.go Outdated
Comment thread lib/web/apiserver_test.go Outdated
@codingllama
Copy link
Copy Markdown
Contributor

Thanks for the cleanly-split backend PR.

Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

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

LGTM after Alan's comments
I've just tested (from the UI Branch) and looks to be working 👍
image

Comment thread lib/web/mfa.go Outdated
Comment thread lib/auth/auth_with_roles_test.go Outdated
"github.com/gravitational/teleport/lib/session"
"github.com/gravitational/teleport/lib/tlsca"

"github.com/pquerna/otp/totp"
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.

You'll probably need to move this import to the 2nd group (or run make fix-imports)

Comment thread lib/web/apiserver.go Outdated
@kimlisa kimlisa force-pushed the lisa/mfa-check-conntest branch from dcd17b5 to 037ea12 Compare March 6, 2023 21:58
Comment thread lib/auth/auth_with_roles_test.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.

FYI, I tend to use wantErr string in tests like this, so the name was alright IMO. (No need to change it though.)

@kimlisa kimlisa force-pushed the lisa/mfa-check-conntest branch from cff9f2a to d01349f Compare March 8, 2023 08:30
@kimlisa kimlisa force-pushed the lisa/mfa-check-conntest branch from d01349f to f035473 Compare March 9, 2023 19:58
@kimlisa kimlisa enabled auto-merge March 9, 2023 19:58
@kimlisa kimlisa added this pull request to the merge queue Mar 9, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 9, 2023
@kimlisa kimlisa added this pull request to the merge queue Mar 9, 2023
Merged via the queue into master with commit 98230bb Mar 9, 2023
@public-teleport-github-review-bot
Copy link
Copy Markdown

@kimlisa See the table below for backport results.

Branch Result
branch/v10 Failed
branch/v11 Failed
branch/v12 Failed

kimlisa added a commit that referenced this pull request Mar 10, 2023
* Add MFAAuthenticateResponse field to GenerateUserCerts

* Pass MFA response for connection testers

* Add checking MFA web endpoint

* Address CRs
kimlisa added a commit that referenced this pull request Mar 10, 2023
* Add MFAAuthenticateResponse field to GenerateUserCerts

* Pass MFA response for connection testers

* Add checking MFA web endpoint

* Address CRs
kimlisa added a commit that referenced this pull request Mar 10, 2023
* Add MFAAuthenticateResponse field to GenerateUserCerts

* Pass MFA response for connection testers

* Add checking MFA web endpoint

* Address CRs
kimlisa added a commit that referenced this pull request Mar 10, 2023
)

* Add per-session mfa support to connection testers (#22528)

* Add MFAAuthenticateResponse field to GenerateUserCerts

* Pass MFA response for connection testers

* Add checking MFA web endpoint

* Address CRs

* Fix lint: all cap acronym
kimlisa added a commit that referenced this pull request Mar 10, 2023
* Add per-session mfa support to connection testers (#22528)

* Add MFAAuthenticateResponse field to GenerateUserCerts

* Pass MFA response for connection testers

* Add checking MFA web endpoint

* Address CRs

* Fix lint: all cap acronym
kimlisa added a commit that referenced this pull request Mar 10, 2023
)

* Add per-session mfa support to connection testers (#22528)

* Add MFAAuthenticateResponse field to GenerateUserCerts

* Pass MFA response for connection testers

* Add checking MFA web endpoint

* Address CRs

* Use legacy field (new fields doesn't exist)

* Fix lint: all cap acronym
@r0mant r0mant deleted the lisa/mfa-check-conntest branch March 13, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants