diff --git a/cli/server.go b/cli/server.go index 5018007e2b..097dd9460c 100644 --- a/cli/server.go +++ b/cli/server.go @@ -350,6 +350,11 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. return xerrors.Errorf("access-url must include a scheme (e.g. 'http://' or 'https://)") } + // Cross-field configuration validation after initial parsing. + if err := vals.Validate(); err != nil { + return err + } + // Disable rate limits if the `--dangerous-disable-rate-limits` flag // was specified. loginRateLimit := 60 diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 9c94953239..447ce1ae4f 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -25,6 +25,10 @@ OPTIONS: systemd. This directory is NOT safe to be configured as a shared directory across coderd/provisionerd replicas. + --default-oauth-refresh-lifetime duration, $CODER_DEFAULT_OAUTH_REFRESH_LIFETIME (default: 720h0m0s) + The default lifetime duration for OAuth2 refresh tokens. This controls + how long refresh tokens remain valid after issuance or rotation. + --default-token-lifetime duration, $CODER_DEFAULT_TOKEN_LIFETIME (default: 168h0m0s) The default lifetime duration for API tokens. This value is used when creating a token without specifying a duration, such as when diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index e23274e442..2d28ada458 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -454,6 +454,10 @@ updateCheck: false # IDE plugin. # (default: 168h0m0s, type: duration) defaultTokenLifetime: 168h0m0s +# The default lifetime duration for OAuth2 refresh tokens. This controls how long +# refresh tokens remain valid after issuance or rotation. +# (default: 720h0m0s, type: duration) +defaultOAuthRefreshLifetime: 720h0m0s # Expose the swagger endpoint via /swagger. # (default: , type: bool) enableSwagger: false diff --git a/cli/vpndaemon_darwin.go b/cli/vpndaemon_darwin.go index a1b836dd6b..0e019a728a 100644 --- a/cli/vpndaemon_darwin.go +++ b/cli/vpndaemon_darwin.go @@ -10,7 +10,7 @@ import ( "github.com/coder/serpent" ) -func (r *RootCmd) vpnDaemonRun() *serpent.Command { +func (*RootCmd) vpnDaemonRun() *serpent.Command { var ( rpcReadFD int64 rpcWriteFD int64 diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 9e9d7d5e87..0182c92c2f 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -16424,6 +16424,10 @@ const docTemplate = `{ }, "max_token_lifetime": { "type": "integer" + }, + "refresh_default_duration": { + "description": "RefreshDefaultDuration is the default lifetime for OAuth2 refresh tokens.\nThis should generally be longer than access token lifetimes to allow\nrefreshing after access token expiry.", + "type": "integer" } } }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b550a19438..af2e2d43e2 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -14945,6 +14945,10 @@ }, "max_token_lifetime": { "type": "integer" + }, + "refresh_default_duration": { + "description": "RefreshDefaultDuration is the default lifetime for OAuth2 refresh tokens.\nThis should generally be longer than access token lifetimes to allow\nrefreshing after access token expiry.", + "type": "integer" } } }, diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index 04ce3d7519..d5755b695d 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -20,6 +20,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/oauth2provider" @@ -27,6 +28,7 @@ import ( "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" + "github.com/coder/serpent" ) func TestOAuth2ProviderApps(t *testing.T) { @@ -1184,6 +1186,71 @@ func TestOAuth2ProviderCrossResourceAudienceValidation(t *testing.T) { // For now, this verifies the basic token flow works correctly } +// TestOAuth2RefreshExpiryOutlivesAccess verifies that refresh token expiry is +// greater than the provisioned access token (API key) expiry per configuration. +func TestOAuth2RefreshExpiryOutlivesAccess(t *testing.T) { + t.Parallel() + + // Set explicit lifetimes to make comparison deterministic. + db, pubsub := dbtestutil.NewDB(t) + dv := coderdtest.DeploymentValues(t, func(d *codersdk.DeploymentValues) { + d.Sessions.DefaultDuration = serpent.Duration(1 * time.Hour) + d.Sessions.RefreshDefaultDuration = serpent.Duration(48 * time.Hour) + }) + ownerClient := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + DeploymentValues: dv, + }) + _ = coderdtest.CreateFirstUser(t, ownerClient) + ctx := testutil.Context(t, testutil.WaitLong) + + // Create app and secret + // Keep suffix short to satisfy name validation (<=32 chars, alnum + hyphens). + apps := generateApps(ctx, t, ownerClient, "ref-exp") + //nolint:gocritic // Owner permission required for app secret creation + secret, err := ownerClient.PostOAuth2ProviderAppSecret(ctx, apps.Default.ID) + require.NoError(t, err) + + cfg := &oauth2.Config{ + ClientID: apps.Default.ID.String(), + ClientSecret: secret.ClientSecretFull, + Endpoint: oauth2.Endpoint{ + AuthURL: apps.Default.Endpoints.Authorization, + DeviceAuthURL: apps.Default.Endpoints.DeviceAuth, + TokenURL: apps.Default.Endpoints.Token, + AuthStyle: oauth2.AuthStyleInParams, + }, + RedirectURL: apps.Default.CallbackURL, + Scopes: []string{}, + } + + // Authorization and token exchange + code, err := authorizationFlow(ctx, ownerClient, cfg) + require.NoError(t, err) + tok, err := cfg.Exchange(ctx, code) + require.NoError(t, err) + require.NotEmpty(t, tok.AccessToken) + require.NotEmpty(t, tok.RefreshToken) + + // Parse refresh token prefix (coder__) + parts := strings.Split(tok.RefreshToken, "_") + require.Len(t, parts, 3) + prefix := parts[1] + + // Look up refresh token row and associated API key + dbToken, err := db.GetOAuth2ProviderAppTokenByPrefix(dbauthz.AsSystemRestricted(ctx), []byte(prefix)) + require.NoError(t, err) + apiKey, err := db.GetAPIKeyByID(dbauthz.AsSystemRestricted(ctx), dbToken.APIKeyID) + require.NoError(t, err) + + // Assert refresh token expiry is strictly after access token expiry + require.Truef(t, dbToken.ExpiresAt.After(apiKey.ExpiresAt), + "expected refresh expiry %s to be after access expiry %s", + dbToken.ExpiresAt, apiKey.ExpiresAt, + ) +} + // customTokenExchange performs a custom OAuth2 token exchange with support for resource parameter // This is needed because golang.org/x/oauth2 doesn't support custom parameters in token requests func customTokenExchange(ctx context.Context, baseURL, clientID, clientSecret, code, redirectURI, resource string) (*oauth2.Token, error) { diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index afbc27dd8b..ac29d26ea5 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -92,9 +92,8 @@ func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []c } // Tokens -// TODO: the sessions lifetime config passed is for coder api tokens. -// Should there be a separate config for oauth2 tokens? They are related, -// but they are not the same. +// Uses Sessions.DefaultDuration for access token (API key) TTL and +// Sessions.RefreshDefaultDuration for refresh token TTL. func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerFunc { return func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -280,6 +279,13 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database } // Do the actual token exchange in the database. + // Determine refresh token expiry independently from the access token. + refreshLifetime := lifetimes.RefreshDefaultDuration.Value() + if refreshLifetime == 0 { + refreshLifetime = lifetimes.DefaultDuration.Value() + } + refreshExpiresAt := dbtime.Now().Add(refreshLifetime) + err = db.InTx(func(tx database.Store) error { ctx := dbauthz.As(ctx, actor) err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, dbCode.ID) @@ -307,7 +313,7 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database _, err = tx.InsertOAuth2ProviderAppToken(ctx, database.InsertOAuth2ProviderAppTokenParams{ ID: uuid.New(), CreatedAt: dbtime.Now(), - ExpiresAt: key.ExpiresAt, + ExpiresAt: refreshExpiresAt, HashPrefix: []byte(refreshToken.Prefix), RefreshHash: []byte(refreshToken.Hashed), AppSecretID: dbSecret.ID, @@ -401,6 +407,13 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut } // Replace the token. + // Determine refresh token expiry independently from the access token. + refreshLifetime := lifetimes.RefreshDefaultDuration.Value() + if refreshLifetime == 0 { + refreshLifetime = lifetimes.DefaultDuration.Value() + } + refreshExpiresAt := dbtime.Now().Add(refreshLifetime) + err = db.InTx(func(tx database.Store) error { ctx := dbauthz.As(ctx, actor) err = tx.DeleteAPIKeyByID(ctx, prevKey.ID) // This cascades to the token. @@ -416,7 +429,7 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut _, err = tx.InsertOAuth2ProviderAppToken(ctx, database.InsertOAuth2ProviderAppTokenParams{ ID: uuid.New(), CreatedAt: dbtime.Now(), - ExpiresAt: key.ExpiresAt, + ExpiresAt: refreshExpiresAt, HashPrefix: []byte(refreshToken.Prefix), RefreshHash: []byte(refreshToken.Hashed), AppSecretID: dbToken.AppSecretID, diff --git a/codersdk/deployment.go b/codersdk/deployment.go index a70a6b5550..2284e0266e 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -566,6 +566,11 @@ type SessionLifetime struct { // DefaultDuration is only for browser, workspace app and oauth sessions. DefaultDuration serpent.Duration `json:"default_duration" typescript:",notnull"` + // RefreshDefaultDuration is the default lifetime for OAuth2 refresh tokens. + // This should generally be longer than access token lifetimes to allow + // refreshing after access token expiry. + RefreshDefaultDuration serpent.Duration `json:"refresh_default_duration,omitempty" typescript:",notnull"` + DefaultTokenDuration serpent.Duration `json:"default_token_lifetime,omitempty" typescript:",notnull"` MaximumTokenDuration serpent.Duration `json:"max_token_lifetime,omitempty" typescript:",notnull"` @@ -2464,6 +2469,16 @@ func (c *DeploymentValues) Options() serpent.OptionSet { YAML: "defaultTokenLifetime", Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), }, + { + Name: "Default OAuth Refresh Lifetime", + Description: "The default lifetime duration for OAuth2 refresh tokens. This controls how long refresh tokens remain valid after issuance or rotation.", + Flag: "default-oauth-refresh-lifetime", + Env: "CODER_DEFAULT_OAUTH_REFRESH_LIFETIME", + Default: (30 * 24 * time.Hour).String(), + Value: &c.Sessions.RefreshDefaultDuration, + YAML: "defaultOAuthRefreshLifetime", + Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"), + }, { Name: "Enable swagger endpoint", Description: "Expose the swagger endpoint via /swagger.", @@ -3223,6 +3238,30 @@ type LinkConfig struct { Icon string `json:"icon" yaml:"icon" enums:"bug,chat,docs"` } +// Validate checks cross-field constraints for deployment values. +// It must be called after all values are loaded from flags/env/YAML. +func (c *DeploymentValues) Validate() error { + // For OAuth2, access tokens (API keys) issued via the authorization code/refresh flows + // use Sessions.DefaultDuration as their lifetime, while refresh tokens use + // Sessions.RefreshDefaultDuration (falling back to DefaultDuration when set to 0). + // Enforce that refresh token lifetime is strictly greater than the access token lifetime. + access := c.Sessions.DefaultDuration.Value() + refresh := c.Sessions.RefreshDefaultDuration.Value() + + // Check if values appear uninitialized + if access == 0 { + return xerrors.New("developer error: sessions configuration appears uninitialized - ensure all values are loaded before validation") + } + + if refresh <= access { + return xerrors.Errorf( + "default OAuth refresh lifetime (%s) must be strictly greater than session duration (%s); set --default-oauth-refresh-lifetime to a value greater than --session-duration", + refresh, access, + ) + } + return nil +} + // DeploymentOptionsWithoutSecrets returns a copy of the OptionSet with secret values omitted. func DeploymentOptionsWithoutSecrets(set serpent.OptionSet) serpent.OptionSet { cpy := make(serpent.OptionSet, 0, len(set)) diff --git a/codersdk/deployment_test.go b/codersdk/deployment_test.go index fcddab0a53..c113d46cc5 100644 --- a/codersdk/deployment_test.go +++ b/codersdk/deployment_test.go @@ -292,6 +292,57 @@ func must[T any](value T, err error) T { return value } +func TestDeploymentValues_Validate_RefreshLifetime(t *testing.T) { + t.Parallel() + + mk := func(access, refresh time.Duration) *codersdk.DeploymentValues { + dv := &codersdk.DeploymentValues{} + dv.Sessions.DefaultDuration = serpent.Duration(access) + dv.Sessions.RefreshDefaultDuration = serpent.Duration(refresh) + return dv + } + + t.Run("EqualDurations_Error", func(t *testing.T) { + t.Parallel() + dv := mk(1*time.Hour, 1*time.Hour) + err := dv.Validate() + require.Error(t, err) + require.ErrorContains(t, err, "must be strictly greater") + }) + + t.Run("RefreshShorter_Error", func(t *testing.T) { + t.Parallel() + dv := mk(2*time.Hour, 1*time.Hour) + err := dv.Validate() + require.Error(t, err) + require.ErrorContains(t, err, "must be strictly greater") + }) + + t.Run("RefreshZero_Error", func(t *testing.T) { + t.Parallel() + dv := mk(1*time.Hour, 0) + err := dv.Validate() + require.Error(t, err) + require.ErrorContains(t, err, "must be strictly greater") + }) + + t.Run("AccessUninitialized_Error", func(t *testing.T) { + t.Parallel() + // Access duration is zero (uninitialized); refresh is valid. + dv := mk(0, 48*time.Hour) + err := dv.Validate() + require.Error(t, err) + require.ErrorContains(t, err, "developer error: sessions configuration appears uninitialized") + }) + + t.Run("RefreshLonger_OK", func(t *testing.T) { + t.Parallel() + dv := mk(1*time.Hour, 48*time.Hour) + err := dv.Validate() + require.NoError(t, err) + }) +} + func TestDeploymentValues_DurationFormatNanoseconds(t *testing.T) { t.Parallel() diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 72543f6774..569ef067d8 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -442,7 +442,8 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "default_token_lifetime": 0, "disable_expiry_refresh": true, "max_admin_token_lifetime": 0, - "max_token_lifetime": 0 + "max_token_lifetime": 0, + "refresh_default_duration": 0 }, "ssh_keygen_algorithm": "string", "strict_transport_security": 0, diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index b3959ceafa..df8aac2115 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -2466,7 +2466,8 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "default_token_lifetime": 0, "disable_expiry_refresh": true, "max_admin_token_lifetime": 0, - "max_token_lifetime": 0 + "max_token_lifetime": 0, + "refresh_default_duration": 0 }, "ssh_keygen_algorithm": "string", "strict_transport_security": 0, @@ -2953,7 +2954,8 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "default_token_lifetime": 0, "disable_expiry_refresh": true, "max_admin_token_lifetime": 0, - "max_token_lifetime": 0 + "max_token_lifetime": 0, + "refresh_default_duration": 0 }, "ssh_keygen_algorithm": "string", "strict_transport_security": 0, @@ -6867,19 +6869,21 @@ Only certain features set these fields: - FeatureManagedAgentLimit| "default_token_lifetime": 0, "disable_expiry_refresh": true, "max_admin_token_lifetime": 0, - "max_token_lifetime": 0 + "max_token_lifetime": 0, + "refresh_default_duration": 0 } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -|----------------------------|---------|----------|--------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `default_duration` | integer | false | | Default duration is only for browser, workspace app and oauth sessions. | -| `default_token_lifetime` | integer | false | | | -| `disable_expiry_refresh` | boolean | false | | Disable expiry refresh will disable automatically refreshing api keys when they are used from the api. This means the api key lifetime at creation is the lifetime of the api key. | -| `max_admin_token_lifetime` | integer | false | | | -| `max_token_lifetime` | integer | false | | | +| Name | Type | Required | Restrictions | Description | +|----------------------------|---------|----------|--------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `default_duration` | integer | false | | Default duration is only for browser, workspace app and oauth sessions. | +| `default_token_lifetime` | integer | false | | | +| `disable_expiry_refresh` | boolean | false | | Disable expiry refresh will disable automatically refreshing api keys when they are used from the api. This means the api key lifetime at creation is the lifetime of the api key. | +| `max_admin_token_lifetime` | integer | false | | | +| `max_token_lifetime` | integer | false | | | +| `refresh_default_duration` | integer | false | | Refresh default duration is the default lifetime for OAuth2 refresh tokens. This should generally be longer than access token lifetimes to allow refreshing after access token expiry. | ## codersdk.SlimRole diff --git a/docs/reference/cli/server.md b/docs/reference/cli/server.md index 8d601cace5..bdc424bdd7 100644 --- a/docs/reference/cli/server.md +++ b/docs/reference/cli/server.md @@ -932,6 +932,17 @@ The maximum lifetime duration administrators can specify when creating an API to The default lifetime duration for API tokens. This value is used when creating a token without specifying a duration, such as when authenticating the CLI or an IDE plugin. +### --default-oauth-refresh-lifetime + +| | | +|-------------|----------------------------------------------------| +| Type | duration | +| Environment | $CODER_DEFAULT_OAUTH_REFRESH_LIFETIME | +| YAML | defaultOAuthRefreshLifetime | +| Default | 720h0m0s | + +The default lifetime duration for OAuth2 refresh tokens. This controls how long refresh tokens remain valid after issuance or rotation. + ### --swagger-enable | | | diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index e86cb52692..162d4214cc 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -26,6 +26,10 @@ OPTIONS: systemd. This directory is NOT safe to be configured as a shared directory across coderd/provisionerd replicas. + --default-oauth-refresh-lifetime duration, $CODER_DEFAULT_OAUTH_REFRESH_LIFETIME (default: 720h0m0s) + The default lifetime duration for OAuth2 refresh tokens. This controls + how long refresh tokens remain valid after issuance or rotation. + --default-token-lifetime duration, $CODER_DEFAULT_TOKEN_LIFETIME (default: 168h0m0s) The default lifetime duration for API tokens. This value is used when creating a token without specifying a duration, such as when diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ccbe5924b5..4314d00ef4 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2741,6 +2741,7 @@ export interface SessionCountDeploymentStats { export interface SessionLifetime { readonly disable_expiry_refresh?: boolean; readonly default_duration: number; + readonly refresh_default_duration?: number; readonly default_token_lifetime?: number; readonly max_token_lifetime?: number; readonly max_admin_token_lifetime?: number;