diff --git a/docs/auth-and-profiles.md b/docs/auth-and-profiles.md index bc44dba..1d308ee 100644 --- a/docs/auth-and-profiles.md +++ b/docs/auth-and-profiles.md @@ -22,9 +22,12 @@ A profile signs in one of two ways: `--api-client` to be prompted for them. This is the path for CI and scripts, because it needs no browser and the token renews itself. -A browser session lasts until the time login prints. After that, -commands fail with exit status 5 and name the login command to run -again. A profile holds one kind of credential, so signing in one way +A browser session renews itself while you use it. It ends after a +period without use, or after a fixed maximum however much you use +it. Login keeps what renews it in the OS keychain; +where no keychain can be used, the session lasts until the time login +prints. Once the session ends, commands fail with exit status 5 and +name the login command, which starts a new session. A profile holds one kind of credential, so signing in one way removes what the other way stored. When you belong to several tenants, login asks which one to use, or @@ -225,8 +228,11 @@ never silently runs as a previous credential. `pgedge starfleet auth logout` removes the active profile's cached token file (and any staging file an interrupted write left beside it), and deletes the stored `client_id` and `client_secret` from the profile -and the secret from the OS keychain. Other profiles' tokens and -credentials are left alone. +and the secret from the OS keychain. For a browser session, logout +first asks pgEdge to end the session, then removes it from the OS +keychain; if that request fails, logout warns and still removes the +session from this machine. Other profiles' tokens and credentials are +left alone. `pgedge starfleet auth status` reports the resolution state without making a server call, and `pgedge starfleet doctor` reports the whole diff --git a/docs/ci.md b/docs/ci.md index 6ddee33..986d996 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -17,7 +17,7 @@ The page leans on these terms: An unattended run signs in with an API client, never through the browser: a browser sign-in needs a person at the sign-in page, and its -session ends without renewing itself. +session ends after a period without use or a fixed maximum. Set `PGEDGE_CLIENT_ID` and `PGEDGE_CLIENT_SECRET` from your secret store. The CLI treats the pair as a profile that lasts one invocation. diff --git a/docs/doctor.md b/docs/doctor.md index a9e9146..4a897a3 100644 --- a/docs/doctor.md +++ b/docs/doctor.md @@ -81,7 +81,7 @@ The following table describes each row and what a warning means: |---|---|---| | Auth | Which credential source resolved, whether a token is cached, and whether that token is still valid. | Credentials resolve but the token is expired or absent, or a valid cached token was minted for a different connection. It reports `error` when no credentials resolve at all. | | API connectivity | An unauthenticated GET against the resolved base URL, with the status and latency. | Never. An unreachable endpoint is reported as `error`. | -| Tenant | The tenant the credential authenticates as, its ID and its plan. | The plan is anything but `enterprise`, or the tenant could not be read. | +| Tenant | The tenant the credential authenticates as, its ID and its plan. | The plan is anything but `enterprise`, the tenant could not be read, or a browser session must renew before it can be read; the next command renews it. | | Environment | The operating system, architecture, and whether `NO_COLOR` is set. | Never. | A cached token is bound to the credential and the API base URL that @@ -122,12 +122,14 @@ login will not help. `pgedge starfleet auth status` answers a narrower question than doctor: which credential resolved, without calling any endpoint. It -prints `authenticated`, `source`, `problem`, `client_id`, `api_url`, -`token_valid`, `token_bound` and `expires_at`, and the client secret's -value never appears in any format. Only the three booleans are always -there: the five string fields are omitted when empty, so a run with no -credentials configured answers with `authenticated`, `token_valid`, -`token_bound` and the `api_url` that resolved anyway. +prints `authenticated`, `source`, `problem`, `client_id`, `user`, +`tenant_id`, `api_url`, `token_valid`, `token_bound`, `expires_at` and +`renews`, and the client secret's value never appears in any format. +`user`, `tenant_id` and `renews` describe a browser session. Only the +four booleans are always there: the seven string fields are omitted +when empty, so a run with no credentials configured answers with +`authenticated`, `token_valid`, `token_bound`, `renews` and the +`api_url` that resolved anyway. Because it calls nothing, `authenticated: true` means the credentials resolved, not that the server accepts them: a wrong secret that merely diff --git a/docs/error-catalog.md b/docs/error-catalog.md index 0a77da5..9d39632 100644 --- a/docs/error-catalog.md +++ b/docs/error-catalog.md @@ -30,6 +30,7 @@ describes the messages both steps print: | `--client-id given without --client-secret: supply both or neither` | 2 | Half a credential pair reached the command. Pass both flags, or drop both and let the profile answer. | [Authentication and profiles](auth-and-profiles.md) | | `authentication failed: ` | 5 | The token exchange did not complete. The reason names the cause, most often a refused credential or a token endpoint that could not be reached. | [Authentication and profiles](auth-and-profiles.md) | | `the browser sign-in for has expired or is missing: run 'pgedge starfleet auth login' to sign in again` | 5 | The profile's browser session ended, or its token was signed in for another API URL. Sign in again; for unattended runs, use an API client. | [Authentication and profiles](auth-and-profiles.md) | +| `the browser sign-in for could not be renewed: ; run 'pgedge starfleet auth login' to sign in again` | 5 | The sign-in service refused to renew the session and gave a reason. When the reason is `user is blocked`, signing in again is refused too; contact pgEdge support. | [Authentication and profiles](auth-and-profiles.md) | | `browser sign-in is not available for yet` | 5 | This API endpoint has no browser sign-in. Sign in with an API client: `--client-id` and `--client-secret`, or `--api-client`. | [Authentication and profiles](auth-and-profiles.md) | | `--tenant is not a tenant this sign-in can reach; reachable tenant IDs: ` | 2 | The browser session's user does not belong to that tenant. Pick one of the IDs listed. | [Authentication and profiles](auth-and-profiles.md) | | `authentication error (401): ` | 5 | The API rejected the bearer token. Run `pgedge starfleet auth logout` and log in again to rebuild the cache. | [Authentication and profiles](auth-and-profiles.md) | diff --git a/docs/getting-started.md b/docs/getting-started.md index 8353eb1..6168f97 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -45,9 +45,11 @@ describes each one and what it needs. ## Running Your First Commands Sign in once per profile. Login opens your browser on the pgEdge -sign-in page, and from then on the CLI works as you, in your tenant, -until the session expires. Login prints when that is; run it again -after that. It saves the profile in `~/.pgedge/cli/config.yaml`: +sign-in page, and from then on the CLI works as you, in your tenant. +The session renews itself while you use it, and ends after a period +without use or after a fixed maximum; run login again to start a new +one. Login saves the +profile in `~/.pgedge/cli/config.yaml`: pgedge starfleet auth login --profile dev diff --git a/internal/auth/auth.go b/internal/auth/auth.go index d95471c..f0438d8 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -321,6 +321,46 @@ func (a *Auth) KeychainSecret() (string, error) { return a.Keychain.Get(a.KeychainAccount()) } +// sessionAccountPrefix keeps a browser session's refresh token in an +// entry of its own, apart from any client secret of the same profile. +const sessionAccountPrefix = "session-refresh-token:" + +// SessionAccount is the keychain entry for this profile's browser +// session refresh token. +func (a *Auth) SessionAccount() string { + return sessionAccountPrefix + a.KeychainAccount() +} + +// SessionRefreshToken reads the browser session's refresh token. +func (a *Auth) SessionRefreshToken() (string, error) { + if a.Keychain == nil { + return "", keychain.ErrUnavailable + } + return a.Keychain.Get(a.SessionAccount()) +} + +// SaveSessionRefreshToken stores the browser session's refresh token. +// It is never written to the config file or the token cache. +func (a *Auth) SaveSessionRefreshToken(token string) error { + if a.Keychain == nil { + return keychain.ErrUnavailable + } + return a.Keychain.Set(a.SessionAccount(), token) +} + +// ClearSessionRefreshToken removes the browser session's refresh +// token; there being none is not an error. +func (a *Auth) ClearSessionRefreshToken() error { + if a.Keychain == nil { + return nil + } + err := a.Keychain.Delete(a.SessionAccount()) + if errors.Is(err, keychain.ErrNotFound) { + return nil + } + return err +} + // SaveToken writes the token to the profile+module cache file with // 0600 permissions. It refuses a token with no Fingerprint, so no // caller, including a future one, can write an unbound cache file; diff --git a/internal/auth/lock.go b/internal/auth/lock.go new file mode 100644 index 0000000..60acd7f --- /dev/null +++ b/internal/auth/lock.go @@ -0,0 +1,60 @@ +package auth + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "time" +) + +// Session lock timings. A renewal holds the lock for one bounded token +// request and two keychain calls, so a lock older than staleLockAge was +// left by a process that died holding it. lockWait outlasts that age so +// a waiter recovers a dead holder's lock rather than failing. +var ( + staleLockAge = 45 * time.Second + lockWait = 50 * time.Second + lockPoll = 50 * time.Millisecond +) + +// ErrLockTimeout reports that another process held the session lock +// for the whole wait. +var ErrLockTimeout = errors.New("auth: timed out waiting for the " + + "session lock") + +// LockSession takes this profile's session lock, a file beside the +// token cache, so that only one process at a time renews the session: +// two renewals sending the same rotating refresh token would have Auth0 +// revoke both. An O_EXCL file rather than flock, so one implementation +// serves every OS. +func (a *Auth) LockSession() (unlock func(), err error) { + p, err := a.cachePath() + if err != nil { + return nil, err + } + p += ".lock" + if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + return nil, fmt.Errorf("auth: create cache dir: %w", err) + } + deadline := time.Now().Add(lockWait) + for { + f, err := os.OpenFile(p, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) //nolint:gosec // G304: cachePath validates both name segments + if err == nil { + _ = f.Close() + return func() { _ = os.Remove(p) }, nil + } + if !errors.Is(err, os.ErrExist) { + return nil, fmt.Errorf("auth: session lock: %w", err) + } + if fi, err := os.Stat(p); err == nil && + time.Since(fi.ModTime()) > staleLockAge { + _ = os.Remove(p) + continue + } + if time.Now().After(deadline) { + return nil, ErrLockTimeout + } + time.Sleep(lockPoll) + } +} diff --git a/internal/auth/lock_test.go b/internal/auth/lock_test.go new file mode 100644 index 0000000..df1b702 --- /dev/null +++ b/internal/auth/lock_test.go @@ -0,0 +1,92 @@ +package auth + +import ( + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/pgEdge/pgedge-cli/internal/keychain" +) + +// TestLockSessionIsPerProfile checks that a held lock shuts out the +// same profile only, and that a dead holder's lock is recovered. +func TestLockSessionIsPerProfile(t *testing.T) { + savedWait, savedAge := lockWait, staleLockAge + t.Cleanup(func() { lockWait, staleLockAge = savedWait, savedAge }) + lockWait, staleLockAge = 100*time.Millisecond, time.Hour + + dir := t.TempDir() + dev := &Auth{Profile: "dev", Module: "starfleet", CacheDir: dir} + unlock, err := dev.LockSession() + if err != nil { + t.Fatalf("LockSession: %v", err) + } + if _, err := (&Auth{Profile: "dev", Module: "starfleet", + CacheDir: dir}).LockSession(); !errors.Is(err, ErrLockTimeout) { + t.Fatalf("second lock on dev: err = %v, want ErrLockTimeout", err) + } + other, err := (&Auth{Profile: "prod", Module: "starfleet", + CacheDir: dir}).LockSession() + if err != nil { + t.Fatalf("lock on another profile: %v", err) + } + other() + unlock() + if _, err := dev.LockSession(); err != nil { + t.Fatalf("lock after unlock: %v", err) + } + + // The holder dies without unlocking: its lock goes stale. + old := time.Now().Add(-2 * time.Hour) + if err := os.Chtimes(filepath.Join(dir, "dev-starfleet.json.lock"), + old, old); err != nil { + t.Fatal(err) + } + staleLockAge = time.Minute + if _, err := dev.LockSession(); err != nil { + t.Fatalf("stale lock was not recovered: %v", err) + } +} + +// TestSessionRefreshTokenHasItsOwnKeychainEntry checks that the refresh +// token neither overwrites nor is cleared with the profile's client +// secret, and that it is refused rather than kept elsewhere when there +// is no keychain. +func TestSessionRefreshTokenHasItsOwnKeychainEntry(t *testing.T) { + kc := &keychain.Fake{} + a := &Auth{Profile: "dev", ConfigPath: "/cfg/config.yaml", Keychain: kc} + if err := kc.Set(a.KeychainAccount(), "client-secret"); err != nil { + t.Fatal(err) + } + if err := a.SaveSessionRefreshToken("rt"); err != nil { + t.Fatalf("SaveSessionRefreshToken: %v", err) + } + if got, err := a.SessionRefreshToken(); err != nil || got != "rt" { + t.Errorf("SessionRefreshToken = %q, %v", got, err) + } + if err := a.ClearSessionRefreshToken(); err != nil { + t.Fatalf("ClearSessionRefreshToken: %v", err) + } + if err := a.ClearSessionRefreshToken(); err != nil { + t.Errorf("clearing an absent token: %v", err) + } + if got, err := a.KeychainSecret(); err != nil || got != "client-secret" { + t.Errorf("client secret = %q, %v after the session's churn", got, + err) + } + + none := &Auth{Profile: "dev"} + if err := none.SaveSessionRefreshToken("rt"); !errors.Is(err, + keychain.ErrUnavailable) { + t.Errorf("save without a keychain: err = %v", err) + } + if _, err := none.SessionRefreshToken(); !errors.Is(err, + keychain.ErrUnavailable) { + t.Errorf("read without a keychain: err = %v", err) + } + if err := none.ClearSessionRefreshToken(); err != nil { + t.Errorf("clear without a keychain: %v", err) + } +} diff --git a/internal/clitest/stderr_sanitize_test.go b/internal/clitest/stderr_sanitize_test.go index 224671a..b935d4b 100644 --- a/internal/clitest/stderr_sanitize_test.go +++ b/internal/clitest/stderr_sanitize_test.go @@ -299,8 +299,13 @@ var stderrComposedMessages = map[string]struct { // 199 -> 200: `branch get` prints the display name. // 200 -> 211: browser sign-in's URL, tenant prompt and summary lines, // and whoami's `User:` line. +// 211 -> 215: session renewal's keychain warning, and logout's revoke +// and keychain warnings. +// 215 -> 218: a renewal that falls back to the current token, a +// replaced session that could not be revoked, and an API-client login +// that could not remove the session it replaced. const ( - stderrArgSites = 211 + stderrArgSites = 218 stderrComposedSites = 22 ) diff --git a/internal/httplog/httplog.go b/internal/httplog/httplog.go index 8f793ca..f12ef2a 100644 --- a/internal/httplog/httplog.go +++ b/internal/httplog/httplog.go @@ -308,9 +308,9 @@ func RedactBody(raw []byte, limit int) string { return wholeBodySummary(doc) } -// formSecretNames are the form fields of an OAuth code exchange that -// would let a reader of the log finish it. -var formSecretNames = []string{"code", "code_verifier"} +// formSecretNames are the OAuth form fields that would let a reader of +// the log finish a code exchange or use a token being revoked. +var formSecretNames = []string{"code", "code_verifier", "token"} // RedactForm is RedactBody for a form-encoded body: the value of every // secret field, JSON names included, is masked, and the field order is diff --git a/internal/starfleet/account/cmd/auth.go b/internal/starfleet/account/cmd/auth.go index 6ea43c4..52e72d4 100644 --- a/internal/starfleet/account/cmd/auth.go +++ b/internal/starfleet/account/cmd/auth.go @@ -60,13 +60,16 @@ you, through the browser, or as an API client. With no credential flags, login opens your browser on the pgEdge sign-in page and prints the same URL on stderr, so you can open it by hand when no browser starts. Once you sign in, every command runs as -you, in your tenant, until the session expires; login prints when -that is, and you run login again after it. The profile stores the API -URL, the tenant and the fact that it is a browser sign-in; the token -goes to the token cache, never the config file. When you belong to -several tenants, login asks which to use when it can prompt, and uses -your account's default tenant otherwise. --tenant picks one by ID or -name without asking. Browser sign-in is available only for pgEdge +you, in your tenant. The session renews itself while in use. It ends +after a period without use, or after a fixed maximum however much it +is used; run login again to start a new one. The profile stores the +API URL, the tenant and the fact that it is a browser sign-in; the +access token goes to the token cache and the token that renews the +session to the OS keychain, never the config file. Where no keychain +can be used, the session lasts until the time login prints. When you +belong to several tenants, login asks which to use when it can +prompt, and uses your account's default tenant otherwise. --tenant +picks one by ID or name without asking. Browser sign-in is available only for pgEdge Starfleet APIs that have a sign-in app; for any other --api-url, login fails with exit 5 and asks for an API client. @@ -184,6 +187,16 @@ Example: } return fmt.Errorf("save credentials: %w", err) } + if cp.IsBrowserSession() { + if prev, err := store.SessionRefreshToken(); err == nil { + revokeDiscarded(rt, conn.ResolveAPIURL(cp, ""), prev) + } + if err := store.ClearSessionRefreshToken(); err != nil { + fmt.Fprintf(rt.Stderr, "Warning: could not remove the "+ + "previous session from the OS keychain: %s\n", + output.Sanitize(err.Error())) + } + } // The file Save() actually wrote, not DefaultPath(): Save // writes the --config value when one was given. @@ -268,6 +281,8 @@ type authStatusReport struct { // TokenBound means what authInfo.TokenBound (doctor.go) means. TokenBound bool `json:"token_bound"` ExpiresAt string `json:"expires_at,omitempty"` + // Renews reports a browser session that can renew its own token. + Renews bool `json:"renews"` } func newAuthStatusCmd(rt *module.Runtime, f *conn.Flags) *cobra.Command { @@ -279,15 +294,18 @@ current profile, the API URL in effect, and whether a cached token is present and still valid. -o json and -o yaml carry the same facts as a single object, so a script can check them without parsing prose. -For a browser sign-in it reports the user, the tenant and when the -session expires; for an API client, its client ID. +For a browser sign-in it reports the user, the tenant, when the +current access token expires and whether the session renews itself; +for an API client, its client ID. It exits 5 (the auth-failure code) when no usable credentials resolve, 2 if you supplied half a credential pair, and 0 whenever they do. An API client's token may be missing or expired with exit 0, since the CLI mints a new one on demand the next time it -is needed. A browser session cannot be renewed that way, so an -expired one exits 5; run 'pgedge starfleet auth login' again. Run +is needed. A browser session that renews itself does the same, so an +expired access token is exit 0 and renews on next use; one that +cannot renew itself exits 5 once it expires, and 'pgedge starfleet +auth login' starts a new session. Run status to confirm you are signed in before using resource commands. Example: @@ -383,10 +401,10 @@ Example: // sessionState is what status and doctor read from a browser session's // cache, and the failure a command would hit with it, if any. type sessionState struct { - tok *auth.CachedToken - valid, bound bool - tenant string - problem error + tok *auth.CachedToken + valid, bound, renews bool + tenant string + problem error } // readSession checks a session the way conn.Resolve would, without @@ -403,8 +421,10 @@ func readSession(rt *module.Runtime, res *conn.Resolved, s.tok = tok s.valid = time.Until(tok.ExpiresAt) > auth.RefreshWindow s.bound = tok.SessionFor(res.APIURL) + _, rerr := conn.Store(rt).SessionRefreshToken() + s.renews = rerr == nil claims, err := auth.ParseClaims(tok.AccessToken) - if !s.valid || !s.bound || err != nil { + if (!s.valid && !s.renews) || !s.bound || err != nil { s.problem = expired return s } @@ -415,9 +435,8 @@ func readSession(rt *module.Runtime, res *conn.Resolved, return s } -// sessionStatus reports a browser session. Unlike an API client, an -// expired session cannot be renewed on demand, so it exits 5; a tenant -// the token cannot reach exits 2. +// sessionStatus reports a browser session. An expired one that cannot +// renew itself exits 5; a tenant the token cannot reach exits 2. func sessionStatus( rt *module.Runtime, res *conn.Resolved, flagTenant string, ) error { @@ -429,6 +448,7 @@ func sessionStatus( TenantID: s.tenant, TokenValid: s.valid, TokenBound: s.bound, + Renews: s.renews, } if s.tok != nil { report.User = s.tok.User @@ -449,12 +469,19 @@ func sessionStatus( case !s.bound: lines = append(lines, "Token: stale (signed in for a different API URL)") + case !s.valid && s.renews: + lines = append(lines, "Token: expired, renews on next use") case !s.valid: lines = append(lines, "Token: expired") default: lines = append(lines, fmt.Sprintf( "Token: valid (expires %s)", report.ExpiresAt)) } + renews := "no" + if s.renews { + renews = "yes" + } + lines = append(lines, "Renews: "+renews) if pErr := printAuthStatus( rt, report, strings.Join(lines, "\n")+"\n"); pErr != nil { return pErr @@ -481,14 +508,19 @@ func newAuthLogoutCmd(rt *module.Runtime) *cobra.Command { Long: `logout removes the cached access token and the stored credential for the active profile: for an API client, its client ID and secret, from the OS keychain and from ~/.pgedge/cli/config.yaml; -for a browser sign-in, the session and its tenant. The API URL and the -profile itself are kept, so 'pgedge starfleet auth login' can sign in -again without re-entering the endpoint. +for a browser sign-in, the session and its tenant, after asking pgEdge +to end the session so it cannot be renewed. If that request fails, +logout warns and still removes the session from this machine. The API +URL and the profile itself are kept, so 'pgedge starfleet auth login' +can sign in again without re-entering the endpoint. Example: pgedge starfleet auth logout`, Args: cobra.NoArgs, RunE: func(_ *cobra.Command, _ []string) error { + if rt.Config.StarfleetProfile(rt.Profile).IsBrowserSession() { + endSession(rt) + } if err := conn.Logout(rt); err != nil { return fmt.Errorf("clear token: %w", err) } @@ -522,6 +554,35 @@ Example: } } +// endSession revokes the session's refresh token, best effort, and +// removes it from the keychain. +func endSession(rt *module.Runtime) { + store := conn.Store(rt) + token, err := store.SessionRefreshToken() + if err != nil { + return + } + apiURL := conn.ResolveAPIURL(rt.Config.StarfleetProfile(rt.Profile), "") + if err := conn.RevokeSessionToken(rt, apiURL, token); err != nil { + fmt.Fprintf(rt.Stderr, "Warning: could not end the session "+ + "with pgEdge (%s); it is removed from this machine.\n", + output.Sanitize(err.Error())) + } + if err := store.ClearSessionRefreshToken(); err != nil { + fmt.Fprintf(rt.Stderr, "Warning: could not remove the session "+ + "from the OS keychain: %s\n", output.Sanitize(err.Error())) + } +} + +// revokeDiscarded ends a refresh token the profile no longer keeps, +// best effort. +func revokeDiscarded(rt *module.Runtime, apiURL, token string) { + if err := conn.RevokeSessionToken(rt, apiURL, token); err != nil { + fmt.Fprintf(rt.Stderr, "Warning: could not end a replaced "+ + "session with pgEdge: %s\n", output.Sanitize(err.Error())) + } +} + // promptCredentials asks for a client ID and secret on stdin. Call it // only when neither credential flag was supplied: a closed stdin fails // at the first read, which would break `login` in a script. diff --git a/internal/starfleet/account/cmd/auth_browser.go b/internal/starfleet/account/cmd/auth_browser.go index f3f70dc..c33697e 100644 --- a/internal/starfleet/account/cmd/auth_browser.go +++ b/internal/starfleet/account/cmd/auth_browser.go @@ -23,10 +23,6 @@ import ( "github.com/pgEdge/pgedge-cli/internal/starfleet/signin" ) -// auth0For finds the sign-in app for an API URL. A variable so tests -// can sign in against a stub API. -var auth0For = apidefaults.Auth0For - // signInFlow builds the browser sign-in. A variable so tests can point // it at a fake Auth0 and stand in for the browser. var signInFlow = func(rt *module.Runtime, app apidefaults.Auth0App, @@ -59,7 +55,7 @@ var promptIsInteractive = func(rt *module.Runtime) bool { func browserLogin(rt *module.Runtime, f *conn.Flags) error { apiURL := conn.ResolveAPIURL( rt.Config.StarfleetProfile(rt.Profile), f.APIURL) - app, ok := auth0For(apiURL) + app, _, ok := conn.SignInApp(apiURL) if !ok { // Exit 5, not 2: the command is well formed, and the same line // works once the API has a sign-in app. @@ -103,18 +99,35 @@ func browserLogin(rt *module.Runtime, f *conn.Flags) error { return newExitError(fmt.Sprintf("save session: %v", err), ExitAuth) } + // A refresh token the profile will not keep is ended, not left live. + store := conn.Store(rt) + old := rt.Config.StarfleetProfile(rt.Profile) + if old.IsBrowserSession() { + if prev, err := store.SessionRefreshToken(); err == nil { + revokeDiscarded(rt, conn.ResolveAPIURL(old, ""), prev) + } + } + renews := false + if toks.RefreshToken != "" { + renews = store.SaveSessionRefreshToken(toks.RefreshToken) == nil + if !renews { + revokeDiscarded(rt, apiURL, toks.RefreshToken) + } + } + if !renews { + _ = store.ClearSessionRefreshToken() + } // One profile, one kind of credential: an API client's secret goes. // Saved before the secret goes, so a failed save keeps a working // API client. - old := rt.Config.StarfleetProfile(rt.Profile) rt.Config.SetStarfleetProfile(rt.Profile, &config.StarfleetProfile{ APIURL: apiURL, Auth: config.AuthBrowser, TenantID: tenantID}) if err := rt.Config.Save(); err != nil { return fmt.Errorf("save profile: %w", err) } if old.ClientID != "" && old.ClientSecret == "" { - if err := keychainDelete(conn.Store(rt)); err != nil { + if err := keychainDelete(store); err != nil { fmt.Fprintf(rt.Stderr, "Warning: could not remove the "+ "previous client secret from the OS keychain: %s\n", output.Sanitize(err.Error())) @@ -123,9 +136,16 @@ func browserLogin(rt *module.Runtime, f *conn.Flags) error { fmt.Fprintf(rt.Stderr, "Signed in as %s.\n", output.Sanitize(user)) fmt.Fprintf(rt.Stderr, "Tenant: %s\n", output.Sanitize(label)) - fmt.Fprintf(rt.Stderr, "The session lasts until %s; run 'pgedge "+ - "starfleet auth login' again after that.\n", - expiresAt.Format(time.RFC3339)) + if renews { + fmt.Fprintln(rt.Stderr, "The session renews itself while in use. "+ + "It ends after a period without use, or after a fixed "+ + "maximum however much it is used; run 'pgedge starfleet "+ + "auth login' to start a new one.") + } else { + fmt.Fprintf(rt.Stderr, "The session lasts until %s; run 'pgedge "+ + "starfleet auth login' again after that.\n", + expiresAt.Format(time.RFC3339)) + } fmt.Fprintf(rt.Stderr, "Profile %s saved to %s\n", output.Sanitize(rt.Profile), output.Sanitize(rt.Config.Path())) return nil diff --git a/internal/starfleet/account/cmd/auth_browser_test.go b/internal/starfleet/account/cmd/auth_browser_test.go index 5f473dc..70c612e 100644 --- a/internal/starfleet/account/cmd/auth_browser_test.go +++ b/internal/starfleet/account/cmd/auth_browser_test.go @@ -4,18 +4,22 @@ import ( "bytes" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" "net/url" "os" "path/filepath" + "slices" "strings" + "sync" "sync/atomic" "testing" "time" "github.com/pgEdge/pgedge-cli/internal/apidefaults" + "github.com/pgEdge/pgedge-cli/internal/auth" "github.com/pgEdge/pgedge-cli/internal/config" "github.com/pgEdge/pgedge-cli/internal/keychain" "github.com/pgEdge/pgedge-cli/internal/module" @@ -31,7 +35,9 @@ const tenantsBody = `[` + // signInStubs stands up a fake Auth0 and a fake API, and points the // login command's seams at them. The fake Auth0 issues an access token -// reaching tenants, with the first as its default. +// reaching tenants, with the first as its default, and, when +// issueRefresh is set, a new refresh token per answer: refresh~1, +// refresh~2 and so on. A tilde cannot occur in a JWT. type signInStubs struct { api string tenants []string @@ -40,6 +46,21 @@ type signInStubs struct { reply func(q url.Values) url.Values apiHits map[string]int headers []http.Header + + issueRefresh bool + refreshes atomic.Int32 + // refreshError, when set, is the OAuth error a refresh gets, with + // refreshReason as its error_description. + refreshError string + refreshReason string + // revokeStatus, when set, is the status /oauth/revoke answers. + revokeStatus int + // atAPI, when set, runs as each API request arrives. + atAPI func(*http.Request) + + mu sync.Mutex + refreshedWith []string + revoked []string } // stubCode is the authorization code the fake browser returns. @@ -54,20 +75,49 @@ func newSignInStubs(t *testing.T, tenants ...string) *signInStubs { auth0 := httptest.NewServer(http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { _ = r.ParseForm() - s.verifier.Store(r.PostForm.Get("code_verifier")) + w.Header().Set("Content-Type", "application/json") + s.mu.Lock() + defer s.mu.Unlock() + if r.URL.Path == "/oauth/revoke" { + s.revoked = append(s.revoked, r.PostForm.Get("token")) + if s.revokeStatus != 0 { + w.WriteHeader(s.revokeStatus) + } + return + } + if r.PostForm.Get("grant_type") == "refresh_token" { + s.refreshedWith = append(s.refreshedWith, + r.PostForm.Get("refresh_token")) + if s.refreshError != "" { + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(map[string]string{ + "error": s.refreshError, + "error_description": s.refreshReason}) + return + } + } else { + s.verifier.Store(r.PostForm.Get("code_verifier")) + } at := testsupport.SessionToken(t, "auth0|u1", time.Now().Add(24*time.Hour), s.tenants...) it := testsupport.FakeJWT(t, map[string]any{"email": "user@example.com"}) s.issued.Store(at + " " + it) - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]any{ + body := map[string]any{ "access_token": at, "id_token": it, "expires_in": 86400, - }) + } + if s.issueRefresh { + body["refresh_token"] = fmt.Sprintf("refresh~%d", + s.refreshes.Add(1)) + } + _ = json.NewEncoder(w).Encode(body) })) t.Cleanup(auth0.Close) api := httptest.NewServer(http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { + if s.atAPI != nil { + s.atAPI(r) + } s.apiHits[r.URL.Path]++ s.headers = append(s.headers, r.Header.Clone()) w.Header().Set("Content-Type", "application/json") @@ -83,15 +133,15 @@ func newSignInStubs(t *testing.T, tenants ...string) *signInStubs { t.Cleanup(api.Close) s.api = api.URL - savedFor, savedFlow, savedTTY := auth0For, signInFlow, + savedApp, savedFlow, savedTTY := conn.SignInApp, signInFlow, promptIsInteractive t.Cleanup(func() { - auth0For, signInFlow, promptIsInteractive = savedFor, savedFlow, - savedTTY + conn.SignInApp, signInFlow, promptIsInteractive = savedApp, + savedFlow, savedTTY }) - auth0For = func(string) (apidefaults.Auth0App, bool) { + conn.SignInApp = func(string) (apidefaults.Auth0App, string, bool) { return apidefaults.Auth0App{Domain: "unused", - Audience: "aud", ClientID: "cid"}, true + Audience: "aud", ClientID: "cid"}, auth0.URL, true } promptIsInteractive = func(*module.Runtime) bool { return false } signInFlow = func(rt *module.Runtime, app apidefaults.Auth0App, @@ -131,19 +181,34 @@ func exitCode(err error) int { return -1 } +// TestBrowserLoginDebugOutputCarriesNoSecrets follows a session from +// sign-in through a renewal to logout's revoke. func TestBrowserLoginDebugOutputCarriesNoSecrets(t *testing.T) { s := newSignInStubs(t, "t-a") + s.issueRefresh = true rt, _, errb := testsupport.NewRuntime(t, "", "text") + rt.Keychain = &keychain.Fake{} rt.Debug = true if err := login(t, rt, s); err != nil { t.Fatalf("login: %v", err) } - if !strings.Contains(errb.String(), "/oauth/token") { - t.Fatalf("--debug did not log the exchange:\n%s", errb.String()) - } issued, _ := s.issued.Load().(string) + testsupport.SeedSession(t, rt, s.api, "u1", + time.Now().Add(-time.Minute), "t-a") + for _, args := range [][]string{{"auth", "whoami"}, {"auth", "logout"}} { + if err := runAccount(t, rt, &bytes.Buffer{}, args...); err != nil { + t.Fatalf("%v: %v", args, err) + } + } + for _, want := range []string{"/oauth/token", "/oauth/revoke"} { + if !strings.Contains(errb.String(), want) { + t.Fatalf("--debug did not log %s:\n%s", want, errb.String()) + } + } + renewed, _ := s.issued.Load().(string) verifier, _ := s.verifier.Load().(string) - secrets := append(strings.Fields(issued), verifier, stubCode) + secrets := append(strings.Fields(issued+" "+renewed), verifier, + stubCode, "refresh~1", "refresh~2") for _, secret := range secrets { if secret == "" || strings.Contains(errb.String(), secret) { t.Errorf("--debug output carries %q", secret) @@ -151,6 +216,56 @@ func TestBrowserLoginDebugOutputCarriesNoSecrets(t *testing.T) { } } +// TestBrowserLoginSavesASession checks that a session renews itself +// only when Auth0 issued a refresh token and the keychain holds it, and +// that the token is never written to a file. +func TestBrowserLoginSavesASession(t *testing.T) { + for _, tc := range []struct { + name string + issue, keychain bool + renews bool + }{ + {"refresh token kept", true, true, true}, + {"no refresh token issued", false, true, false}, + {"no keychain", true, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + s := newSignInStubs(t, "t-a") + s.issueRefresh = tc.issue + rt, _, errb := testsupport.NewRuntime(t, "", "text") + kc := &keychain.Fake{} + if tc.keychain { + rt.Keychain = kc + } + // A previous session's refresh token must not survive. + _ = kc.Set(conn.Store(rt).SessionAccount(), "refresh~old") + if err := login(t, rt, s); err != nil { + t.Fatalf("login: %v\n%s", err, errb.String()) + } + got, _ := kc.Get(conn.Store(rt).SessionAccount()) + if want := map[bool]string{true: "refresh~1"}[tc.renews]; tc.keychain && + got != want { + t.Errorf("keychain holds %q, want %q", got, want) + } + for _, path := range []string{rt.Config.Path(), filepath.Join( + os.Getenv("HOME"), ".pgedge", "cli", "cache", + "default-starfleet.json")} { + raw, err := os.ReadFile(path) + if err != nil || bytes.Contains(raw, []byte("refresh~")) { + t.Errorf("%s: %v, or it holds a refresh token:\n%s", + path, err, raw) + } + } + renews := strings.Contains(errb.String(), + "The session renews itself while in use") + if renews != tc.renews { + t.Errorf("stderr says renews = %v, want %v:\n%s", renews, + tc.renews, errb.String()) + } + }) + } +} + func TestBrowserLoginTenantChoice(t *testing.T) { for _, tc := range []struct { name string @@ -347,6 +462,11 @@ func TestStatusForAnExpiredSession(t *testing.T) { &config.StarfleetProfile{APIURL: api, Auth: config.AuthBrowser}) }, "Token: not cached"}, + // Not a failure: the next command renews it. + {"expired, renews", func(t *testing.T, rt *module.Runtime, + api string) { + seedRenewableSession(t, rt, api, time.Now().Add(-time.Minute)) + }, "Token: expired, renews on next use"}, } { t.Run(tc.name, func(t *testing.T) { rt, out, _ := testsupport.NewRuntime(t, "", "text") @@ -354,7 +474,11 @@ func TestStatusForAnExpiredSession(t *testing.T) { tc.seed(t, rt, api) err := runAccount(t, rt, &bytes.Buffer{}, "auth", "status", "--api-url", api) - if exitCode(err) != ExitAuth || + if strings.HasSuffix(tc.name, "renews") { + if err != nil { + t.Fatalf("err = %v, want exit 0", err) + } + } else if exitCode(err) != ExitAuth || !strings.Contains(err.Error(), "auth login") { t.Fatalf("err = %v, want exit 5", err) } @@ -406,3 +530,156 @@ func TestTenantFlagIsValidatedByStatusAndDoctor(t *testing.T) { }) } } + +// seedRenewableSession is a session expiring at expiresAt whose +// keychain holds refresh~1, and the fake keychain. +func seedRenewableSession(t *testing.T, rt *module.Runtime, api string, + expiresAt time.Time) (kc *keychain.Fake, accessToken string) { + t.Helper() + kc = &keychain.Fake{} + rt.Keychain = kc + accessToken = testsupport.SeedSession(t, rt, api, "u1", expiresAt, + "t-a") + if err := conn.Store(rt).SaveSessionRefreshToken("refresh~1"); err != nil { + t.Fatal(err) + } + return kc, accessToken +} + +// orderedKeychain records the access token cached at each Set. +type orderedKeychain struct { + *keychain.Fake + cached func() string + atSet []string +} + +func (k *orderedKeychain) Set(account, secret string) error { + k.atSet = append(k.atSet, k.cached()) + return k.Fake.Set(account, secret) +} + +// TestSessionRenewalRotatesBeforeUse checks that Auth0's rotated refresh +// token is saved first, then the new access token, and both before the +// API sees it: the old refresh token is dead from that moment. +func TestSessionRenewalRotatesBeforeUse(t *testing.T) { + s := newSignInStubs(t, "t-a") + s.issueRefresh = true + s.refreshes.Store(1) + rt, _, _ := testsupport.NewRuntime(t, "", "text") + kc, old := seedRenewableSession(t, rt, s.api, + time.Now().Add(auth.RefreshWindow/2)) + ok := &orderedKeychain{Fake: kc, cached: func() string { + tok, _ := conn.Store(rt).LoadToken() + return tok.AccessToken + }} + rt.Keychain = ok + type seen struct { + refresh string + bearer, saved string + } + var atAPI []seen + s.atAPI = func(r *http.Request) { + v, _ := kc.Get(conn.Store(rt).SessionAccount()) + tok, _ := conn.Store(rt).LoadToken() + atAPI = append(atAPI, seen{v, strings.TrimPrefix( + r.Header.Get("Authorization"), "Bearer "), tok.AccessToken}) + } + if err := runAccount(t, rt, &bytes.Buffer{}, "auth", "whoami"); err != nil { + t.Fatalf("whoami: %v", err) + } + if !slices.Equal(s.refreshedWith, []string{"refresh~1"}) { + t.Fatalf("Auth0 got refresh tokens %v, want [refresh~1]", + s.refreshedWith) + } + if len(atAPI) == 0 { + t.Fatal("the API was never called") + } + for _, got := range atAPI { + if got.refresh != "refresh~2" || got.bearer == old || + got.bearer != got.saved { + t.Errorf("at the API: keychain %q, bearer renewed %v, "+ + "bearer cached %v", got.refresh, got.bearer != old, + got.bearer == got.saved) + } + } + if !slices.Equal(ok.atSet, []string{old}) { + t.Error("the access token was cached before the rotated " + + "refresh token was saved") + } +} + +func TestSessionRenewalRefused(t *testing.T) { + for _, tc := range []struct { + name string + reason string + auth0Down bool + want string + keepRefresh bool + }{ + {"invalid_grant", "", false, "has expired or is missing: run " + + "'pgedge starfleet auth login'", false}, + {"blocked user", "user is blocked", false, "could not be renewed: " + + "user is blocked", false}, + {"network failure", "", true, "could not renew", true}, + } { + t.Run(tc.name, func(t *testing.T) { + s := newSignInStubs(t, "t-a") + s.refreshError = "invalid_grant" + s.refreshReason = tc.reason + if tc.auth0Down { + conn.SignInApp = func(string) (apidefaults.Auth0App, + string, bool) { + return apidefaults.Auth0App{ClientID: "cid"}, + "http://127.0.0.1:1", true + } + } + rt, _, _ := testsupport.NewRuntime(t, "", "text") + kc, _ := seedRenewableSession(t, rt, s.api, + time.Now().Add(-time.Minute)) + err := runAccount(t, rt, &bytes.Buffer{}, "auth", "whoami") + if exitCode(err) != ExitAuth || + !strings.Contains(err.Error(), tc.want) { + t.Fatalf("err = %v, want exit 5 with %q", err, tc.want) + } + _, kerr := kc.Get(conn.Store(rt).SessionAccount()) + if kept := kerr == nil; kept != tc.keepRefresh { + t.Errorf("refresh token kept = %v, want %v", kept, + tc.keepRefresh) + } + }) + } +} + +// TestLogoutRevokesTheSession checks that logout revokes the refresh +// token, then removes it and the session even when the revoke fails. +func TestLogoutRevokesTheSession(t *testing.T) { + for _, status := range []int{http.StatusOK, + http.StatusInternalServerError} { + t.Run(http.StatusText(status), func(t *testing.T) { + s := newSignInStubs(t, "t-a") + s.revokeStatus = status + rt, _, errb := testsupport.NewRuntime(t, "", "text") + kc, _ := seedRenewableSession(t, rt, s.api, + time.Now().Add(time.Hour)) + if err := runAccount(t, rt, &bytes.Buffer{}, "auth", + "logout"); err != nil { + t.Fatalf("logout: %v", err) + } + if !slices.Equal(s.revoked, []string{"refresh~1"}) { + t.Errorf("revoked %v, want [refresh~1]", s.revoked) + } + if _, err := kc.Get(conn.Store(rt).SessionAccount()); err == nil { + t.Error("logout left the refresh token in the keychain") + } + if _, err := conn.Store(rt).LoadToken(); err == nil { + t.Error("logout left the session token") + } + warned := strings.Contains(errb.String(), + "could not end the session") + if warned != (status != http.StatusOK) { + t.Errorf("warning = %v for %d:\n%s", warned, status, + errb.String()) + } + }) + } +} diff --git a/internal/starfleet/account/cmd/doctor.go b/internal/starfleet/account/cmd/doctor.go index 9188074..5435fda 100644 --- a/internal/starfleet/account/cmd/doctor.go +++ b/internal/starfleet/account/cmd/doctor.go @@ -40,6 +40,8 @@ type authInfo struct { ExpiresAt string `json:"expires_at,omitempty"` // User is a browser session's email or subject. User string `json:"user,omitempty"` + // Renews reports a browser session that can renew its own token. + Renews bool `json:"renews"` } type apiInfo struct { @@ -78,6 +80,8 @@ type tenantInfo struct { // Count makes a credential spanning more than one tenant visible // rather than silently reported as its first. Count int `json:"count,omitempty"` + // Skipped says why the tenant was not read, when doctor chose not to. + Skipped string `json:"skipped,omitempty"` // selected: a browser session's chosen tenant, not the first. selected bool } @@ -179,9 +183,8 @@ func checkAuth(rt *module.Runtime, f *conn.Flags) authInfo { return info } -// checkSessionAuth fills the Auth row for a browser session, whose -// token cannot be re-minted: TokenValid and TokenBound are read the way -// conn.LoadSession reads them. +// checkSessionAuth fills the Auth row for a browser session the way +// conn.Resolve reads it, without renewing it. func checkSessionAuth(rt *module.Runtime, res *conn.Resolved, flagTenant string, info authInfo, ) authInfo { @@ -191,6 +194,7 @@ func checkSessionAuth(rt *module.Runtime, res *conn.Resolved, info.ExpiresAt = s.tok.ExpiresAt.Format(time.RFC3339) } info.TokenValid, info.TokenBound = s.valid, s.bound + info.Renews = s.renews if s.problem != nil { info.Authenticated = false info.Problem = s.problem.Error() @@ -296,6 +300,13 @@ func checkTenant(rt *module.Runtime, f *conn.Flags) tenantInfo { // enterprise is a warning, not an error: the credential works, it just // cannot drive byoc. func tenantCheckRow(info tenantInfo) output.CheckRow { + if info.Skipped != "" { + return output.CheckRow{ + Check: "Tenant", + Status: "warning", + Details: "not read: " + info.Skipped, + } + } if !info.Resolved { return output.CheckRow{ Check: "Tenant", @@ -348,9 +359,15 @@ func runDoctor(rt *module.Runtime, f *conn.Flags) error { report := accountDoctorReport{ Auth: checkAuth(rt, f), API: checkAPI(rt, f), - Tenant: checkTenant(rt, f), Environment: checkEnvironment(), } + // Reading the tenant would need the renewal doctor must not make. + if a := report.Auth; a.Source == auth.SourceBrowser && + a.Authenticated && !a.TokenValid { + report.Tenant.Skipped = "the browser session renews on next use" + } else { + report.Tenant = checkTenant(rt, f) + } if rt.Output.Structured() { return rt.Output.Print(report, nil) @@ -369,9 +386,15 @@ func runDoctor(rt *module.Runtime, f *conn.Flags) error { switch { case session && report.Auth.Authenticated: authStatus = "ok" - authDetail = fmt.Sprintf("signed in as %s via browser "+ - "(expires %s)", output.Sanitize(report.Auth.User), - report.Auth.ExpiresAt) + state := "expires " + report.Auth.ExpiresAt + switch { + case !report.Auth.TokenValid: + state = "token expired, renews on next use" + case report.Auth.Renews: + state += ", renews itself" + } + authDetail = fmt.Sprintf("signed in as %s via browser (%s)", + output.Sanitize(report.Auth.User), state) case report.Auth.Authenticated && report.Auth.Source == auth.SourceEnv: authStatus = "ok" authDetail = fmt.Sprintf("authenticated via %s and %s "+ diff --git a/internal/starfleet/conn/conn.go b/internal/starfleet/conn/conn.go index 0b9c337..bd05dbb 100644 --- a/internal/starfleet/conn/conn.go +++ b/internal/starfleet/conn/conn.go @@ -27,7 +27,9 @@ import ( "github.com/pgEdge/pgedge-cli/internal/errbody" "github.com/pgEdge/pgedge-cli/internal/httplog" "github.com/pgEdge/pgedge-cli/internal/module" + "github.com/pgEdge/pgedge-cli/internal/output" accountapi "github.com/pgEdge/pgedge-cli/internal/starfleet/account/api" + "github.com/pgEdge/pgedge-cli/internal/starfleet/signin" "github.com/pgEdge/pgedge-cli/internal/useragent" "github.com/spf13/pflag" ) @@ -614,6 +616,19 @@ func SessionExpiredError(rt *module.Runtime, apiURL string) *ExitError { apiURL, loginCommand(rt)), ExitAuth) } +// SessionRefusedError is SessionExpiredError carrying the reason Auth0 +// gave for refusing a refresh, so a blocked user is told so. +func SessionRefusedError(rt *module.Runtime, apiURL string, + err error) *ExitError { + var d *signin.DeniedError + if !errors.As(err, &d) || d.Description == "" { + return SessionExpiredError(rt, apiURL) + } + return NewExitError(fmt.Sprintf("the browser sign-in for %s could "+ + "not be renewed: %s; run '%s' to sign in again", + apiURL, d.Description, loginCommand(rt)), ExitAuth) +} + // loginCommand is the auth login line for rt's profile. func loginCommand(rt *module.Runtime) string { login := "pgedge starfleet auth login" @@ -654,16 +669,33 @@ func SessionTenant(rt *module.Runtime, profileTenant, flagTenant string, return claims.TenantID, nil } -// LoadSession reads the profile's browser session token, usable when it -// is bound to apiURL and more than auth.RefreshWindow from expiry. -// Anything else is SessionExpiredError. -func LoadSession(rt *module.Runtime, apiURL string, +// SignInApp finds the Auth0 sign-in app for an API URL and the base URL +// of its endpoints. A variable so tests can point it at a fake Auth0. +var SignInApp = func(apiURL string) (apidefaults.Auth0App, string, bool) { + app, ok := apidefaults.Auth0For(apiURL) + return app, "https://" + app.Domain, ok +} + +// loadSession reads the profile's browser session token. One bound to +// apiURL but near expiry is renewed with the session's refresh token +// when policy allows; anything else unusable is SessionExpiredError. +func loadSession(rt *module.Runtime, apiURL string, timeout time.Duration, + policy tokenPolicy, warn *sync.Once, ) (*auth.CachedToken, *auth.Claims, error) { tok, err := Store(rt).LoadToken() - if err != nil || !tok.SessionFor(apiURL) || - time.Until(tok.ExpiresAt) <= auth.RefreshWindow { + if err != nil || !tok.SessionFor(apiURL) { return nil, nil, SessionExpiredError(rt, apiURL) } + if time.Until(tok.ExpiresAt) <= auth.RefreshWindow { + if policy == ephemeralToken { + return nil, nil, SessionExpiredError(rt, apiURL) + } + tok, err = renewSession(context.Background(), rt, apiURL, tok, + timeout, warn) + if err != nil { + return nil, nil, err + } + } claims, err := auth.ParseClaims(tok.AccessToken) if err != nil { return nil, nil, SessionExpiredError(rt, apiURL) @@ -671,6 +703,106 @@ func LoadSession(rt *module.Runtime, apiURL string, return tok, claims, nil } +// renewSession renews cur. A failure that leaves the session alive, such +// as a network error, falls back to cur while it is unexpired. +func renewSession(ctx context.Context, rt *module.Runtime, apiURL string, + cur *auth.CachedToken, timeout time.Duration, warn *sync.Once, +) (*auth.CachedToken, error) { + tok, ended, err := renewLocked(ctx, rt, apiURL, cur.User, timeout) + if err == nil || ended || !cur.ExpiresAt.After(now()) { + return tok, err + } + warn.Do(func() { + fmt.Fprintf(rt.Stderr, "Warning: %s; using the current sign-in "+ + "until it expires.\n", output.Sanitize(err.Error())) + }) + return cur, nil +} + +// renewLocked exchanges the session's refresh token for a new access +// token under the session lock, so concurrent processes renew once. +// ended reports a session that can never renew again. +func renewLocked(ctx context.Context, rt *module.Runtime, apiURL, + user string, timeout time.Duration, +) (tok *auth.CachedToken, ended bool, err error) { + failed := func(err error) *ExitError { + return NewExitError(fmt.Sprintf("could not renew the browser "+ + "sign-in for %s: %v", apiURL, err), ExitAuth) + } + store := Store(rt) + unlock, err := store.LockSession() + if err != nil { + return nil, false, failed(err) + } + defer unlock() + // Another process may have renewed while this one waited. + if t, err := store.LoadToken(); err == nil && t.SessionFor(apiURL) && + t.ExpiresAt.Sub(now()) > auth.RefreshWindow { + return t, false, nil + } + refresh, err := store.SessionRefreshToken() + if err != nil { + return nil, true, SessionExpiredError(rt, apiURL) + } + app, base, ok := SignInApp(apiURL) + if !ok { + return nil, true, SessionExpiredError(rt, apiURL) + } + // Bounded however patient --timeout is: the lock is held meanwhile. + if timeout <= 0 || timeout > RequestTimeout { + timeout = RequestTimeout + } + toks, err := signin.Refresh(ctx, SignInHTTPClient(rt, timeout), base, + app.ClientID, refresh) + if signin.IsInvalidGrant(err) { + // Only the token this process sent is known dead. + if v, _ := store.SessionRefreshToken(); v == refresh { + _ = store.ClearSessionRefreshToken() + } + return nil, true, SessionRefusedError(rt, apiURL, err) + } + if err != nil { + return nil, false, failed(err) + } + claims, err := auth.ParseClaims(toks.AccessToken) + if err != nil { + return nil, false, failed(err) + } + expiresAt := claims.ExpiresAt() + if expiresAt.IsZero() { + expiresAt = time.Now().Add(time.Duration(toks.ExpiresIn) * + time.Second) + } + // The rotated refresh token first: the one sent is now dead. + if toks.RefreshToken != "" { + if err := store.SaveSessionRefreshToken(toks.RefreshToken); err != nil { + _ = store.ClearSessionRefreshToken() + fmt.Fprintf(rt.Stderr, "Warning: could not save the renewed "+ + "sign-in to the OS keychain (%s); run '%s' when this "+ + "session ends.\n", output.Sanitize(err.Error()), + output.Sanitize(loginCommand(rt))) + } + } + tok, err = SaveSession(rt, apiURL, toks.AccessToken, user, expiresAt) + if err != nil { + return nil, false, failed(err) + } + return tok, false, nil +} + +// RevokeSessionToken revokes a browser session's refresh token at the +// sign-in app for apiURL. +func RevokeSessionToken(rt *module.Runtime, apiURL, token string) error { + app, base, ok := SignInApp(apiURL) + if !ok { + return fmt.Errorf("no sign-in app for %s", apiURL) + } + ctx, cancel := context.WithTimeout(context.Background(), RequestTimeout) + defer cancel() + return signin.Revoke(ctx, SignInHTTPClient(rt, RequestTimeout), base, + app.ClientID, token) +} + // SaveSession writes a browser sign-in's access token to the profile's // cache, bound to apiURL and the token's subject. func SaveSession(rt *module.Runtime, apiURL, accessToken, user string, @@ -705,18 +837,14 @@ func TenantUsageError(tenant string, reachable []string) *ExitError { // Resolve produces an authenticated connection for the runtime's // active profile, honouring per-invocation flag overrides. It reuses a // cached token that is more than auth.RefreshWindow from expiry and -// otherwise exchanges credentials for a fresh one; a browser session -// has nothing to exchange, so it fails instead. +// otherwise exchanges credentials, or a browser session's refresh +// token, for a fresh one. func Resolve(rt *module.Runtime, f Flags) (*Conn, error) { return resolve(rt, f, persistToken) } // ResolveEphemeral is Resolve for `starfleet doctor`, which must not -// change what it reports: it reuses a valid cached token but never -// writes one. Writing would let doctor report "token expired or missing" -// and leave a fresh token behind, and `doctor --api-url ` would -// evict the profile's working token, since a cached token is bound to -// its API URL. +// change what it reports: it never writes a token or renews a session. func ResolveEphemeral(rt *module.Runtime, f Flags) (*Conn, error) { return resolve(rt, f, ephemeralToken) } @@ -741,7 +869,7 @@ func resolve(rt *module.Runtime, f Flags, policy tokenPolicy, return nil, NewExitError(err.Error(), ExitAuth) } if res.Session { - return resolveSession(rt, res, f.Tenant, timeout) + return resolveSession(rt, res, f.Tenant, timeout, policy) } if f.Tenant != "" { return nil, ErrTenantWithAPIClient() @@ -790,9 +918,10 @@ func resolve(rt *module.Runtime, f Flags, policy tokenPolicy, // resolveSession builds a browser session's connection from its cached // token. Every request carries SelectedTenantHeader. func resolveSession(rt *module.Runtime, res *Resolved, flagTenant string, - timeout time.Duration, + timeout time.Duration, policy tokenPolicy, ) (*Conn, error) { - tok, claims, err := LoadSession(rt, res.APIURL) + warn := &sync.Once{} + tok, claims, err := loadSession(rt, res.APIURL, timeout, policy, warn) if err != nil { return nil, err } @@ -806,13 +935,21 @@ func resolveSession(rt *module.Runtime, res *Resolved, flagTenant string, TenantIDs: claims.Tenants(), ExpiresAt: tok.ExpiresAt, } - // A wait that outlives the token ends with the same message, not a - // 401: there is nothing to re-mint with. + // A wait that outlives the token renews it as the first call would. + cur := tok src := &tokenSource{ value: tok.AccessToken, expiry: tok.ExpiresAt, - mint: func(context.Context) (string, time.Time, error) { - return "", time.Time{}, SessionExpiredError(rt, res.APIURL) + mint: func(ctx context.Context) (string, time.Time, error) { + if policy == ephemeralToken { + return "", time.Time{}, SessionExpiredError(rt, res.APIURL) + } + t, err := renewSession(ctx, rt, res.APIURL, cur, timeout, warn) + if err != nil { + return "", time.Time{}, err + } + cur = t + return t.AccessToken, t.ExpiresAt, nil }, } return &Conn{ diff --git a/internal/starfleet/conn/session_test.go b/internal/starfleet/conn/session_test.go index b01b6c9..ec23033 100644 --- a/internal/starfleet/conn/session_test.go +++ b/internal/starfleet/conn/session_test.go @@ -1,15 +1,24 @@ package conn import ( + "bytes" "context" + "encoding/json" "errors" + "fmt" + "io" "net/http" + "net/http/httptest" "strings" + "sync" + "sync/atomic" "testing" "time" + "github.com/pgEdge/pgedge-cli/internal/apidefaults" "github.com/pgEdge/pgedge-cli/internal/auth" "github.com/pgEdge/pgedge-cli/internal/config" + "github.com/pgEdge/pgedge-cli/internal/keychain" "github.com/pgEdge/pgedge-cli/internal/module" "github.com/pgEdge/pgedge-cli/internal/testsupport" "github.com/spf13/pflag" @@ -119,6 +128,13 @@ func TestSessionExpiredOrMissing(t *testing.T) { "https://other.example", "https://other.example"}, {"no cached token", "dev", seedProfileOnly, "", "auth login --profile dev"}, + {"refresh refused", "default", + seedRenewable(`{"error":"invalid_grant"}`, true), "", + "has expired"}, + {"no sign-in app", "default", seedRenewable("", false), "", + "has expired"}, + {"sign-in app unreachable", "default", seedRenewable("", true), "", + "could not renew"}, } { t.Run(tc.name, func(t *testing.T) { rt, _, _ := testsupport.NewRuntime(t, "", "text") @@ -147,11 +163,25 @@ func TestSessionWaitThatOutlivesTheTokenFailsWithTheSameMessage(t *testing.T) { wantExit(t, err, ExitAuth, "has expired or is missing") } +// TestFlagsOverASessionLeaveItsCacheAlone uses a session due for +// renewal, so flags that touched it would renew or clear its refresh +// token. func TestFlagsOverASessionLeaveItsCacheAlone(t *testing.T) { stub := newTokenStub(t, "client-tok") + auth0 := newTokenStub(t, "unused") + saved := SignInApp + t.Cleanup(func() { SignInApp = saved }) + SignInApp = func(string) (apidefaults.Auth0App, string, bool) { + return apidefaults.Auth0App{ClientID: "cid"}, auth0.url(), true + } rt, _, _ := testsupport.NewRuntime(t, "", "text") + kc := &keychain.Fake{} + rt.Keychain = kc sess := testsupport.SeedSession(t, rt, sessionAPI, "u1", - time.Now().Add(time.Hour), "t-a") + time.Now().Add(auth.RefreshWindow/2), "t-a") + if err := Store(rt).SaveSessionRefreshToken("rt"); err != nil { + t.Fatal(err) + } c, err := Resolve(rt, Flags{ClientID: "id", ClientSecret: "s", APIURL: stub.url(), Timeout: RequestTimeout}) if err != nil { @@ -164,8 +194,91 @@ func TestFlagsOverASessionLeaveItsCacheAlone(t *testing.T) { if err != nil || tok.AccessToken != sess || !tok.SessionFor(sessionAPI) { t.Fatalf("the session cache was overwritten: %+v, %v", tok, err) } - if _, err := Resolve(rt, Flags{}); err != nil { - t.Errorf("the session no longer resolves: %v", err) + if v, err := Store(rt).SessionRefreshToken(); v != "rt" || + kc.Sets != 1 || kc.Deletes != 0 || auth0.hits() != 0 { + t.Fatalf("flags touched the refresh token: %q, %v, %d sets, "+ + "%d deletes, %d renewals", v, err, kc.Sets, kc.Deletes, + auth0.hits()) + } + // Positive control: without flags the session does renew. + _, _ = Resolve(rt, Flags{}) + if auth0.hits() != 1 { + t.Errorf("the session did not try to renew: %d", auth0.hits()) + } +} + +// TestConcurrentRenewalsRefreshOnce races two renewals of one session, +// as two processes would. Sending a rotating refresh token twice counts +// as reuse, which makes Auth0 revoke the whole session. +func TestConcurrentRenewalsRefreshOnce(t *testing.T) { + var ( + mu sync.Mutex + valid = map[string]bool{"rt-1": true} + calls atomic.Int32 + ) + auth0 := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + n := calls.Add(1) + time.Sleep(50 * time.Millisecond) // let a rival arrive + mu.Lock() + defer mu.Unlock() + if sent := r.PostForm.Get("refresh_token"); valid[sent] { + delete(valid, sent) + } else { + clear(valid) + w.WriteHeader(http.StatusForbidden) + _, _ = io.WriteString(w, `{"error":"invalid_grant"}`) + return + } + next := fmt.Sprintf("rt-%d", n+1) + valid[next] = true + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": testsupport.SessionToken(t, "u1", + time.Now().Add(24*time.Hour), "t-a"), + "refresh_token": next, + }) + })) + t.Cleanup(auth0.Close) + saved := SignInApp + t.Cleanup(func() { SignInApp = saved }) + SignInApp = func(string) (apidefaults.Auth0App, string, bool) { + return apidefaults.Auth0App{ClientID: "cid"}, auth0.URL, true + } + rt, _, _ := testsupport.NewRuntime(t, "", "text") + rt.Keychain = &keychain.Fake{} + testsupport.SeedSession(t, rt, sessionAPI, "u1", + time.Now().Add(auth.RefreshWindow/2), "t-a") + if err := Store(rt).SaveSessionRefreshToken("rt-1"); err != nil { + t.Fatal(err) + } + + errs := make([]error, 2) + var wg sync.WaitGroup + for i := range errs { + proc := *rt + proc.Stderr = &bytes.Buffer{} + wg.Add(1) + go func() { + defer wg.Done() + _, errs[i] = Resolve(&proc, Flags{}) + }() + } + wg.Wait() + for i, err := range errs { + if err != nil { + t.Errorf("renewal %d failed: %v", i, err) + } + } + if calls.Load() != 1 { + t.Errorf("Auth0 got %d refreshes, want 1", calls.Load()) + } + kept, _ := Store(rt).SessionRefreshToken() + mu.Lock() + defer mu.Unlock() + if !valid[kept] { + t.Errorf("the keychain holds %q, which Auth0 no longer honours", + kept) } } @@ -229,6 +342,35 @@ func seedExpiring(in time.Duration) func(*testing.T, *module.Runtime) { } } +// seedRenewable seeds an expired session holding a refresh token, its +// sign-in app answering a refresh with a 403 carrying refusal. An empty +// refusal points the app at a closed port instead. +func seedRenewable(refusal string, ok bool) func(*testing.T, + *module.Runtime) { + return func(t *testing.T, rt *module.Runtime) { + base := "http://127.0.0.1:1" + if refusal != "" { + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = io.WriteString(w, refusal) + })) + t.Cleanup(srv.Close) + base = srv.URL + } + saved := SignInApp + t.Cleanup(func() { SignInApp = saved }) + SignInApp = func(string) (apidefaults.Auth0App, string, bool) { + return apidefaults.Auth0App{ClientID: "cid"}, base, ok + } + rt.Keychain = &keychain.Fake{} + seedExpiring(-time.Minute)(t, rt) + if err := Store(rt).SaveSessionRefreshToken("rt"); err != nil { + t.Fatal(err) + } + } +} + func seedProfileOnly(t *testing.T, rt *module.Runtime) { rt.Config.SetStarfleetProfile(rt.Profile, &config.StarfleetProfile{ APIURL: sessionAPI, Auth: config.AuthBrowser}) diff --git a/internal/starfleet/llms.txt b/internal/starfleet/llms.txt index 0265937..caa4059 100644 --- a/internal/starfleet/llms.txt +++ b/internal/starfleet/llms.txt @@ -33,10 +33,13 @@ A profile signs in one of two ways: - **As a user, through the browser.** `pgedge starfleet auth login` with no credential flags opens the browser, and every command then - runs as that user, in the tenant the profile names, until the - session expires. Nothing renews it: once it expires, a command - fails at exit 5 and tells the user to run `auth login` again. An - agent cannot complete a browser sign-in, so ask the user to run it. + runs as that user, in the tenant the profile names. The session + renews itself while in use and ends after a period without use or + after a fixed maximum however much it is used, or at the time login + printed when no OS keychain could hold it. Once + it ends, a command fails at exit 5 and tells the user to run + `auth login` again. An agent cannot complete a browser sign-in, so + ask the user to run it. - **As an API client,** for CI, scripts and agents. Supply a client ID and secret through one of the sources below. diff --git a/internal/starfleet/llms/auth.txt b/internal/starfleet/llms/auth.txt index 2066a15..8c7916b 100644 --- a/internal/starfleet/llms/auth.txt +++ b/internal/starfleet/llms/auth.txt @@ -44,9 +44,12 @@ It opens the pgEdge sign-in page and prints the same URL on stderr, then waits for the browser to return. A person has to complete the page, so an agent asks the user to run this command. The profile stores `api_url`, `auth: browser` and `tenant_id`; the user's access -token goes to the token cache. Every later command runs as that user -until the session expires. Nothing renews it, so after that a command -fails at exit 5 and names the `auth login` command to run. +token goes to the token cache and the token that renews the session +to the OS keychain. Every later command runs as that user. The +session renews itself while in use and ends after a period without +use or after a fixed maximum however much it is used; where no +keychain could hold it, it ends at the time login printed. Once it ends, a command fails at exit 5 and names the +`auth login` command to run. A user in several tenants is asked which to use when stdin and stdout are a terminal. Otherwise login uses the account's default tenant and @@ -96,8 +99,8 @@ pgedge starfleet auth login --api-client --insecure-storage ``` A browser sign-in ends with four lines on stderr: who signed in -(`Signed in as .`), the tenant, when the session ends, and the -file the profile was saved to. An API-client sign-in ends with +(`Signed in as .`), the tenant, whether the session renews +itself or the time it ends, and the file the profile was saved to. An API-client sign-in ends with `Authenticated. Token expires