opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-09-24 15:04:27 +08:00
Code Issues Packages Projects Releases Wiki Activity

chore: prevent authentication of non-unique oidc subjects (#16498)

Browse Source 
Any IdP returning an empty field here breaks the assumption of a
unique subject id. This is defined in the OIDC spec.
This commit is contained in:
Steven Masley
2025-02-10 09:31:08 -06:00
committed by GitHub
parent 695d552cd0
commit d0a534e30d
6 changed files with 92 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
coderd/oauthpki/okidcpki_test.go
+2
View File
@@ -13,6 +13,7 @@ import (
"github.com/coreos/go-oidc/v3/oidc"
"github.com/golang-jwt/jwt/v4"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/oauth2"
@@ -169,6 +170,7 @@ func TestAzureAKPKIWithCoderd(t *testing.T) {
const email = "alice@coder.com"
claims := jwt.MapClaims{
"email": email,
"sub": uuid.NewString(),
}
helper := oidctest.NewLoginHelper(owner, fake)
user, _ := helper.Login(t, claims)
coderd/userauth.go
+14
View File
@@ -1112,6 +1112,20 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
return
}
if idToken.Subject == "" {
logger.Error(ctx, "oauth2: missing 'sub' claim field in OIDC token",
slog.F("source", "id_token"),
slog.F("claim_fields", claimFields(idtokenClaims)),
slog.F("blank", blankFields(idtokenClaims)),
)
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "OIDC token missing 'sub' claim field or 'sub' claim field is empty.",
Detail: "'sub' claim field is required to be unique for all users by a given issue, " +
"an empty field is invalid and this authentication attempt is rejected.",
})
return
}
logger.Debug(ctx, "got oidc claims",
slog.F("source", "id_token"),
slog.F("claim_fields", claimFields(idtokenClaims)),
coderd/userauth_test.go
+41
View File
@@ -72,6 +72,7 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
"email": "alice@coder.com",
"email_verified": true,
"preferred_username": username,
"sub": uuid.NewString(),
}
helper := oidctest.NewLoginHelper(client, fake)
@@ -899,10 +900,19 @@ func TestUserOIDC(t *testing.T) {
IgnoreEmailVerified bool
IgnoreUserInfo bool
}{
{
Name: "NoSub",
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
},
AllowSignups: true,
StatusCode: http.StatusBadRequest,
},
{
Name: "EmailOnly",
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusOK,
@@ -915,6 +925,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": false,
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusForbidden,
@@ -924,6 +935,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": 3.14159,
"email_verified": false,
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusBadRequest,
@@ -933,6 +945,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": false,
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusOK,
@@ -946,6 +959,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"sub": uuid.NewString(),
},
AllowSignups: true,
EmailDomain: []string{
@@ -958,6 +972,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "cian@coder.com",
"email_verified": true,
"sub": uuid.NewString(),
},
AllowSignups: true,
EmailDomain: []string{
@@ -970,6 +985,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"sub": uuid.NewString(),
},
AllowSignups: true,
EmailDomain: []string{
@@ -982,6 +998,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@KWC.io",
"email_verified": true,
"sub": uuid.NewString(),
},
AllowSignups: true,
AssertUser: func(t testing.TB, u codersdk.User) {
@@ -997,6 +1014,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "colin@gmail.com",
"email_verified": true,
"sub": uuid.NewString(),
},
AllowSignups: true,
EmailDomain: []string{
@@ -1015,6 +1033,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"sub": uuid.NewString(),
},
StatusCode: http.StatusForbidden,
},
@@ -1023,6 +1042,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"sub": uuid.NewString(),
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
@@ -1036,6 +1056,7 @@ func TestUserOIDC(t *testing.T) {
"email": "kyle@kwc.io",
"email_verified": true,
"preferred_username": "hotdog",
"sub": uuid.NewString(),
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "hotdog", u.Username)
@@ -1049,6 +1070,7 @@ func TestUserOIDC(t *testing.T) {
"email": "kyle@kwc.io",
"email_verified": true,
"name": "Hot Dog",
"sub": uuid.NewString(),
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "Hot Dog", u.Name)
@@ -1065,6 +1087,7 @@ func TestUserOIDC(t *testing.T) {
// However, we should not fail to log someone in if their name is too long.
// Just truncate it.
"name": strings.Repeat("a", 129),
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusOK,
@@ -1080,6 +1103,7 @@ func TestUserOIDC(t *testing.T) {
// Full names must not have leading or trailing whitespace, but this is a
// daft reason to fail a login.
"name": " Bobby Whitespace ",
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusOK,
@@ -1096,6 +1120,7 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
"name": "Kylium Carbonate",
"preferred_username": "kyle@kwc.io",
"sub": uuid.NewString(),
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
@@ -1108,6 +1133,7 @@ func TestUserOIDC(t *testing.T) {
Name: "UsernameIsEmail",
IDTokenClaims: jwt.MapClaims{
"preferred_username": "kyle@kwc.io",
"sub": uuid.NewString(),
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
@@ -1123,6 +1149,7 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
"preferred_username": "kyle",
"picture": "/example.png",
"sub": uuid.NewString(),
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "/example.png", u.AvatarURL)
@@ -1136,6 +1163,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"sub": uuid.NewString(),
},
UserInfoClaims: jwt.MapClaims{
"preferred_username": "potato",
@@ -1155,6 +1183,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "coolin@coder.com",
"groups": []string{"pingpong"},
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusOK,
@@ -1164,6 +1193,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "internaluser@internal.domain",
"email_verified": false,
"sub": uuid.NewString(),
},
UserInfoClaims: jwt.MapClaims{
"email": "externaluser@external.domain",
@@ -1182,6 +1212,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "internaluser@internal.domain",
"email_verified": false,
"sub": uuid.NewString(),
},
UserInfoClaims: jwt.MapClaims{
"email": 1,
@@ -1197,6 +1228,7 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
"name": "User McName",
"preferred_username": "user",
"sub": uuid.NewString(),
},
UserInfoClaims: jwt.MapClaims{
"email": "user.mcname@external.domain",
@@ -1216,6 +1248,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: inflateClaims(t, jwt.MapClaims{
"email": "user@domain.tld",
"email_verified": true,
"sub": uuid.NewString(),
}, 65536),
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "user", u.Username)
@@ -1228,6 +1261,7 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "user@domain.tld",
"email_verified": true,
"sub": uuid.NewString(),
},
UserInfoClaims: inflateClaims(t, jwt.MapClaims{}, 65536),
AssertUser: func(t testing.TB, u codersdk.User) {
@@ -1242,6 +1276,7 @@ func TestUserOIDC(t *testing.T) {
"iss": "https://mismatch.com",
"email": "user@domain.tld",
"email_verified": true,
"sub": uuid.NewString(),
},
AllowSignups: true,
StatusCode: http.StatusBadRequest,
@@ -1331,6 +1366,7 @@ func TestUserOIDC(t *testing.T) {
client, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
"email": user.Email,
"sub": uuid.NewString(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
@@ -1369,6 +1405,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": userData.Email,
"sub": uuid.NewString(),
}
var err error
user.HTTPClient.Jar, err = cookiejar.New(nil)
@@ -1439,6 +1476,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": userData.Email,
"sub": uuid.NewString(),
}
user.HTTPClient.Jar, err = cookiejar.New(nil)
require.NoError(t, err)
@@ -1509,6 +1547,7 @@ func TestUserOIDC(t *testing.T) {
numLogs := len(auditor.AuditLogs())
claims := jwt.MapClaims{
"email": "jon@coder.com",
"sub": uuid.NewString(),
}
userClient, _ := fake.Login(t, client, claims)
@@ -1629,6 +1668,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": "user@example.com",
"email_verified": true,
"sub": uuid.NewString(),
}
// Perform the login
@@ -1794,6 +1834,7 @@ func TestOIDCSkipIssuer(t *testing.T) {
userClient, _ := fake.Login(t, owner, jwt.MapClaims{
"iss": secondaryURLString,
"email": "alice@coder.com",
"sub": uuid.NewString(),
})
found, err := userClient.User(ctx, "me")
require.NoError(t, err)
coderd/users_test.go
+1
View File
@@ -831,6 +831,7 @@ func TestPostUsers(t *testing.T) {
// Try to log in with OIDC.
userClient, _ := fake.Login(t, client, jwt.MapClaims{
"email": email,
"sub": uuid.NewString(),
})
found, err := userClient.User(ctx, "me")
enterprise/coderd/scim_test.go
+3
View File
@@ -10,6 +10,7 @@ import (
"testing"
"github.com/golang-jwt/jwt/v4"
"github.com/google/uuid"
"github.com/imulab/go-scim/pkg/v2/handlerutil"
"github.com/imulab/go-scim/pkg/v2/spec"
"github.com/stretchr/testify/assert"
@@ -568,6 +569,7 @@ func TestScim(t *testing.T) {
//nolint:bodyclose
scimUserClient, _ := fake.Login(t, client, jwt.MapClaims{
"email": sUser.Emails[0].Value,
"sub": uuid.NewString(),
})
scimUser, err = scimUserClient.User(ctx, codersdk.Me)
require.NoError(t, err)
@@ -836,6 +838,7 @@ func TestScim(t *testing.T) {
//nolint:bodyclose
scimUserClient, _ := fake.Login(t, client, jwt.MapClaims{
"email": sUser.Emails[0].Value,
"sub": uuid.NewString(),
})
scimUser, err = scimUserClient.User(ctx, codersdk.Me)
require.NoError(t, err)
enterprise/coderd/userauth_test.go
+31 -1
View File
@@ -50,6 +50,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": "alice@coder.com",
"sub": uuid.NewString(),
}
// Login a new client that signs up
@@ -82,6 +83,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": "alice@coder.com",
"sub": uuid.NewString(),
}
// Login a new client that signs up
@@ -152,9 +154,11 @@ func TestUserOIDC(t *testing.T) {
require.NoError(t, err)
require.Equal(t, expectedSettings.Field, settings.Field)
sub := uuid.NewString()
claims := jwt.MapClaims{
"email": "alice@coder.com",
"organization": []string{"first", "second"},
"sub": sub,
}
// Then: a new user logs in with claims "second" and "third", they
@@ -169,7 +173,7 @@ func TestUserOIDC(t *testing.T) {
fields, err := runner.AdminClient.GetAvailableIDPSyncFields(ctx)
require.NoError(t, err)
require.ElementsMatch(t, []string{
"aud", "exp", "iss", // Always included from jwt
"sub", "aud", "exp", "iss", // Always included from jwt
"email", "organization",
}, fields)
@@ -204,6 +208,7 @@ func TestUserOIDC(t *testing.T) {
runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
"organization": []string{"second"},
"sub": sub,
})
runner.AssertOrganizations(t, "alice", true, []uuid.UUID{orgTwo.ID})
})
@@ -238,10 +243,12 @@ func TestUserOIDC(t *testing.T) {
})
fourth := dbgen.Organization(t, runner.API.Database, database.Organization{})
sub := uuid.NewString()
ctx := testutil.Context(t, testutil.WaitMedium)
claims := jwt.MapClaims{
"email": "alice@coder.com",
"organization": []string{"second", "third"},
"sub": sub,
}
// Then: a new user logs in with claims "second" and "third", they
@@ -265,6 +272,7 @@ func TestUserOIDC(t *testing.T) {
runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
"organization": []string{"third"},
"sub": sub,
})
runner.AssertOrganizations(t, "alice", false, []uuid.UUID{third})
})
@@ -289,6 +297,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": "alice@coder.com",
"sub": uuid.NewString(),
}
// Login a new client that signs up
client, resp := runner.Login(t, claims)
@@ -328,6 +337,7 @@ func TestUserOIDC(t *testing.T) {
// This is sent as a **string** intentionally instead
// of an array.
"roles": oidcRoleName,
"sub": uuid.NewString(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin().String()})
@@ -398,9 +408,11 @@ func TestUserOIDC(t *testing.T) {
})
// User starts with the owner role
sub := uuid.NewString()
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
"roles": []string{"random", oidcRoleName, rbac.RoleOwner().String()},
"sub": sub,
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String(), rbac.RoleOwner().String()})
@@ -409,6 +421,7 @@ func TestUserOIDC(t *testing.T) {
_, resp = runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
"roles": []string{"random"},
"sub": sub,
})
require.Equal(t, http.StatusOK, resp.StatusCode)
@@ -429,9 +442,11 @@ func TestUserOIDC(t *testing.T) {
},
})
sub := uuid.NewString()
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
"roles": []string{},
"sub": sub,
})
require.Equal(t, http.StatusOK, resp.StatusCode)
// Try to manually update user roles, even though controlled by oidc
@@ -476,6 +491,7 @@ func TestUserOIDC(t *testing.T) {
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{groupName},
"sub": uuid.New(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{groupName})
@@ -510,6 +526,7 @@ func TestUserOIDC(t *testing.T) {
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{oidcGroupName},
"sub": uuid.New(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{coderGroupName})
@@ -546,6 +563,7 @@ func TestUserOIDC(t *testing.T) {
client, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{groupName},
"sub": uuid.New(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{groupName})
@@ -579,9 +597,11 @@ func TestUserOIDC(t *testing.T) {
require.NoError(t, err)
require.Len(t, group.Members, 0)
sub := uuid.NewString()
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{groupName},
"sub": sub,
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{groupName})
@@ -589,6 +609,7 @@ func TestUserOIDC(t *testing.T) {
// Refresh without the group claim
_, resp = runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
"sub": sub,
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{})
@@ -612,6 +633,7 @@ func TestUserOIDC(t *testing.T) {
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{"not-exists"},
"sub": uuid.New(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{})
@@ -637,6 +659,7 @@ func TestUserOIDC(t *testing.T) {
_, resp := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{groupName},
"sub": uuid.New(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{groupName})
@@ -665,6 +688,7 @@ func TestUserOIDC(t *testing.T) {
// This is sent as a **string** intentionally instead
// of an array.
groupClaim: groupName,
"sub": uuid.New(),
})
require.Equal(t, http.StatusOK, resp.StatusCode)
runner.AssertGroups(t, "alice", []string{groupName})
@@ -686,9 +710,11 @@ func TestUserOIDC(t *testing.T) {
})
// Test forbidden
sub := uuid.NewString()
_, resp := runner.AttemptLogin(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{"not-allowed"},
"sub": sub,
})
require.Equal(t, http.StatusForbidden, resp.StatusCode)
@@ -696,6 +722,7 @@ func TestUserOIDC(t *testing.T) {
client, _ := runner.Login(t, jwt.MapClaims{
"email": "alice@coder.com",
groupClaim: []string{allowedGroup},
"sub": sub,
})
ctx := testutil.Context(t, testutil.WaitShort)
@@ -719,6 +746,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": "alice@coder.com",
"sub": uuid.NewString(),
}
// Login a new client that signs up
client, resp := runner.Login(t, claims)
@@ -747,6 +775,7 @@ func TestUserOIDC(t *testing.T) {
claims := jwt.MapClaims{
"email": "alice@coder.com",
"sub": uuid.NewString(),
}
// Login a new client that signs up
client, resp := runner.Login(t, claims)
@@ -921,6 +950,7 @@ func TestGroupSync(t *testing.T) {
require.NoError(t, err, "user must be oidc type")
// Log in the new user
tc.claims["sub"] = uuid.NewString()
tc.claims["email"] = user.Email
_, resp := runner.Login(t, tc.claims)
require.Equal(t, http.StatusOK, resp.StatusCode)