Skip to content

RequireMFAType#16034

Merged
Joerger merged 7 commits into
masterfrom
joerger/require-mfa-type
Sep 22, 2022
Merged

RequireMFAType#16034
Joerger merged 7 commits into
masterfrom
joerger/require-mfa-type

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Sep 1, 2022 •
edited
Loading

This PR is a prerequisite for #15874, which will actually make use of the new require mfa types.

Changes:

role.options.require_session_mfa and auth_preference.require_session_mfa have been changed from booleans to a protobuf enum type - types.RequireMFAType - with the following values.

  • OFF: corresponds to require_session_mfa: false
  • SESSION: corresponds to require_session_mfa: true
  • SESSION_AND_HARDWARE_KEY: corresponds to require_session_mfa: hardware_key
  • HARDWARE_KEY_TOUCH: corresponds to require_session_mfa: hardware_key_touch
    • This disables per-session MFA if found in any roles of a user or in the cluster auth preference. Instead, per-request PIV touch will be required. This functionality will be added in PIV login enforcement #15874.

Backwards compatibility

This PR mirrors #12054 in how the type is changed in a backwards compatible way. Take a look at that PR for a thorough explanation.

@Joerger Joerger changed the title Joerger/require mfa type RequireMFAType Sep 1, 2022
@github-actions github-actions Bot added application-access database-access Database access related issues and PRs desktop-access kubernetes-access machine-id tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Sep 1, 2022
@Joerger Joerger force-pushed the joerger/require-mfa-type branch from 4e964db to 8f481b9 Compare September 1, 2022 02:29
GavinFrazar
GavinFrazar requested changes Sep 1, 2022
View reviewed changes
Comment thread api/types/authentication.go Outdated
Comment thread api/types/authentication.go Outdated
Comment thread api/types/authentication.go Outdated
Comment thread api/types/authentication.go Outdated
GavinFrazar
GavinFrazar reviewed Sep 1, 2022
View reviewed changes
Comment thread lib/auth/grpcserver_test.go Outdated
Comment thread api/client/client_test.go Outdated
Comment thread api/client/client_test.go Outdated
@Joerger Joerger force-pushed the joerger/require-mfa-type branch 2 times, most recently from 416fab4 to 33c9a31 Compare September 2, 2022 01:19
@Joerger Joerger requested a review from GavinFrazar September 2, 2022 01:21
GavinFrazar
GavinFrazar approved these changes Sep 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple more comments but only minor things

Comment thread api/types/authentication.go Outdated
Comment thread api/types/authentication.go Outdated
Comment thread api/types/authentication.go Outdated
@Joerger Joerger mentioned this pull request Sep 2, 2022
Merged
@Joerger Joerger force-pushed the joerger/require-mfa-type branch from 27af113 to a9db185 Compare September 12, 2022 23:37
@Joerger
Replace role/authPref RequireSessionMFA (bool) with RequireMFAType
5b834f8 
(string).

  - Add new RequireMFAType constant values with custom boolean marshalling.

  - Add RequireMFAType to role and auth preference and deprecate
RequireSessionMFA.

  - Add session-mfa override login when hardware_key_touch is enforced.

  - Add protobuf enum for RequireMFAType.

  - Add support for proto enums in protoc-gen-crd and update Kubernetes
Operator manifests.
@Joerger Joerger force-pushed the joerger/require-mfa-type branch from a9db185 to 5b834f8 Compare September 12, 2022 23:51
jakule
jakule reviewed Sep 20, 2022
View reviewed changes
Comment thread api/client/client_test.go Outdated
Comment thread api/client/client_test.go Outdated
Comment thread api/types/authentication.go Outdated
Comment thread lib/auth/grpcserver_test.go Outdated
Comment thread lib/auth/grpcserver_test.go
expectRequired = expectRequired && (roleRequireMFAType.IsSessionMFARequired() || authPrefRequireMFAType.IsSessionMFARequired())

