Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v15] Don't count mfa-verifeid Private Key Policy as MFA for Admin Actions #39306

Merged
merged 3 commits into from Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/utils/keys/policy.go
Expand Up @@ -116,6 +116,10 @@ func (p PrivateKeyPolicy) IsHardwareKeyPolicy() bool {

// MFAVerified checks that private keys with this key policy count as MFA verified.
// Both Hardware key touch and pin are count as MFA verification.
//
// Note: MFA checks with private key policies are only performed during the establishment
// of the connection, during the TLS/SSH handshake. For long term connections, MFA should
// be re-verified through other methods (e.g. webauthn).
func (p PrivateKeyPolicy) MFAVerified() bool {
return p.isHardwareKeyTouchVerified() || p.isHardwareKeyPINVerified()
}
Expand Down
6 changes: 0 additions & 6 deletions lib/authz/permissions.go
Expand Up @@ -490,12 +490,6 @@ func (a *authorizer) isAdminActionAuthorizationRequired(ctx context.Context, aut
}

func (a *authorizer) authorizeAdminAction(ctx context.Context, authContext *Context) error {
// Certain hardware-key based private key policies require MFA for each request.
if authContext.Identity.GetIdentity().PrivateKeyPolicy.MFAVerified() {
authContext.AdminActionAuthState = AdminActionAuthMFAVerified
return nil
}

// MFA is required to be passed through the request context.
mfaResp, err := mfa.CredentialsFromContext(ctx)
if err != nil {
Expand Down
20 changes: 10 additions & 10 deletions lib/authz/permissions_test.go
Expand Up @@ -561,6 +561,16 @@ func TestAuthorizer_AuthorizeAdminAction(t *testing.T) {
},
},
wantAdminActionAuthorized: false,
}, {
name: "NOK local user mfa verified private key policy",
user: LocalUser{
Username: localUser.GetName(),
Identity: tlsca.Identity{
Username: localUser.GetName(),
PrivateKeyPolicy: keys.PrivateKeyPolicyHardwareKeyTouch,
},
},
wantAdminActionAuthorized: false,
}, {
// edge case for the admin role check.
name: "NOK local user with host-like username",
Expand Down Expand Up @@ -613,16 +623,6 @@ func TestAuthorizer_AuthorizeAdminAction(t *testing.T) {
withMFA: validMFAWithReuse,
allowedReusedMFA: true,
wantAdminActionAuthorized: true,
}, {
name: "OK local user mfa verified private key policy",
user: LocalUser{
Username: localUser.GetName(),
Identity: tlsca.Identity{
Username: localUser.GetName(),
PrivateKeyPolicy: keys.PrivateKeyPolicyHardwareKeyTouch,
},
},
wantAdminActionAuthorized: true,
}, {
name: "OK admin",
user: BuiltinRole{
Expand Down
5 changes: 4 additions & 1 deletion lib/tlsca/ca.go
Expand Up @@ -1119,7 +1119,10 @@ func (id Identity) GetSessionMetadata(sid string) events.SessionMetadata {
}
}

// IsMFAVerified returns whether this identity is MFA verified.
// IsMFAVerified returns whether this identity is MFA verified. This MFA
// verification may or may not have taken place recently, so it should not
// be treated as blanket MFA verification uncritically. For example, MFA
// should be re-verified for login procedures or admin actions.
func (id *Identity) IsMFAVerified() bool {
return id.MFAVerified != "" || id.PrivateKeyPolicy.MFAVerified()
}
Expand Down