mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
Merge commit from fork
The Login V1 MFA init/enrollment handlers acted on authReq.UserID via setUserContext without verifying the auth request had completed its first factor. A request that only submitted a loginname could enroll TOTP/SMS/Email/U2F factors and overwrite the victim's phone number, and the discrepant errors bypassed IgnoreUnknownUsernames. Gate the enrollment handlers on PossibleSteps[0] == *domain.MFAPromptStep, which nextSteps only yields after firstFactorChecked passes. Co-authored-by: Claude Opus 4.8 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
8a61aa4f36
commit
522b0ed7ed
@@ -26,6 +26,7 @@ import (
|
|||||||
"github.com/zitadel/zitadel/internal/form"
|
"github.com/zitadel/zitadel/internal/form"
|
||||||
"github.com/zitadel/zitadel/internal/query"
|
"github.com/zitadel/zitadel/internal/query"
|
||||||
"github.com/zitadel/zitadel/internal/static"
|
"github.com/zitadel/zitadel/internal/static"
|
||||||
|
"github.com/zitadel/zitadel/internal/zerrors"
|
||||||
)
|
)
|
||||||
|
|
||||||
type Login struct {
|
type Login struct {
|
||||||
@@ -336,6 +337,31 @@ func setUserContext(ctx context.Context, userID, resourceOwner string) context.C
|
|||||||
return authz.SetCtxData(ctx, data)
|
return authz.SetCtxData(ctx, data)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// checkMFAPromptStep ensures the auth request has legitimately reached the MFA prompt
|
||||||
|
// (enrollment) step before any MFA registration/initialization is performed.
|
||||||
|
// MFA enrollment promotes authReq.UserID to the request principal via setUserContext, so it
|
||||||
|
// must only be reachable once the preceding factors have been checked. nextSteps only yields
|
||||||
|
// an *domain.MFAPromptStep after firstFactorChecked has passed, so an unauthenticated request
|
||||||
|
// that merely submitted a loginname (or otherwise skipped the first factor) will not carry it.
|
||||||
|
func (l *Login) checkMFAPromptStep(w http.ResponseWriter, r *http.Request, authReq *domain.AuthRequest) bool {
|
||||||
|
if !isMFAPromptStep(authReq) {
|
||||||
|
l.renderError(w, r, authReq, zerrors.ThrowPreconditionFailed(nil, "LOGIN-s9GW3M", "Errors.User.NotAllowed"))
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
// isMFAPromptStep reports whether the auth request's current step is the MFA prompt
|
||||||
|
// (enrollment) step. nextSteps only yields an *domain.MFAPromptStep once firstFactorChecked
|
||||||
|
// has passed, so this is false for a request that merely submitted a loginname.
|
||||||
|
func isMFAPromptStep(authReq *domain.AuthRequest) bool {
|
||||||
|
if authReq == nil || len(authReq.PossibleSteps) == 0 {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
_, ok := authReq.PossibleSteps[0].(*domain.MFAPromptStep)
|
||||||
|
return ok
|
||||||
|
}
|
||||||
|
|
||||||
func (l *Login) baseURL(ctx context.Context) string {
|
func (l *Login) baseURL(ctx context.Context) string {
|
||||||
return http_utils.DomainContext(ctx).Origin() + HandlerPrefix
|
return http_utils.DomainContext(ctx).Origin() + HandlerPrefix
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -79,6 +79,9 @@ func (l *Login) handleRegisterSMSCheck(w http.ResponseWriter, r *http.Request) {
|
|||||||
l.renderError(w, r, authReq, err)
|
l.renderError(w, r, authReq, err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if !l.checkMFAPromptStep(w, r, authReq) {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
ctx := setUserContext(r.Context(), authReq.UserID, authReq.UserOrgID)
|
ctx := setUserContext(r.Context(), authReq.UserID, authReq.UserOrgID)
|
||||||
// save the current state
|
// save the current state
|
||||||
|
|||||||
@@ -0,0 +1,76 @@
|
|||||||
|
package login
|
||||||
|
|
||||||
|
import (
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
|
||||||
|
"github.com/zitadel/zitadel/internal/domain"
|
||||||
|
)
|
||||||
|
|
||||||
|
// Test_isMFAPromptStep guards the fix for the unauthenticated MFA-enrollment / phone-overwrite
|
||||||
|
// issue: MFA init/enrollment handlers must only proceed when the auth request has legitimately
|
||||||
|
// reached the MFA prompt step (i.e. after the first factor was checked). A request that only
|
||||||
|
// submitted a loginname carries a first-factor step, not an *domain.MFAPromptStep.
|
||||||
|
func Test_isMFAPromptStep(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
authReq *domain.AuthRequest
|
||||||
|
want bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "nil auth request",
|
||||||
|
authReq: nil,
|
||||||
|
want: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "no possible steps",
|
||||||
|
authReq: &domain.AuthRequest{},
|
||||||
|
want: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "loginname only - first factor (password) step",
|
||||||
|
authReq: &domain.AuthRequest{
|
||||||
|
PossibleSteps: []domain.NextStep{&domain.PasswordStep{}},
|
||||||
|
},
|
||||||
|
want: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "existing factor - MFA verification step, not enrollment",
|
||||||
|
authReq: &domain.AuthRequest{
|
||||||
|
PossibleSteps: []domain.NextStep{
|
||||||
|
&domain.MFAVerificationStep{MFAProviders: []domain.MFAType{domain.MFATypeTOTP}},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
want: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "legitimate enrollment - MFA prompt step",
|
||||||
|
authReq: &domain.AuthRequest{
|
||||||
|
PossibleSteps: []domain.NextStep{
|
||||||
|
&domain.MFAPromptStep{Required: true, MFAProviders: []domain.MFAType{domain.MFATypeTOTP}},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
want: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "MFA prompt step not first - only the current step counts",
|
||||||
|
authReq: &domain.AuthRequest{
|
||||||
|
PossibleSteps: []domain.NextStep{
|
||||||
|
&domain.PasswordStep{},
|
||||||
|
&domain.MFAPromptStep{Required: true, MFAProviders: []domain.MFAType{domain.MFATypeTOTP}},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
want: false,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tc := range tests {
|
||||||
|
t.Run(tc.name, func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
assert.Equal(t, tc.want, isMFAPromptStep(tc.authReq))
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -44,6 +44,9 @@ func (l *Login) handleRegisterU2F(w http.ResponseWriter, r *http.Request) {
|
|||||||
l.renderError(w, r, authReq, err)
|
l.renderError(w, r, authReq, err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if !l.checkMFAPromptStep(w, r, authReq) {
|
||||||
|
return
|
||||||
|
}
|
||||||
credData, err := base64.URLEncoding.DecodeString(data.CredentialData)
|
credData, err := base64.URLEncoding.DecodeString(data.CredentialData)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
l.renderRegisterU2F(w, r, authReq, err)
|
l.renderRegisterU2F(w, r, authReq, err)
|
||||||
|
|||||||
@@ -31,6 +31,9 @@ func (l *Login) handleMFAInitVerify(w http.ResponseWriter, r *http.Request) {
|
|||||||
l.renderError(w, r, authReq, err)
|
l.renderError(w, r, authReq, err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if !l.checkMFAPromptStep(w, r, authReq) {
|
||||||
|
return
|
||||||
|
}
|
||||||
var verifyData *mfaVerifyData
|
var verifyData *mfaVerifyData
|
||||||
switch data.MFAType {
|
switch data.MFAType {
|
||||||
case domain.MFATypeTOTP:
|
case domain.MFATypeTOTP:
|
||||||
|
|||||||
@@ -23,6 +23,9 @@ func (l *Login) handleMFAPrompt(w http.ResponseWriter, r *http.Request) {
|
|||||||
l.renderError(w, r, authReq, err)
|
l.renderError(w, r, authReq, err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if !l.checkMFAPromptStep(w, r, authReq) {
|
||||||
|
return
|
||||||
|
}
|
||||||
if !data.Skip {
|
if !data.Skip {
|
||||||
mfaVerifyData := new(mfaVerifyData)
|
mfaVerifyData := new(mfaVerifyData)
|
||||||
mfaVerifyData.MFAType = data.MFAProvider
|
mfaVerifyData.MFAType = data.MFAProvider
|
||||||
|
|||||||
Reference in New Issue
Block a user