Skip to content

Commit

Permalink
Fix Admin MFA required check (gravitational#51618)
Browse files Browse the repository at this point in the history
* Fix admin mfa required check.

* Fix test.

* Add client fallback mechanism by removing pre-existing mfa context from admin action mfa ceremony.
  • Loading branch information
Joerger authored and carloscastrojumo committed Feb 19, 2025
1 parent a8d8eb8 commit d9d1951
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 10 deletions.
9 changes: 8 additions & 1 deletion api/mfa/ceremony.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,13 @@ func PerformAdminActionMFACeremony(ctx context.Context, mfaCeremony CeremonyFn,
},
}

resp, err := mfaCeremony(ctx, challengeRequest, WithPromptReasonAdminAction())
// Remove MFA resp from context if set. This way, the mfa required
// check will return true as long as MFA for admin actions is enabled,
// even if the current context has a reusable MFA. v18 server will
// return this requirement as expected.
// TODO(Joerger): DELETE IN v19.0.0
ceremonyCtx := ContextWithMFAResponse(ctx, nil)

resp, err := mfaCeremony(ceremonyCtx, challengeRequest, WithPromptReasonAdminAction())
return resp, trace.Wrap(err)
}
10 changes: 5 additions & 5 deletions lib/auth/auth_with_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -5991,15 +5991,15 @@ func (a *ServerWithRoles) IsMFARequired(ctx context.Context, req *proto.IsMFAReq
// Check if MFA is required for admin actions. We don't currently have
// a reason to check the name of the admin action in question.
if _, ok := req.Target.(*proto.IsMFARequiredRequest_AdminAction); ok {
if a.context.AdminActionAuthState == authz.AdminActionAuthUnauthorized {
if a.context.AdminActionAuthState == authz.AdminActionAuthNotRequired {
return &proto.IsMFARequiredResponse{
Required: true,
MFARequired: proto.MFARequired_MFA_REQUIRED_YES,
Required: false,
MFARequired: proto.MFARequired_MFA_REQUIRED_NO,
}, nil
} else {
return &proto.IsMFARequiredResponse{
Required: false,
MFARequired: proto.MFARequired_MFA_REQUIRED_NO,
Required: true,
MFARequired: proto.MFARequired_MFA_REQUIRED_YES,
}, nil
}
}
Expand Down
8 changes: 4 additions & 4 deletions lib/auth/auth_with_roles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9545,15 +9545,15 @@ func TestIsMFARequired_AdminAction(t *testing.T) {
name: "mfa verified",
adminActionAuthState: authz.AdminActionAuthMFAVerified,
expectResp: &proto.IsMFARequiredResponse{
Required: false,
MFARequired: proto.MFARequired_MFA_REQUIRED_NO,
Required: true,
MFARequired: proto.MFARequired_MFA_REQUIRED_YES,
},
}, {
name: "mfa verified with reuse",
adminActionAuthState: authz.AdminActionAuthMFAVerifiedWithReuse,
expectResp: &proto.IsMFARequiredResponse{
Required: false,
MFARequired: proto.MFARequired_MFA_REQUIRED_NO,
Required: true,
MFARequired: proto.MFARequired_MFA_REQUIRED_YES,
},
},
} {
Expand Down

0 comments on commit d9d1951

Please sign in to comment.