From ac8ed652038ce13ceaf886bb4b08aa6c42b620b8 Mon Sep 17 00:00:00 2001 From: tegwick Date: Sun, 13 Sep 2026 00:27:28 +0200 Subject: [PATCH] Support opt-in MFA per browser client with authoritative enrollment checks Assistant: codex Assistant-Model: gpt-6-astra Assistant-Session: 01a092fe-13b1-7f12-ac74-7d258af4d79c --- docs/optional-mfa.md | 59 +++++++++++++++++++ spec/canonical-model.yaml | 7 +++ src/cmd/keycape/main.go | 1 + src/internal/adapters/privacyidea/adapter.go | 20 +++++-- .../adapters/privacyidea/adapter_test.go | 6 +- .../adapters/privacyidea/optional_mfa_test.go | 43 ++++++++++++++ src/internal/config/config.go | 2 + src/internal/config/optional_mfa_test.go | 40 +++++++++++++ src/internal/config/validate.go | 6 ++ src/internal/domain/model.go | 10 ++-- .../migration/tokeycloak/transformer.go | 4 ++ src/internal/server/oidc/authorize.go | 6 +- src/internal/server/oidc/optional_mfa_test.go | 50 ++++++++++++++++ workplans/KEY-WP-0035-optional-mfa.md | 56 ++++++++++++++++++ 14 files changed, 298 insertions(+), 12 deletions(-) create mode 100644 docs/optional-mfa.md create mode 100644 src/internal/adapters/privacyidea/optional_mfa_test.go create mode 100644 src/internal/config/optional_mfa_test.go create mode 100644 src/internal/server/oidc/optional_mfa_test.go create mode 100644 workplans/KEY-WP-0035-optional-mfa.md diff --git a/docs/optional-mfa.md b/docs/optional-mfa.md new file mode 100644 index 0000000..ef4fdaa --- /dev/null +++ b/docs/optional-mfa.md @@ -0,0 +1,59 @@ +# Optional MFA for browser clients + +Set `mfaOptional: true` on a reviewed browser registration to allow AAL1 login +when privacyIDEA authoritatively reports no active enrolled factor. Once an +active factor exists, require AAL2. A previous AAL1 session cannot satisfy that +requirement. Explicit AAL2 requests still require MFA. Lookup errors deny login +through the existing account-recovery path. This setting overrides only the +provider `requireForAll` default for that client. + +Do not combine this with `mfaRequired`. In particular, `mfaRequired: false` +means unconditional AAL1 and is not an opt-in enrollment policy. Unchanged +registrations keep their existing policy. Service clients reject mfaOptional. + +The adapter sends the raw privacyIDEA JWT in Authorization, filters active +factors before pagination, and requires a successful, complete response before +concluding there is no active factor. The credential must have realm-scoped +administrative tokenlist permission: a user-role token only lists its own factors +regardless of the requested username. Never replace it with a self-service JWT. +See the [provider API](https://privacyidea.readthedocs.io/en/stable/modules/api/token.html). + +## Deployment gate — not yet enabled + +Live inspection on 2026-09-13 found `requireForAll: true`; both the demo-company +and account-portal registrations inherit it. Configured token-list credentials +return HTTP 401, so enabling this setting now would replace the OTP prompt with +a lookup failure. No live policy has been changed. + +Credential owner: railiance-platform / OpenBao, route +`net-kingdom-privacyidea-admin-token`. Its concrete delivery and renewal contract +is unpublished (`resolvable: false`). Obtain an owner-approved realm-scoped +factor-read credential through the native custody path, with renewal and +revocation ownership. Do not put credentials in chat, arguments, work records, +or config examples. The older refresh-pi-token-live.sh needs review before use. + +After credential delivery: + +1. Verify the deployed provider accepts the raw JWT and returns authoritative + count/tokens results for controlled accounts with and without a factor. + Confirm realm/resolver mapping and administrative tokenlist scope, including + visibility of another user's factor; an empty list alone is not proof. +2. Verify self-service at https://pink-account.coulomb.social: password login, + TOTP enrollment with possession confirmation before activation, cancellation, + and factor removal/recovery. This user-facing flow is not yet verified. +3. Build and pin the reviewed issuer image. Migrate only the exact + vergabe-demo-company registration to mfaOptional using the owner CAS rollout + lane; keep unrelated registrations and Secret bytes intact. The current + vergabe-client-rollout.py intentionally refuses registration differences and + needs an explicit migration before this policy can be applied. +4. Resolve account-portal access for unenrolled users without weakening privileged + access. Review portal policy and self-service permissions before expanding the + optional setting to this shared client. Add the verified enrollment link to + account management once that flow works. +5. Verify fresh password-only admission without a factor, enrollment confirmation, + then OTP enforcement (including reuse of an old AAL1 session). Verify explicit + AAL2 still challenges, lookup failures recover without issuing a code, logout + permits identity switching, and other clients retain their policies. + +Rollback: restore the exact previous client registration and image pin via CAS. +This restores mandatory MFA for the demo client; it is not password-only access. diff --git a/spec/canonical-model.yaml b/spec/canonical-model.yaml index 7ccbefb..a657bcd 100644 --- a/spec/canonical-model.yaml +++ b/spec/canonical-model.yaml @@ -176,6 +176,13 @@ entities: items: type: string description: "Role claims emitted for this client's service tokens." + mfaOptional: + type: boolean + description: > + Require MFA only after factor enrollment for this browser client, + overriding the provider require-for-all default. Enrollment lookup + failures deny login. Cannot be combined with mfaRequired. Explicit + AAL2 requests still require MFA. mfaRequired: type: boolean nullable: true diff --git a/src/cmd/keycape/main.go b/src/cmd/keycape/main.go index 1e51b2a..bf17c13 100644 --- a/src/cmd/keycape/main.go +++ b/src/cmd/keycape/main.go @@ -410,6 +410,7 @@ func buildClientRegistry(cfgClients []config.ClientConfig) (map[string]*domain.C Roles: c.Roles, TokenLifetime: clientTokenLifetime, MFARequired: c.MFARequired, + MFAOptional: c.MFAOptional, RegistrationURL: c.RegistrationURL, EnrollmentURL: c.EnrollmentURL, } diff --git a/src/internal/adapters/privacyidea/adapter.go b/src/internal/adapters/privacyidea/adapter.go index c3618bc..03e87dd 100644 --- a/src/internal/adapters/privacyidea/adapter.go +++ b/src/internal/adapters/privacyidea/adapter.go @@ -57,12 +57,14 @@ func (a *PrivacyIDEAAdapter) hasActiveToken(ctx context.Context, userID string) q := url.Values{} q.Set("user", userID) q.Set("realm", a.cfg.realm()) + // Filter before pagination so an active factor cannot be hidden on page two. + q.Set("active", "True") req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint+"?"+q.Encode(), nil) if err != nil { return false, fmt.Errorf("privacyidea: build token list request: %w", err) } - req.Header.Set("Authorization", "Bearer "+a.cfg.AdminToken) + req.Header.Set("Authorization", a.cfg.AdminToken) resp, err := a.client.Do(req) if err != nil { @@ -84,11 +86,20 @@ func (a *PrivacyIDEAAdapter) hasActiveToken(ctx context.Context, userID string) return false, fmt.Errorf("privacyidea: decode token list response: %w", err) } + if !parsed.Result.Status || parsed.Result.Value.Tokens == nil || parsed.Result.Value.Count == nil || *parsed.Result.Value.Count < len(parsed.Result.Value.Tokens) { + return false, fmt.Errorf("privacyidea: incomplete or unsuccessful token list response") + } for _, tok := range parsed.Result.Value.Tokens { - if tok.Active { + if tok.Active == nil { + return false, fmt.Errorf("privacyidea: token missing active state") + } + if *tok.Active { return true, nil } } + if *parsed.Result.Value.Count > len(parsed.Result.Value.Tokens) { + return false, fmt.Errorf("privacyidea: incomplete token list page") + } return false, nil } @@ -110,7 +121,7 @@ func (a *PrivacyIDEAAdapter) ValidateMFAToken(ctx context.Context, userID, token return fmt.Errorf("privacyidea: build validate request: %w", err) } req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - req.Header.Set("Authorization", "Bearer "+a.cfg.AdminToken) + req.Header.Set("Authorization", a.cfg.AdminToken) resp, err := a.client.Do(req) if err != nil { @@ -148,6 +159,7 @@ type tokenListResponse struct { Status bool `json:"status"` Value struct { Tokens []tokenEntry `json:"tokens"` + Count *int `json:"count"` } `json:"value"` } `json:"result"` } @@ -155,7 +167,7 @@ type tokenListResponse struct { // tokenEntry represents a single token entry in the token list response. type tokenEntry struct { Serial string `json:"serial"` - Active bool `json:"active"` + Active *bool `json:"active"` } // validateResponse models the privacyIDEA POST /validate/check response envelope. diff --git a/src/internal/adapters/privacyidea/adapter_test.go b/src/internal/adapters/privacyidea/adapter_test.go index f17359c..49a56c6 100644 --- a/src/internal/adapters/privacyidea/adapter_test.go +++ b/src/internal/adapters/privacyidea/adapter_test.go @@ -64,7 +64,7 @@ func tokenListResponse(tokens []map[string]interface{}) string { tokenJSON += fmt.Sprintf(`{"serial":"TOK%d","active":%v}`, i, active) } tokenJSON += "]" - return fmt.Sprintf(`{"result":{"status":true,"value":{"tokens":%s}}}`, tokenJSON) + return fmt.Sprintf(`{"result":{"status":true,"value":{"tokens":%s,"count":%d}}}`, tokenJSON, len(tokens)) } // validateResponse builds a privacyIDEA /validate/check JSON response. @@ -243,8 +243,8 @@ func TestCheckMFARequired_SendsAdminToken(t *testing.T) { client := &mockHTTPClient{ doFn: func(req *http.Request) (*http.Response, error) { auth := req.Header.Get("Authorization") - if !strings.HasPrefix(auth, "Bearer ") { - t.Errorf("expected Bearer token in Authorization, got %q", auth) + if auth != "service-jwt" { + t.Errorf("expected raw service JWT in Authorization, got %q", auth) } if !strings.Contains(auth, "service-jwt") { t.Errorf("expected admin token in Authorization header, got %q", auth) diff --git a/src/internal/adapters/privacyidea/optional_mfa_test.go b/src/internal/adapters/privacyidea/optional_mfa_test.go new file mode 100644 index 0000000..3735ed1 --- /dev/null +++ b/src/internal/adapters/privacyidea/optional_mfa_test.go @@ -0,0 +1,43 @@ +package privacyidea_test + +import ( + "context" + "keycape/internal/adapters/privacyidea" + "net/http" + "testing" +) + +func TestEnrollmentLookupRejectsIndeterminateResponses(t *testing.T) { + for _, body := range []string{ + `{}`, `{"result":{"status":true,"value":{"tokens":[{}],"count":1}}}`, `{"result":{"status":false,"value":{"tokens":[],"count":0}}}`, + `{"result":{"status":true,"value":{"count":0}}}`, + `{"result":{"status":true,"value":{"tokens":[]}}}`, + `{"result":{"status":true,"value":{"tokens":null,"count":0}}}`, + `{"result":{"status":true,"value":{"tokens":[],"count":1}}}`, + `{"result":{"status":true,"value":{"tokens":[],"count":-1}}}`, + } { + t.Run(body, func(t *testing.T) { + adapter := privacyidea.New(testConfig(), &mockHTTPClient{doFn: func(*http.Request) (*http.Response, error) { return jsonResponse(body), nil }}) + required, err := adapter.HasEnrolledFactor(context.Background(), "alice") + if err == nil || required { + t.Fatalf("indeterminate lookup accepted: required=%v error=%v", required, err) + } + }) + } +} +func TestEnrollmentLookupFiltersBeforePagination(t *testing.T) { + adapter := privacyidea.New(testConfig(), &mockHTTPClient{doFn: func(req *http.Request) (*http.Response, error) { + q := req.URL.Query() + if q.Get("user") != "alice" || q.Get("realm") != "netkingdom" || q.Get("active") != "True" { + t.Fatalf("incorrect filter: %v", q) + } + if req.Header.Get("Authorization") != "service-jwt" { + t.Fatal("provider requires raw JWT") + } + return jsonResponse(`{"result":{"status":true,"value":{"tokens":[{"active":true}],"count":20}}}`), nil + }}) + required, err := adapter.HasEnrolledFactor(context.Background(), "alice") + if err != nil || !required { + t.Fatalf("active factor missed: %v %v", required, err) + } +} diff --git a/src/internal/config/config.go b/src/internal/config/config.go index 9e9d4ad..555f3dd 100644 --- a/src/internal/config/config.go +++ b/src/internal/config/config.go @@ -61,6 +61,7 @@ type ClientConfig struct { Tenant string `yaml:"tenant,omitempty"` Roles []string `yaml:"roles,omitempty"` TokenLifetime string `yaml:"tokenLifetime,omitempty"` + MFAOptional bool `yaml:"mfaOptional,omitempty"` MFARequired *bool `yaml:"mfaRequired,omitempty"` RegistrationURL string `yaml:"registrationUrl,omitempty"` EnrollmentURL string `yaml:"enrollmentUrl,omitempty"` @@ -130,6 +131,7 @@ func (c *Config) Registrations() ([]domain.Client, error) { Roles: cc.Roles, TokenLifetime: lifetime, MFARequired: cc.MFARequired, + MFAOptional: cc.MFAOptional, RegistrationURL: cc.RegistrationURL, EnrollmentURL: cc.EnrollmentURL, }) diff --git a/src/internal/config/optional_mfa_test.go b/src/internal/config/optional_mfa_test.go new file mode 100644 index 0000000..585aa62 --- /dev/null +++ b/src/internal/config/optional_mfa_test.go @@ -0,0 +1,40 @@ +package config_test + +import ( + "keycape/internal/config" + "strings" + "testing" +) + +func TestOptionalMFALoadAndRegistration(t *testing.T) { + cfg, err := config.Load(writeTempFile(t, "clients:\n - clientId: demo\n mfaOptional: true\n")) + if err != nil { + t.Fatal(err) + } + clients, err := cfg.Registrations() + if err != nil { + t.Fatal(err) + } + if !clients[0].MFAOptional || clients[0].MFARequired != nil { + t.Fatal("optional MFA policy lost in registration") + } +} +func TestOptionalMFAValidation(t *testing.T) { + for _, explicit := range []bool{false, true} { + cfg := validConfig(writeTempFile(t, "placeholder")) + cfg.Clients[0].MFAOptional = true + if err := config.ValidateConfig(cfg); len(err) != 0 { + t.Fatal(err) + } + cfg.Clients[0].MFARequired = &explicit + if err := config.ValidateConfig(cfg); len(err) == 0 || !strings.Contains(strings.Join(err, ";"), "mfaOptional cannot be combined") { + t.Fatalf("ambiguous policy accepted: %v", err) + } + } + cfg := validConfig(writeTempFile(t, "placeholder")) + cfg.Clients[0].MFAOptional = true + cfg.Clients[0].GrantTypes = []string{"client_credentials"} + if err := config.ValidateConfig(cfg); len(err) == 0 || !strings.Contains(strings.Join(err, ";"), "mfaOptional is only supported") { + t.Fatalf("service policy accepted: %v", err) + } +} diff --git a/src/internal/config/validate.go b/src/internal/config/validate.go index 2ba7405..da7847c 100644 --- a/src/internal/config/validate.go +++ b/src/internal/config/validate.go @@ -78,6 +78,12 @@ func ValidateConfig(cfg *Config) []string { if strings.TrimSpace(c.Audience) != c.Audience || strings.ContainsAny(c.Audience, " \t\r\n") { errs = append(errs, prefix+": audience must be a single non-whitespace identifier") } + if c.MFAOptional && c.MFARequired != nil { + errs = append(errs, prefix+": mfaOptional cannot be combined with mfaRequired") + } + if c.MFAOptional && contains(c.GrantTypes, "client_credentials") { + errs = append(errs, prefix+": mfaOptional is only supported for browser clients") + } hasAuthorizationCode := contains(c.GrantTypes, "authorization_code") hasClientCredentials := contains(c.GrantTypes, "client_credentials") if (hasAuthorizationCode || !hasClientCredentials) && len(c.RedirectURIs) == 0 { diff --git a/src/internal/domain/model.go b/src/internal/domain/model.go index e44b30e..5a94da7 100644 --- a/src/internal/domain/model.go +++ b/src/internal/domain/model.go @@ -63,10 +63,12 @@ type Client struct { Roles []string `yaml:"roles,omitempty" json:"roles,omitempty"` // TokenLifetime overrides the server default for this confidential client. // It is internal runtime policy, not identity data serialized into tokens. - TokenLifetime time.Duration `yaml:"-" json:"-"` - MFARequired *bool `yaml:"mfaRequired,omitempty" json:"mfaRequired,omitempty"` - RegistrationURL string `yaml:"registrationUrl,omitempty" json:"registrationUrl,omitempty"` - EnrollmentURL string `yaml:"enrollmentUrl,omitempty" json:"enrollmentUrl,omitempty"` + TokenLifetime time.Duration `yaml:"-" json:"-"` + // MFAOptional requires MFA for enrolled users, independently of RequireForAll. + MFAOptional bool `yaml:"mfaOptional,omitempty" json:"mfaOptional,omitempty"` + MFARequired *bool `yaml:"mfaRequired,omitempty" json:"mfaRequired,omitempty"` + RegistrationURL string `yaml:"registrationUrl,omitempty" json:"registrationUrl,omitempty"` + EnrollmentURL string `yaml:"enrollmentUrl,omitempty" json:"enrollmentUrl,omitempty"` } // Membership links a user to a group. diff --git a/src/internal/migration/tokeycloak/transformer.go b/src/internal/migration/tokeycloak/transformer.go index cabfe62..4666a2b 100644 --- a/src/internal/migration/tokeycloak/transformer.go +++ b/src/internal/migration/tokeycloak/transformer.go @@ -372,6 +372,10 @@ func mapClient(c domain.Client) (KeycloakClient, []string) { if c.EnrollmentURL != "" { kc.Attributes["keycape.enrollmentUrl"] = c.EnrollmentURL } + if c.MFAOptional { + kc.Attributes["keycape.mfaOptional"] = "true" + unpreserved = append(unpreserved, fmt.Sprintf("client %q: mfaOptional requires a manually verified conditional MFA authentication flow; enrollment policy is not enforced by import", c.ClientID)) + } if c.MFARequired != nil && *c.MFARequired { // Keycloak expresses this as an authentication flow binding, which a // realm import cannot synthesise from a boolean. diff --git a/src/internal/server/oidc/authorize.go b/src/internal/server/oidc/authorize.go index c94bf0a..b3abc06 100644 --- a/src/internal/server/oidc/authorize.go +++ b/src/internal/server/oidc/authorize.go @@ -375,7 +375,11 @@ func (h *AuthorizeHandler) decideAssurance(ctx context.Context, ps *PendingState providerRequired := false if !domain.ACRRequiresAAL2(ps.ACRValues) && (client == nil || client.MFARequired == nil) { var err error - providerRequired, err = h.MFA.CheckMFARequired(ctx, username) + if client != nil && client.MFAOptional { + providerRequired, err = h.MFA.HasEnrolledFactor(ctx, username) + } else { + providerRequired, err = h.MFA.CheckMFARequired(ctx, username) + } if err != nil { return domain.AssuranceDecision{}, err } diff --git a/src/internal/server/oidc/optional_mfa_test.go b/src/internal/server/oidc/optional_mfa_test.go new file mode 100644 index 0000000..2f8eecd --- /dev/null +++ b/src/internal/server/oidc/optional_mfa_test.go @@ -0,0 +1,50 @@ +package oidc_test + +import ( + "errors" + "keycape/internal/domain" + "keycape/internal/server/oidc" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestOptionalMFAFollowsEnrollment(t *testing.T) { + for _, tc := range []struct { + name string + enrolled bool + lookupErr error + acr []string + want int + challenge bool + }{ + {name: "unenrolled despite global requirement", want: http.StatusFound}, + {name: "enrolled", enrolled: true, want: http.StatusOK, challenge: true}, + {name: "lookup unavailable", lookupErr: errors.New("provider unavailable"), want: http.StatusInternalServerError}, + {name: "explicit step up", acr: []string{"aal2"}, lookupErr: errors.New("must not query enrollment"), want: http.StatusOK, challenge: true}, + } { + t.Run(tc.name, func(t *testing.T) { + sessions := oidc.NewSessionStore() + h := &oidc.AuthorizeHandler{ + ClientConfig: map[string]*domain.Client{"test-client": {ClientID: "test-client", MFAOptional: true}}, + Auth: &mockAuthProvider{callbackResult: &domain.AuthResult{Username: "alice"}}, + MFA: &mockMFAProvider{required: true, requiredErr: errors.New("global policy must not be queried"), enrolled: tc.enrolled, enrolledErr: tc.lookupErr}, + Sessions: sessions, Emitter: &captureEmitter{}, + } + h.PendingStates().Store("optional", &oidc.PendingState{ClientID: "test-client", RedirectURI: "https://app.example/callback", State: "optional", ACRValues: tc.acr, ExpiresAt: time.Now().Add(time.Minute)}) + rec := httptest.NewRecorder() + h.ServeHTTPCallback(rec, httptest.NewRequest(http.MethodGet, "/authorize/callback?code=x&state=optional", nil)) + if rec.Code != tc.want { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "KeyCape MFA") != tc.challenge { + t.Fatal("unexpected MFA challenge state") + } + if tc.want != http.StatusFound && strings.Contains(rec.Header().Get("Location"), "code=") { + t.Fatal("issued authorization code before MFA") + } + }) + } +} diff --git a/workplans/KEY-WP-0035-optional-mfa.md b/workplans/KEY-WP-0035-optional-mfa.md new file mode 100644 index 0000000..83ee54a --- /dev/null +++ b/workplans/KEY-WP-0035-optional-mfa.md @@ -0,0 +1,56 @@ +--- +id: KEY-WP-0035 +type: workplan +title: "Opt-in MFA for demo-company login" +domain: infotech +repo: key-cape +status: active +owner: codex +topic_slug: infotech +created: "2026-09-13" +updated: "2026-09-13" +--- + +Requested behavior: password-only login before OTP activation; require OTP after +activation. Do not lower assurance of unrelated applications. + +## Implement enrollment-dependent browser policy + +```task +id: KEY-WP-0035-T01 +status: done +priority: high +``` + +Add mfaOptional, preserve explicit AAL2 and existing client policy, fail closed on +provider lookup errors and malformed/incomplete responses. Preserve the setting +in config/runtime/canonical model and flag manual migration requirements. +Validation: `go test ./...` and `git diff --check` passed on 2026-09-13. See docs/optional-mfa.md. + +## Restore authoritative factor lookup and enable the reviewed client + +```task +id: KEY-WP-0035-T02 +status: wait +priority: high +``` + +Live factor-read credentials return HTTP 401. The owner route +net-kingdom-privacyidea-admin-token is non-resolvable pending railiance-platform's +approved custody/renewal contract (NK-WP-0033). Native credential handoff required; +no secrets in work records. Do not enable the policy before lookup is verified. +Prepare exact byte-preserving client migration after the provider contract is +available; deploy digest-pinned source and run no-factor/enrolled/error checks. + +## Verify optional enrollment and account management access + +```task +id: KEY-WP-0035-T03 +status: wait +priority: high +``` + +Verify provider self-service login, possession-confirmed activation, cancellation +and removal/recovery. Resolve shared portal assurance scope before surfacing the +verified OTP setup link. Actual user login acceptance remains open under +KEY-WP-0034 and VERGABE-WP-0019; this work does not finish either workplan.