From 208043c88d2205c45f34b5b27439c04cb41b0f6b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 28 Oct 2026 13:39:52 +0000 Subject: [PATCH] oidc: keep email_verified with the address it came with Fixes ANT-2026-MX8FKG4K --- CHANGELOG.md | 1 + hscontrol/oidc.go | 19 +++++++++-- hscontrol/oidc_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0be972e52..fe1f35d50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -167,6 +167,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - Only the newest follow-up waits on a pending registration or SSH check; older ones are answered at once - Registration locks are freed once no registration uses them - Concurrent deletes of one node free its addresses and announce its removal once +- OIDC verifies an email against the document supplying it; an `email_verified` flag without an address no longer verifies another document's email ## 0.29.7 (202x-xx-xx) diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index fa09f8f6d..c237c6eb4 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -314,8 +314,7 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( if userinfo != nil && userinfo.Claims(&userinfo2) == nil && userinfo2.Sub == claims.Sub { // Update the user with the userinfo claims (with id token claims as fallback). // TODO(kradalby): there might be more interesting fields here that we have not found yet. - claims.Email = cmp.Or(userinfo2.Email, claims.Email) - claims.EmailVerified = cmp.Or(userinfo2.EmailVerified, claims.EmailVerified) + claims.Email, claims.EmailVerified = mergeEmailClaims(&claims, &userinfo2) claims.Username = cmp.Or(userinfo2.PreferredUsername, claims.Username) claims.Name = cmp.Or(userinfo2.Name, claims.Name) @@ -445,6 +444,22 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( } } +// mergeEmailClaims keeps each verification flag with the address it describes. +func mergeEmailClaims( + claims *types.OIDCClaims, + userinfo *types.OIDCUserInfo, +) (string, types.FlexibleBoolean) { + if userinfo.Email == "" { + return claims.Email, claims.EmailVerified + } + + if userinfo.Email == claims.Email { + return claims.Email, claims.EmailVerified || userinfo.EmailVerified + } + + return userinfo.Email, userinfo.EmailVerified +} + func (a *AuthProviderOIDC) determineNodeExpiry(idTokenExpiration time.Time) *time.Time { if a.cfg.UseExpiryFromToken { return &idTokenExpiration diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index c62b4e467..a4325d006 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -621,6 +621,79 @@ func TestRouterMethodNotAllowedIncludesAllow(t *testing.T) { assert.Equal(t, []string{http.MethodGet}, resp.Header.Values("Allow")) } +type oidcUserWithInfo struct { + mockoidc.MockUser + + info types.OIDCUserInfo +} + +func (u *oidcUserWithInfo) Userinfo(_ []string) ([]byte, error) { + return json.Marshal(u.info) +} + +func TestOIDCEmailClaims(t *testing.T) { + for _, tt := range []struct { + name string + tokenEmail string + tokenVerified bool + infoEmail string + infoVerified bool + wantStatus int + }{ + {"different address", "guest@other.example", true, "boss@corp.example", false, http.StatusUnauthorized}, + {"flag without address", "boss@corp.example", false, "", true, http.StatusUnauthorized}, + {"same address token flag", "boss@corp.example", true, "boss@corp.example", false, http.StatusOK}, + {"same address userinfo flag", "boss@corp.example", false, "boss@corp.example", true, http.StatusOK}, + {"no userinfo email", "boss@corp.example", true, "", false, http.StatusOK}, + {"different verified address", "guest@other.example", false, "boss@corp.example", true, http.StatusOK}, + } { + t.Run(tt.name, func(t *testing.T) { + b := newOIDCBrowser(t) + provider, ok := b.app.authProvider.(*AuthProviderOIDC) + require.True(t, ok) + + provider.cfg.EmailVerifiedRequired = true + provider.cfg.AllowedDomains = []string{"corp.example"} + + b.idp.QueueUser(&oidcUserWithInfo{ + Subject: "email-pair", + Email: tt.tokenEmail, + EmailVerified: tt.tokenVerified, + info: types.OIDCUserInfo{ + Sub: "email-pair", + Email: tt.infoEmail, + EmailVerified: types.FlexibleBoolean(tt.infoVerified), + }, + }) + _, registerURL := b.pendingNode(t) + status, _, _ := b.get(t, registerURL) + assert.Equal(t, tt.wantStatus, status) + }) + } +} + +func TestMergeEmailClaims(t *testing.T) { + for _, tt := range []struct { + name string + token types.OIDCClaims + info types.OIDCUserInfo + email string + verified types.FlexibleBoolean + }{ + {"different address", types.OIDCClaims{Email: "old@example.com", EmailVerified: true}, types.OIDCUserInfo{Email: "new@example.com"}, "new@example.com", false}, + {"flag without address", types.OIDCClaims{Email: "old@example.com"}, types.OIDCUserInfo{EmailVerified: true}, "old@example.com", false}, + {"same address token flag", types.OIDCClaims{Email: "same@example.com", EmailVerified: true}, types.OIDCUserInfo{Email: "same@example.com"}, "same@example.com", true}, + {"same address userinfo flag", types.OIDCClaims{Email: "same@example.com"}, types.OIDCUserInfo{Email: "same@example.com", EmailVerified: true}, "same@example.com", true}, + {"no userinfo email", types.OIDCClaims{Email: "old@example.com", EmailVerified: true}, types.OIDCUserInfo{}, "old@example.com", true}, + } { + t.Run(tt.name, func(t *testing.T) { + email, verified := mergeEmailClaims(&tt.token, &tt.info) + assert.Equal(t, tt.email, email) + assert.Equal(t, tt.verified, verified) + }) + } +} + func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) { t.Parallel()