Skip to content

Commit 9eca480

Browse files
BobbyHoclaude
BobbyHo
and
claude
committed
feat: support public (secretless, PKCE-only) OAuth2 clients
An RFC 7591 registration requesting `token_endpoint_auth_method: "none"` now produces a public client: no secret is minted, `client_type` is persisted as `public`, and the token endpoint accepts that client's authorization_code exchange with PKCE alone. Discovery advertises "none" as a supported auth method. PKCE was already mandatory for every authorization_code flow, so public clients inherit it unchanged. That makes the code ownership check (`dbCode.AppID != app.ID`) the sole binding between the exchange and the app named by `client_id` for a public client, where it was defense in depth for confidential ones. It is retained and covered with a public client, along with the refresh and revocation paths, which are the first to see a NULL `app_secret_id`. The RFC 7636 §4.1 code_verifier length floor added in #28003 is exercised here against a public client specifically, since for that client type PKCE is the only client authentication and a later change that scoped the check to confidential clients only would leave public clients silently unprotected. Registration writes the app and its secret in one transaction. Two independently committed inserts could leave a permanently committed app that can never authenticate while still holding a registration access token. An RFC 7592 update can no longer move a client between public and confidential, and secret creation is rejected for a public app: the token endpoint would never validate that secret, and deleting it revokes nothing because a public client's tokens carry a NULL `app_secret_id` instead of cascading from the secret. Clients registered with "none" before it was honored are stored confidential and still require their secret, so an update that resends their own metadata is accepted rather than rejected, and the auth method reported back is the one the server actually enforces. Depends on #27931 for the client_type constraint and the schema-level alignment of the two columns, and on #28003 for the RFC 7636 §4.1 code_verifier length floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent eea4094 commit 9eca480

25 files changed

Lines changed: 1368 additions & 156 deletions

‎coderd/apidoc/docs.go‎

Lines changed: 7 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json‎

Lines changed: 7 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/constants.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,23 @@ import (
1010
// for use as a uuid.UUID. Both must agree; tests pin the value to the
1111
// codersdk constant so the two cannot drift.
1212
var PrebuildsSystemUserID = uuid.MustParse(codersdk.PrebuildsSystemUserID)
13+
14+
// Values stored in oauth2_provider_apps.client_type, as plain strings for
15+
// comparison against the nullable column.
16+
//
17+
// Converted from the codersdk constants rather than redeclared, so the value
18+
// registration writes and the value OAuth2ProviderApp.IsPublic reads back
19+
// cannot disagree. That divergence would fail closed anyway (the app would read
20+
// as confidential and demand a secret it was never issued), but it would fail
21+
// visibly to a client rather than here.
22+
//
23+
// What this does not protect against is the two constants colliding on the same
24+
// value, which would make IsPublic true for confidential apps. Nothing in the
25+
// type system can catch that; the tests that pin these spellings to the wire
26+
// values do, so do not delete them as redundant:
27+
// TestOAuth2ClientRegistrationRequest_DetermineClientType and
28+
// TestCreateDynamicClientRegistration_ClientType.
29+
const (
30+
OAuth2ProviderAppClientTypeConfidential = string(codersdk.OAuth2ClientTypeConfidential)
31+
OAuth2ProviderAppClientTypePublic = string(codersdk.OAuth2ClientTypePublic)
32+
)

‎coderd/database/dbgen/dbgen.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1733,7 +1733,7 @@ func OAuth2ProviderApp(t testing.TB, db database.Store, seed database.OAuth2Prov
17331733
Icon: takeFirst(seed.Icon, ""),
17341734
CallbackURL: takeFirst(seed.CallbackURL, "http://localhost"),
17351735
RedirectUris: takeFirstSlice(seed.RedirectUris, []string{}),
1736-
ClientType: takeFirst(seed.ClientType, "confidential"),
1736+
ClientType: takeFirst(seed.ClientType, database.OAuth2ProviderAppClientTypeConfidential),
17371737
DynamicallyRegistered: takeFirst(seed.DynamicallyRegistered, sql.NullBool{Bool: false, Valid: true}),
17381738
ClientIDIssuedAt: takeFirst(seed.ClientIDIssuedAt, sql.NullTime{}),
17391739
ClientSecretExpiresAt: takeFirst(seed.ClientSecretExpiresAt, sql.NullTime{}),

‎coderd/database/modelmethods.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,6 +685,14 @@ func (OAuth2ProviderApp) RBACObject() rbac.Object {
685685
return rbac.ResourceOauth2App
686686
}
687687

688+
// IsPublic reports whether the app is a public (secretless, PKCE-only)
689+
// OAuth2 client per RFC 7591 §2 / OAuth 2.1 §2.1, as opposed to confidential.
690+
// An unset or unrecognized client type reads as confidential, so an app can
691+
// never skip client authentication by accident.
692+
func (a OAuth2ProviderApp) IsPublic() bool {
693+
return a.ClientType == OAuth2ProviderAppClientTypePublic
694+
}
695+
688696
func (a GetOAuth2ProviderAppsByUserIDRow) RBACObject() rbac.Object {
689697
return a.OAuth2ProviderApp.RBACObject()
690698
}

‎coderd/database/modelmethods_internal_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,37 @@ func TestWorkspaceACLDisabled(t *testing.T) {
221221
})
222222
}
223223

