fix(oauth): validate OAuth token/base URLs and make user agent overrideable - #101
Merged
Conversation
…dable
- A downstream code review of `pkg/tscli`/`pkg/oauth` as a library found
two ways an embedding consumer's secrets or metadata could leak:
`TSCLI_OAUTH_TOKEN_URL` was read with no validation and checked before
an explicit `base-url`, so it could silently redirect an OAuth client
secret to an attacker-controlled host over plain HTTP; and the
`User-Agent` sent on every API request was derived from the calling
process's local git repository state with no way for a library
consumer to override it.
- `pkg/oauth.ValidateTokenURL` now requires `https://`, or `http://`
restricted to a loopback host, and is applied to both the OAuth token
endpoint and `base-url` (`pkg/tscli/client.go`'s `validateBaseURL` now
delegates to it). The OAuth token endpoint is derived from the
validated `base-url` first; `TSCLI_OAUTH_TOKEN_URL` remains a
validated, explicit override, not a silent one.
- `New()` also disables HTTP redirect following (`CheckRedirect` returns
`http.ErrUseLastResponse`) so a redirected request can't carry the
OAuth bearer token or API key to an unconfigured host.
- Library and CLI callers can now set a `user-agent` viper key (flag/env
`TSCLI_USER_AGENT`/config) to replace the git-derived default, so
embedding a repo other than `tscli` no longer leaks that repo's tag,
commit hash, and dirty state to Tailscale's API.
- `cmd/tscli/create/tailnet`, `cmd/tscli/create/token`,
`cmd/tscli/delete/tailnet`, and `cmd/tscli/list/tailnets` now call the
new `tscli.ExchangeOAuthClientCredentials`, which derives the token URL
from the same `base-url` configuration `New()` uses, instead of calling
`oauth.ExchangeClientCredentials` directly with an unvalidated default.
- `pkg/contract/openapi` and `coverage/coverage-gaps.*` were refreshed
against Tailscale's live OpenAPI schema (`make coverage-gaps-latest`),
picking up 3 new operations (tailnets list/create/delete) and a new
`vipServices[].displayName` property; unrelated to the security fix.
- [x] `pkg/oauth/exchange_test.go` — new tests cover the default public
token endpoint, a validated loopback override, rejection of an
unsafe `TSCLI_OAUTH_TOKEN_URL` override, `ValidateTokenURL`, and
`ResolveTokenURL`
- [x] `pkg/tscli/client_test.go` — new coverage for `base-url`/token-URL
validation and resolution
- [x] `test/cli/*` — lifecycle and example-output integration tests
updated to derive the OAuth token endpoint from `TSCLI_BASE_URL`
instead of a separately set `TSCLI_OAUTH_TOKEN_URL`
- [x] `make lint` passes with zero issues
- [x] `make test` passes
- `pkg/oauth/exchange.go` - Add `ValidateTokenURL`/`ResolveTokenURL`,
require `https://` (or loopback `http://`) for both the default and
`TSCLI_OAUTH_TOKEN_URL`-overridden token endpoint
- `pkg/tscli/client.go` - Validate `base-url` via `ValidateTokenURL`,
derive the OAuth token endpoint from it, add
`ExchangeOAuthClientCredentials`, support a `user-agent` viper
override, and disable redirect following on the HTTP client
- `internal/cli/root.go` - Bind `TSCLI_USER_AGENT` to the `user-agent`
viper key
- `cmd/tscli/create/tailnet/cli.go`, `cmd/tscli/create/token/cli.go`,
`cmd/tscli/delete/tailnet/cli.go`, `cmd/tscli/list/tailnets/cli.go` -
Use `tscli.ExchangeOAuthClientCredentials` instead of calling
`oauth.ExchangeClientCredentials` directly
- `cmd/tscli/delete/users/cli_test.go` - Use `https://fake` as the stub
base URL to match the tightened `base-url` validation
- `README.md` - Document `base-url`/`user-agent` validation and the
`TSCLI_USER_AGENT` override
- `pkg/oauth/exchange_test.go`, `pkg/tscli/client_test.go`,
`test/cli/example_output_test.go`,
`test/cli/tailnet_lifecycle_integration_test.go`,
`test/cli/unauthenticated_commands_test.go` - Test coverage for the
above
- `pkg/contract/openapi/tailscale-v2-openapi.yaml`,
`pkg/contract/openapi/command-operation-map.yaml`,
`pkg/contract/openapi/snapshot-metadata.yaml`,
`coverage/coverage-gaps.json`, `coverage/coverage-gaps.md` - Refresh
the OpenAPI snapshot and coverage report against Tailscale's current
API (`make coverage-gaps-latest`)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A downstream code review of
pkg/tscli/pkg/oauthas a library found two ways an embedding consumer's secrets or metadata could leak:TSCLI_OAUTH_TOKEN_URLwas read with no validation and checked before an explicitbase-url, so it could silently redirect an OAuth client secret to an attacker-controlled host over plain HTTP; and theUser-Agentsent on every API request was derived from the calling process's local git repository state with no way for a library consumer to override it.pkg/oauth.ValidateTokenURLnow requireshttps://, orhttp://restricted to a loopback host, and is applied to both the OAuth token endpoint andbase-url(pkg/tscli/client.go'svalidateBaseURLnow delegates to it). The OAuth token endpoint is derived from the validatedbase-urlfirst;TSCLI_OAUTH_TOKEN_URLremains a validated, explicit override, not a silent one.New()also disables HTTP redirect following (CheckRedirectreturnshttp.ErrUseLastResponse) so a redirected request can't carry the OAuth bearer token or API key to an unconfigured host.Library and CLI callers can now set a
user-agentviper key (flag/envTSCLI_USER_AGENT/config) to replace the git-derived default, so embedding a repo other thantsclino longer leaks that repo's tag, commit hash, and dirty state to Tailscale's API.cmd/tscli/create/tailnet,cmd/tscli/create/token,cmd/tscli/delete/tailnet, andcmd/tscli/list/tailnetsnow call the newtscli.ExchangeOAuthClientCredentials, which derives the token URL from the samebase-urlconfigurationNew()uses, instead of callingoauth.ExchangeClientCredentialsdirectly with an unvalidated default.pkg/contract/openapiandcoverage/coverage-gaps.*were refreshed against Tailscale's live OpenAPI schema (make coverage-gaps-latest), picking up 3 new operations (tailnets list/create/delete) and a newvipServices[].displayNameproperty; unrelated to the security fix.pkg/oauth/exchange_test.go— new tests cover the default public token endpoint, a validated loopback override, rejection of an unsafeTSCLI_OAUTH_TOKEN_URLoverride,ValidateTokenURL, andResolveTokenURLpkg/tscli/client_test.go— new coverage forbase-url/token-URL validation and resolutiontest/cli/*— lifecycle and example-output integration tests updated to derive the OAuth token endpoint fromTSCLI_BASE_URLinstead of a separately setTSCLI_OAUTH_TOKEN_URLmake lintpasses with zero issuesmake testpassespkg/oauth/exchange.go- AddValidateTokenURL/ResolveTokenURL, requirehttps://(or loopbackhttp://) for both the default andTSCLI_OAUTH_TOKEN_URL-overridden token endpointpkg/tscli/client.go- Validatebase-urlviaValidateTokenURL, derive the OAuth token endpoint from it, addExchangeOAuthClientCredentials, support auser-agentviper override, and disable redirect following on the HTTP clientinternal/cli/root.go- BindTSCLI_USER_AGENTto theuser-agentviper keycmd/tscli/create/tailnet/cli.go,cmd/tscli/create/token/cli.go,cmd/tscli/delete/tailnet/cli.go,cmd/tscli/list/tailnets/cli.go- Usetscli.ExchangeOAuthClientCredentialsinstead of callingoauth.ExchangeClientCredentialsdirectlycmd/tscli/delete/users/cli_test.go- Usehttps://fakeas the stub base URL to match the tightenedbase-urlvalidationREADME.md- Documentbase-url/user-agentvalidation and theTSCLI_USER_AGENToverridepkg/oauth/exchange_test.go,pkg/tscli/client_test.go,test/cli/example_output_test.go,test/cli/tailnet_lifecycle_integration_test.go,test/cli/unauthenticated_commands_test.go- Test coverage for the abovepkg/contract/openapi/tailscale-v2-openapi.yaml,pkg/contract/openapi/command-operation-map.yaml,pkg/contract/openapi/snapshot-metadata.yaml,coverage/coverage-gaps.json,coverage/coverage-gaps.md- Refresh the OpenAPI snapshot and coverage report against Tailscale's current API (make coverage-gaps-latest)