t.Run(fmt.Sprintf("authPref=%v/role=%v/expect=%v", authPrefRequireMFAType, roleRequireMFAType, expectRequired), func(t *testing.T) {
roleOpt := role.GetOptions()
Copy link
Copy Markdown
Contributor

@jakule jakule Sep 20, 2022

Choose a reason for hiding this comment

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

How much does this test take? Can we run it in parallel?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Sep 20, 2022

Choose a reason for hiding this comment

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

It takes ~2.5s for me locally. Since it's changing the test server's Role and AuthPref for each subtest, it can't be run in parallel.

I tested recreating the server for each subtest in order to parallize the tests, but it results in the same ~2.5s run while using more computing resources.

Comment thread lib/services/role.go Outdated
Comment thread lib/services/role.go
Comment thread api/client/client_test.go Outdated
@Joerger Joerger requested a review from jakule September 21, 2022 19:03
@GavinFrazar
Copy link
Copy Markdown
Contributor

GavinFrazar commented Sep 21, 2022

I noticed we don't document require_session_mfa in the current cluster reference yaml:
https://goteleport.com/docs/ver/11.0/reference/config/

but I don't see any point in opening a docs PR to add it right before you merge this and it will need to be changed to the enum. Just want to mention it so when you update the docs you update that reference yaml as well.

How far back are you backporting this?

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Sep 21, 2022

I noticed we don't document require_session_mfa in the current cluster reference yaml: https://goteleport.com/docs/ver/11.0/reference/config/

Nice catch, I'll be making a docs PR once the main change set is landed in master (a couple more PRs...), I'll make sure to include this.

How far back are you backporting this?

Most likely just v10.

@Joerger Joerger enabled auto-merge (squash) September 22, 2022 18:46
@Joerger Joerger merged commit 2654add into master Sep 22, 2022
@Joerger Joerger deleted the joerger/require-mfa-type branch September 22, 2022 22:52
@Joerger Joerger mentioned this pull request Mar 28, 2023
Merged
Joerger added a commit that referenced this pull request Feb 6, 2024
@Joerger
Add deprecation logic for authpref.PIVSlot, following the logic set out
4378230 
in #16034.
Joerger added a commit that referenced this pull request Feb 8, 2024
@Joerger
Add deprecation logic for authpref.PIVSlot, following the logic set out
3aec3bb 
in #16034.
github-merge-queue Bot pushed a commit that referenced this pull request Feb 8, 2024
@Joerger
Hardware Key settings (#37727)
b60e7a7 
* Add hardware key settings to cluster auth preference.

* Parse HardwareKey auth settings from configuration file.

* Add GetRequireKnownSerialNumber method to AuthPrefence type.

* Add deprecation logic for authpref.PIVSlot, following the logic set out
in #16034.

* Address comments.
Joerger added a commit that referenced this pull request Feb 9, 2024
@Joerger
Hardware Key settings (#37727)
7211f50 
* Add hardware key settings to cluster auth preference.

* Parse HardwareKey auth settings from configuration file.

* Add GetRequireKnownSerialNumber method to AuthPrefence type.

* Add deprecation logic for authpref.PIVSlot, following the logic set out
in #16034.

* Address comments.
Joerger added a commit that referenced this pull request Feb 9, 2024
@Joerger
Hardware Key settings (#37727)
c4edb7c 
* Add hardware key settings to cluster auth preference.

* Parse HardwareKey auth settings from configuration file.

* Add GetRequireKnownSerialNumber method to AuthPrefence type.

* Add deprecation logic for authpref.PIVSlot, following the logic set out
in #16034.

* Address comments.
Joerger added a commit that referenced this pull request Feb 9, 2024
@Joerger
Hardware Key settings (#37727)
fcb9fbb 
* Add hardware key settings to cluster auth preference.

* Parse HardwareKey auth settings from configuration file.

* Add GetRequireKnownSerialNumber method to AuthPrefence type.

* Add deprecation logic for authpref.PIVSlot, following the logic set out
in #16034.

* Address comments.
github-merge-queue Bot pushed a commit that referenced this pull request Feb 12, 2024
@Joerger
Hardware Key settings (#37727) (#37974)
72023b0 
* Add hardware key settings to cluster auth preference.

* Parse HardwareKey auth settings from configuration file.

* Add GetRequireKnownSerialNumber method to AuthPrefence type.

* Add deprecation logic for authpref.PIVSlot, following the logic set out
in #16034.

* Address comments.
github-merge-queue Bot pushed a commit that referenced this pull request Feb 12, 2024
@Joerger
Hardware Key settings (#37727) (#37973)
f1f888c 
* Add hardware key settings to cluster auth preference.

* Parse HardwareKey auth settings from configuration file.

* Add GetRequireKnownSerialNumber method to AuthPrefence type.

* Add deprecation logic for authpref.PIVSlot, following the logic set out
in #16034.

* Address comments.
@greedy52 greedy52 mentioned this pull request Feb 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcoandredinis marcoandredinis marcoandredinis left review comments

@GavinFrazar GavinFrazar GavinFrazar approved these changes

+1 more reviewer

@jakule jakule jakule approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

application-access database-access Database access related issues and PRs desktop-access kubernetes-access machine-id tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Joerger @GavinFrazar @marcoandredinis @jakule