Repository navigation
Fetch the CLI's token from cli.mapbox.com instead of building it in - #99
Conversation
cf5aca2 to
b359936
Compare
mattpodwysocki
left a comment
mattpodwysocki
left a comment
There was a problem hiding this comment.
Nice work, the shape of this is good: the fetch stays in the detached telemetry child so nothing in the foreground waits on it, only pk. tokens are kept (checked on fetch and again on read), and the user's token never goes near the endpoint. A couple of things on rotation inline, plus one behavior change I want to make sure is intended:
Builds from source and CI runs with no user token used to drop telemetry (the old README said so). Since every build now fetches from production, they'll send it on the CLI's token instead, so contributor and CI traffic lands in the same data as real users. Fine if we filter by version downstream, but worth deciding on purpose.
Also a heads up for when #47 lands: MAPBOX_CLI_NO_TELEMETRY is the only thing gating the fetch here, so config set telemetry off will need to gate the same spot.
|
|
||
| /// The decision, with the clock and the network passed in. | ||
| fn acquire(state: &mut State, now: u64, get: impl FnOnce() -> Fetched) -> Option<String> { | ||
| if let Some(token) = state.usable_token() { |
There was a problem hiding this comment.
Once we hold a token, we keep it until the Tokens API calls it dead, so a rotation only reaches clients if the old one is revoked. If ops rotates but leaves the old token valid for a bit, or tightens its scopes so events start answering 403 while the Tokens API still says valid, every machine keeps sending the old one and the Forbidden cooldown just rests for an hour and tries again, forever. Could a Forbidden/Unknown cooldown also re-fetch and switch if the endpoint serves something different? Or re-fetch once the kept token is a day or so old? Either would make "rotating needs no release" true without having to revoke.
There was a problem hiding this comment.
We'll make revoking part of rotation: publish the new token, wait for the endpoint to serve it, then revoke the old one. A revoked token is replaced on its next refusal. Scope or URL changes go through the same path rather than editing the published token, so a valid-but-refused token isn't an expected state. Added this to the PR description.
The token is published at https://cli.mapbox.com/config/token.json (mapbox/mapbox-cli-private#207), so rotating it needs no release. Every build fetches it when there is none on disk, and keeps it in <config dir>/cli-token.json. A 401 or 403 to it is checked with the Tokens API. Only a revoked, invalid, expired or malformed token is replaced; a valid one that an API refuses is kept. If the endpoint still serves the dead token, it is asked again hourly three times, then daily. A fetch that gets no answer at all is retried on the next run. CLI_TOKEN_COMMANDS lists the commands that prefer the CLI's token, falling back to the user's while it can't be used. With no token of the user's either, the command fails with cli_token_unavailable, cli_token_forbidden or cli_token_not_allowed. Every end-to-end test points MAPBOX_INTERNAL_CLI_TOKEN_URL at loopback, held by a new source guard. MAPBOX_CLI_BUNDLED_TOKEN is no longer read.
- Read the Tokens API verdict whatever the status, so a dead token's code is not lost if the endpoint ever answers it with a 401. - Trust a "valid but refused" verdict for an hour, so telemetry refused by Events does not cost a Tokens API check on every run. - Say whether a wait is for a revoked token or an endpoint that never returned a usable one. - Create the config directory when keeping the token needs it, so the retry schedule holds on a machine without ~/.mapbox. An existing one keeps its mode. - Don't ask for confirmation again when a refused request is resent with the user's token, and say when a typed --token went unused. - Drop cli_token_not_allowed: for_command now returns None for a command that is not listed, so it was never reachable. - Share the production-build URL override with telemetry in http.rs. - Fix the module and State docs that still described the old rules.
- Check every refusal with the Tokens API at once, dropping the hour-long "valid" verdict that could hide a revocation. A dead token is replaced at once and the request resent, telemetry included. - When an API refuses a valid token, that caller (telemetry, or the command) rests from it on the fetch's schedule: an hour three times, then a day. While resting it sends nothing with the token and checks nothing; a request that goes through ends the rest. - Write a verdict only if the saved token is still the one checked, so a token another run saved meanwhile is not overwritten. - Never send MAPBOX_CLI_TOKEN to the Tokens API. - Keep state from a build with an endpoint overridden apart from a release's. - Hold MAPBOX_INTERNAL_TOKENS_URL at loopback in every test, in the guard. - Keep the request id when a refetch after a dead token fails. - Say when --token or --use-login went unused, whatever the result. - One plain is_production_build, shared with update_check. - Changelog: list what changes for an existing setup.
No command uses the CLI's token yet, so the fallback to the user's token, its retries and its two error codes could not be reached or tested here. They come with mapbox report, the first command to use them. The old "CLI's token when the user has none" fallback for listed commands, whose list was empty, goes with them. Also from the third review: - Only a 2xx ends a cooldown. A timeout or 5xx says nothing about whether the API takes the token, and used to reset the count. - token() writes only if no other run changed the state while it fetched. - Only the fetched token's answers touch the state file; the user's tokens and MAPBOX_CLI_TOKEN never cost a read of it. - One check for "a wait still holds", shared by retry and cooldown.
- A refusal the Tokens API cannot explain rests the caller too, so telemetry does not resend and recheck on every run. - A fetch merges with what another run wrote meanwhile instead of dropping its own result: the other run's token or count stands, its cooldowns are kept unless this fetch brought a new token. - Drop Unavailable and its wording, which nothing shows until a command uses the CLI's token. - Docs: the token is kept until the Tokens API calls it dead; token() has no fetch flag; url_override is for requests that carry a token. - Test that MAPBOX_CLI_TOKEN is trimmed before it is sent.
- Time only the POST that answered in the delivery log, not the token fetch or check before it. - README: describe the cooldown as the cycle it is, and that an unexplained refusal starts one. - Fix docs that still named a command caller, NotFetched's cause, and a broken doc link. - check_token takes no URL; it only ever had one.
The published token is restricted to https://cli.mapbox.com/, and an API answers 401 to a request with it that does not say it comes from there. Telemetry's send and the Tokens API check now carry that Referer whenever they use the CLI's token; the user's tokens never do.
The endpoint's backoff and a caller's cooldown share one rule: an hour after every third consecutive miss, nothing otherwise. A valid token an API refuses is usually a configuration someone fixes in the Console without the token changing, and a fix should not wait a day to be seen.
…h test on telemetry off A token called invalid for something fixed in the Console stays the same string, so when the endpoint serves the rejected token again it is checked once more and used if the Tokens API now calls it valid. Also drops leftover "a day" wording now that the longest wait is an hour, and adds an end-to-end test that `config set telemetry off` fetches no token.
b359936 to
0e022fc
Compare
|
@mattpodwysocki On telemetry from source builds: intended. Events carry |
mattpodwysocki
left a comment
mattpodwysocki
left a comment
There was a problem hiding this comment.
Thanks for working through these. Making revoke part of rotation works for me now that it's documented. One optional nit: check(&token) == Refusal::Forbidden reads like a refusal when it means valid, so a short comment or an is_valid helper might help the next reader. Not blocking.
2 participants
The CLI now fetches its own token from
https://cli.mapbox.com/config/token.json(mapbox/mapbox-cli-private#207) and keeps it in~/.mapbox/cli-token.json, so rotating it needs no release.MAPBOX_CLI_BUNDLED_TOKENis no longer read.cargo buildincluded.MAPBOX_INTERNAL_CLI_TOKEN_URLoverrides the URL outside production builds.401/403to the CLI's token is checked with the Tokens API at once. A revoked, invalid, expired or malformed token is replaced at once and the request resent. A valid token that an API refuses counts against that caller; the third refusal in a row rests it for an hour. Three fetches in a row that find no usable token, such as the endpoint still serving the dead one, pause fetching for an hour the same way. No wait is ever longer than an hour. A fetch with no answer at all is retried on the next run. When the endpoint serves the dead token again, it is checked once more and taken back if it is now valid, so a fix in the Console without a new token is picked up.mapbox reportin a stacked PR.Referer: https://cli.mapbox.com/. The published token is restricted to that URL, and an API answers401 Invalid access tokenwithout it. The user's tokens never carry it.~/.mapboxwhen it is missing, so the retry schedule holds; an existing one keeps its mode.MAPBOX_INTERNAL_CLI_TOKEN_URLandMAPBOX_INTERNAL_TOKENS_URLat loopback, held by a new source guard.Testing: unit tests for the retry schedule and the Tokens API verdicts; end-to-end tests against loopback servers for fetch and keep, replacing a revoked token and resending in the same run, the cooldown after the third refusal and that a failed send does not end it, waiting while the endpoint still serves the dead token, creating
~/.mapboxwithout changing an existing one's mode, taking back the dead token once it checks valid, and fetching nothing afterconfig set telemetry off. Broke each fix on purpose and watched its test fail. Checked by hand that the real endpoint returns apk.token and that the Tokens API answersTokenValid,TokenInvalidandTokenMalformedfor it and broken variants.Not tested: a real
TokenRevoked/TokenExpiredresponse, and a release build against production.