224+
// TestOAuth2ProviderAppIsPublic pins IsPublic's contract directly, since it is
225+
// what decides whether the token endpoint validates a client secret at all.
226+
// Only the exact string "public" may read as public: anything else, including
227+
// an unset column or a differently-cased value, must read as confidential so
228+
// that a garbled value cannot silently skip client authentication.
229+
func TestOAuth2ProviderAppIsPublic(t *testing.T) {
230+
t.Parallel()
231+
232+
tests := []struct {
233+
clientType string
234+
want bool
235+
}{
236+
{clientType: "public", want: true},
237+
{clientType: "confidential", want: false},
238+
{clientType: "", want: false},
239+
{clientType: "Public", want: false},
240+
{clientType: "PUBLIC", want: false},
241+
{clientType: " public", want: false},
242+
{clientType: "public ", want: false},
243+
{clientType: "bogus", want: false},
244+
}
245+
246+
for _, tt := range tests {
247+
t.Run(tt.clientType, func(t *testing.T) {
248+
t.Parallel()
249+
app := OAuth2ProviderApp{ClientType: tt.clientType}
250+
require.Equal(t, tt.want, app.IsPublic())
251+
})
252+
}
253+
}
254+
224255
// Helpers
225256
func requirePermission(t *testing.T, s rbac.Scope, resource string, action policy.Action) {
226257
t.Helper()

‎coderd/oauth2.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,9 @@ func (api *API) postOAuth2ProviderAppAuthorize() http.HandlerFunc {
147147
// @Produce json
148148
// @Tags Enterprise
149149
// @Param client_id formData string false "Client ID, required if grant_type=authorization_code"
150-
// @Param client_secret formData string false "Client secret, required if grant_type=authorization_code"
150+
// @Param client_secret formData string false "Client secret, required if grant_type=authorization_code and the client is confidential. Public clients (token_endpoint_auth_method=none) send no secret."
151151
// @Param code formData string false "Authorization code, required if grant_type=authorization_code"
152+
// @Param code_verifier formData string false "PKCE code verifier, required if grant_type=authorization_code. 43-128 characters per RFC 7636. This is the only client authentication a public client has."
152153
// @Param refresh_token formData string false "Refresh token, required if grant_type=refresh_token"
153154
// @Param grant_type formData codersdk.OAuth2ProviderGrantType true "Grant type"
154155
// @Success 200 {object} oauth2.Token

‎coderd/oauth2_test.go‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/coder/coder/v2/coderd/database/dbauthz"
2626
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2727
"github.com/coder/coder/v2/coderd/database/dbtime"
28+
"github.com/coder/coder/v2/coderd/httpmw"
2829
"github.com/coder/coder/v2/coderd/oauth2provider"
2930
"github.com/coder/coder/v2/coderd/oauth2provider/oauth2providertest"
3031
"github.com/coder/coder/v2/coderd/userpassword"
@@ -611,6 +612,89 @@ func TestOAuth2ProviderTokenExchangeCodeBelongsToDifferentApp(t *testing.T) {
611612
require.ErrorContains(t, err, "The authorization code is invalid or expired")
612613
}
613614

615+
// TestOAuth2ProviderTokenExchangePublicClientCodeBelongsToDifferentApp is the
616+
// public-client counterpart to the test above. A public client presents no
617+
// secret, so the secret-ownership check never runs and the code-ownership
618+
// check is the only thing binding the exchange to the app identified by
619+
// client_id. Two public clients sharing a redirect URI is the common case for
620+
// native apps, which makes this the scenario the check has to hold in.
621+
func TestOAuth2ProviderTokenExchangePublicClientCodeBelongsToDifferentApp(t *testing.T) {
622+
t.Parallel()
623+
624+
ownerClient := coderdtest.New(t, nil)
625+
owner := coderdtest.CreateFirstUser(t, ownerClient)
626+
oauth2providertest.EnableDCR(t, ownerClient)
627+
ctx := testutil.Context(t, testutil.WaitLong)
628+
629+
// Both apps are public, so neither has a secret and the code-ownership
630+
// check is the only thing binding the exchange to an app.
631+
const sharedCallback = "http://localhost:8080/callback"
632+
appA := oauth2providertest.RegisterPublicClient(ctx, t, ownerClient, "public-code-owner", sharedCallback)
633+
appB := oauth2providertest.RegisterPublicClient(ctx, t, ownerClient, "public-code-thief", sharedCallback)
634+
635+
userClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
636+
637+
authURL := ownerClient.URL.JoinPath("/oauth2/authorize").String()
638+
tokenURL := ownerClient.URL.JoinPath("/oauth2/tokens").String()
639+
640+
cfgA := &oauth2.Config{
641+
ClientID: appA.ClientID,
642+
Endpoint: oauth2.Endpoint{
643+
AuthURL: authURL,
644+
TokenURL: tokenURL,
645+
AuthStyle: oauth2.AuthStyleInParams,
646+
},
647+
RedirectURL: sharedCallback,
648+
Scopes: []string{},
649+
}
650+
code, verifier, err := authorizationFlow(ctx, userClient, cfgA)
651+
require.NoError(t, err)
652+
653+
// Redeem appA's code under appB's client_id, with no secret and a valid
654+
// PKCE verifier. Everything except the code's own app_id lines up.
655+
cfgB := &oauth2.Config{
656+
ClientID: appB.ClientID,
657+
Endpoint: oauth2.Endpoint{
658+
TokenURL: tokenURL,
659+
AuthStyle: oauth2.AuthStyleInParams,
660+
},
661+
RedirectURL: sharedCallback,
662+
Scopes: []string{},
663+
}
664+
_, err = cfgB.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", verifier))
665+
require.Error(t, err)
666+
require.ErrorContains(t, err, "The authorization code is invalid or expired")
667+
668+
// PKCE is the only client authentication a public client has, so exercise
669+
// its failure branches here rather than only on confidential apps. The
670+
// verification currently sits outside the if !isPublic block, so a later
671+
// change that moved it inside would leave every confidential test passing
672+
// while public clients lost authentication entirely.
673+
//
674+
// A rejected verifier must also leave the code unconsumed, otherwise one
675+
// bad guess would deny the legitimate client its own code.
676+
_, err = cfgA.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", ""))
677+
require.Error(t, err)
678+
require.ErrorContains(t, err, "The PKCE code verifier is invalid")
679+
680+
wrongVerifier, _ := oauth2providertest.GeneratePKCE(t)
681+
_, err = cfgA.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", wrongVerifier))
682+
require.Error(t, err)
683+
require.ErrorContains(t, err, "The PKCE code verifier is invalid")
684+
685+
// RFC 7636 §4.1 sets a 43-character floor. A one-character verifier hashes
686+
// to a well-formed challenge, so only a length check refuses it.
687+
_, err = cfgA.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", "a"))
688+
require.Error(t, err)
689+
require.ErrorContains(t, err, "The PKCE code verifier is invalid")
690+
691+
// The rejections must not have consumed the code: appA can still redeem
692+
// its own code afterwards.
693+
token, err := cfgA.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", verifier))
694+
require.NoError(t, err)
695+
require.NotEmpty(t, token.AccessToken)
696+
}
697+
614698
func TestOAuth2ProviderTokenRefresh(t *testing.T) {
615699
t.Parallel()
616700
ctx := testutil.Context(t, testutil.WaitLong)
@@ -1044,6 +1128,129 @@ func TestOAuth2ProviderRevokeCrossApp(t *testing.T) {
10441128
}
10451129
}
10461130

1131+
// TestOAuth2PublicClientTokenLifecycle exercises refresh and revocation for a
1132+
// public client, whose tokens are the first with a NULL app_secret_id.
1133+
//
1134+
// The confidential-client tests all mint tokens with a real secret, so the
1135+
// refresh path that carries dbToken.AppSecretID forward and the two revocation
1136+
// ownership checks in revoke.go were only ever run against a non-NULL value.
1137+
// This PR's premise is that revocation ownership moved onto app_id precisely so
1138+
// a secretless token can still be revoked, and that claim needs a test. Without
1139+
// one, reintroducing a join through app_secret_id would break public clients
1140+
// only, and the suite would stay green.
1141+
func TestOAuth2PublicClientTokenLifecycle(t *testing.T) {
1142+
t.Parallel()
1143+
1144+
tests := []struct {
1145+
name string
1146+
// tokenFor selects which token to revoke, covering both
1147+
// revokeRefreshTokenInTx and revokeAPIKeyInTx.
1148+
tokenFor func(*oauth2.Token) string
1149+
}{
1150+
{
1151+
name: "AccessToken",
1152+
tokenFor: func(tok *oauth2.Token) string { return tok.AccessToken },
1153+
},
1154+
{
1155+
name: "RefreshToken",
1156+
tokenFor: func(tok *oauth2.Token) string { return tok.RefreshToken },
1157+
},
1158+
}
1159+
1160+
for _, test := range tests {
1161+
t.Run(test.name, func(t *testing.T) {
1162+
t.Parallel()
1163+
ctx := testutil.Context(t, testutil.WaitLong)
1164+
1165+
db, pubsub := dbtestutil.NewDB(t)
1166+
ownerClient := coderdtest.New(t, &coderdtest.Options{
1167+
Database: db,
1168+
Pubsub: pubsub,
1169+
})
1170+
owner := coderdtest.CreateFirstUser(t, ownerClient)
1171+
oauth2providertest.EnableDCR(t, ownerClient)
1172+
1173+
const callback = "http://localhost:8080/callback"
1174+
app := oauth2providertest.RegisterPublicClient(ctx, t, ownerClient, "public-lifecycle", callback)
1175+
otherApp := oauth2providertest.RegisterPublicClient(ctx, t, ownerClient, "public-lifecycle-other", callback)
1176+
1177+
appID, err := uuid.Parse(app.ClientID)
1178+
require.NoError(t, err)
1179+
otherAppID, err := uuid.Parse(otherApp.ClientID)
1180+
require.NoError(t, err)
1181+
1182+
userClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
1183+
1184+
cfg := &oauth2.Config{
1185+
ClientID: app.ClientID,
1186+
Endpoint: oauth2.Endpoint{
1187+
AuthURL: ownerClient.URL.JoinPath("/oauth2/authorize").String(),
1188+
TokenURL: ownerClient.URL.JoinPath("/oauth2/tokens").String(),
1189+
AuthStyle: oauth2.AuthStyleInParams,
1190+
},
1191+
RedirectURL: callback,
1192+
Scopes: []string{},
1193+
}
1194+
1195+
code, verifier, err := authorizationFlow(ctx, userClient, cfg)
1196+
require.NoError(t, err)
1197+
token, err := cfg.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", verifier))
1198+
require.NoError(t, err)
1199+
require.NotEmpty(t, token.RefreshToken)
1200+
1201+
// Confirm the precondition this test exists for: the minted row
1202+
// really does have a NULL app_secret_id, with app_id carrying the
1203+
// ownership that revocation depends on.
1204+
assertSecretlessToken := func(accessToken string) {
1205+
t.Helper()
1206+
keyID, _, err := httpmw.SplitAPIToken(accessToken)
1207+
require.NoError(t, err)
1208+
// This is the raw store handle, not the dbauthz-wrapped one the
1209+
// server uses, so no authorization layer is in the path and no
1210+
// system actor is needed.
1211+
dbToken, err := db.GetOAuth2ProviderAppTokenByAPIKeyID(ctx, keyID)
1212+
require.NoError(t, err)
1213+
require.False(t, dbToken.AppSecretID.Valid, "public client token must have a NULL app_secret_id")
1214+
require.Equal(t, appID, dbToken.AppID)
1215+
}
1216+
assertSecretlessToken(token.AccessToken)
1217+
1218+
// Refresh carries AppSecretID forward untouched, so the refreshed
1219+
// row must still be secretless and still owned by the same app.
1220+
refreshCfg := *cfg
1221+
refreshed, err := refreshCfg.TokenSource(ctx, &oauth2.Token{
1222+
RefreshToken: token.RefreshToken,
1223+
Expiry: time.Now().Add(-time.Hour),
1224+
}).Token()
1225+
require.NoError(t, err)
1226+
require.NotEmpty(t, refreshed.AccessToken)
1227+
assertSecretlessToken(refreshed.AccessToken)
1228+
1229+
sessionWorks := func() bool {
1230+
checkClient := codersdk.New(userClient.URL)
1231+
checkClient.SetSessionToken(refreshed.AccessToken)
1232+
_, err := checkClient.User(ctx, codersdk.Me)
1233+
return err == nil
1234+
}
1235+
require.True(t, sessionWorks(), "refreshed public-client session should be valid")
1236+
1237+
tokenUnderTest := test.tokenFor(refreshed)
1238+
1239+
// A different app must not be able to revoke it, and per RFC 7009
1240+
// must not learn that it exists.
1241+
err = userClient.RevokeOAuth2Token(ctx, otherAppID, tokenUnderTest)
1242+
require.NoError(t, err, "cross-app revoke must appear to succeed per RFC 7009")
1243+
require.True(t, sessionWorks(), "cross-app revoke must not end the session")
1244+
1245+
// The issuing app must be able to revoke it, with no secret to join
1246+
// through. This is the claim app_id was promoted for.
1247+
err = userClient.RevokeOAuth2Token(ctx, appID, tokenUnderTest)
1248+
require.NoError(t, err)
1249+
require.False(t, sessionWorks(), "public client must be able to revoke its own token")
1250+
})
1251+
}
1252+
}
1253+
10471254
type provisionedApps struct {
10481255
Default codersdk.OAuth2ProviderApp
10491256
NoPort codersdk.OAuth2ProviderApp

‎coderd/oauth2provider/app_secrets.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,21 @@ func CreateAppSecret(db database.Store, auditor *audit.Auditor, logger slog.Logg
5353
})
5454
)
5555
defer commitAudit()
56+
57+
// A public client authenticates with PKCE alone, so the token endpoint
58+
// never validates a secret for one. Minting a secret anyway would hand
59+
// an operator a credential that does nothing, and worse, one whose
60+
// deletion looks like a kill switch: for a confidential app deleting a
61+
// secret cascades its tokens away, but a public client's tokens carry a
62+
// NULL app_secret_id, so deleting it revokes nothing.
63+
if app.IsPublic() {
64+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
65+
Message: "Cannot create a client secret for a public OAuth2 app.",
66+
Detail: "Public clients authenticate with PKCE and have no client secret. The client type is fixed at registration.",
67+
})
68+
return
69+
}
70+
5671
secret, err := GenerateSecret()
5772
if err != nil {
5873
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{

‎coderd/oauth2provider/apps.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ func CreateApp(db database.Store, accessURL *url.URL, auditor *audit.Auditor, lo
9292
Icon: req.Icon,
9393
CallbackURL: req.CallbackURL,
9494
RedirectUris: []string{},
95-
ClientType: "confidential",
95+
ClientType: database.OAuth2ProviderAppClientTypeConfidential,
9696
DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true},
9797
ClientIDIssuedAt: sql.NullTime{},
9898
ClientSecretExpiresAt: sql.NullTime{},

0 commit comments

Comments
 (0)