Conversation
141b10f to
9ccf909
Compare
| // NOTE: Trusted Access tokens are not stored on disk; this lockfile | ||
| // synchronizes the in-memory token refresh. | ||
| PyxTokens::TrustedAccess(_) => self.subdirectory.join("trusted-access.lock"), |
There was a problem hiding this comment.
Flagging: I'm pretty sure this is effectively a no-op, but it seemed easier to fit things into the existing lockfile pattern rather than carve out a special case for TA.
(Another option here would be to cache the temporary TA token on disk, keyed by workspace name.)
crates/uv-auth/src/pyx.rs
Outdated
| .ok_or_else(|| { | ||
| // This can only happen if we're in an environment that previously | ||
| // offered us an OIDC token, but is no longer detected. | ||
| // This strongly suggests some kind of runtime meddling by the user. | ||
| // TODO: Custom error type here? | ||
| TokenStoreError::Io(io::Error::other("could not refresh Trusted Access token: no ambient OIDC credentials detected")) |
There was a problem hiding this comment.
Flagging: this is an illegal/impossible state: it implies the we initially detected an ambient OIDC environment, but then subsequent refreshes failed to detect it. AFAICT the only way this can really happen is if someone meddles with a CI runner/runtime environment (like deleting needed environment variables).
There was a problem hiding this comment.
There's a group of these errors we have in uv: We know it shouldn't happen, but it's an OS API, so it can happen, and due to the heterogeneity of users it might just happen somewhere. (For a server, we could unwrap and only ever look at it if it shows up in sentry.) A similar case is that std::path::absolute fails because it couldn't detect a CWD. We generally just raise to those errors with a basic message, it doesn't make sense to optimize further around them, unless we get an actual user report.
I'd create a custom error variant though that has the TokenStoreError as source.
477e81f to
42e0c35
Compare
|
This is working, I just need to make the integration tests a bit more verbose + more detailed. But ready for a functional review 🙂 |
There was a problem hiding this comment.
Note: I've done this with a separate integration testing script rather than adding it to registries-test.py since it doesn't fit the same shape as the other registry tests -- we don't have an API key or user/pass pair, for example.
(I want to refactor those tests more thoroughly, though -- I think it'd be ideal to unify this, the general registries tests, and the publish test into a single plan-execute style integration test scheme.)
| // Derive the workspace from the URL. All known pyx URL shapes — Simple API | ||
| // (`/simple/{workspace}/{view}`) and CDN — encode the workspace name. | ||
| let Some(workspace) = token_store.workspace_for_url(url).map(str::to_owned) | ||
| else { | ||
| // URL is recognized as pyx but workspace isn't extractable (shouldn't | ||
| // happen in practice); fall through to other auth methods. | ||
| debug!("Could not extract workspace from pyx URL {url}"); | ||
| self.cache().fetches.done(key.clone(), None); | ||
| return None; | ||
| }; |
There was a problem hiding this comment.
Noting: the workspace discovery here should maybe be a hard error instead, since it basically implies the user has given us a broken index or CDN URL anyways.
There was a problem hiding this comment.
Could this branch surface a bug, such as update on the pyx side not compatible with the client? Especially since the return None prevents fallthrough to the other authentication methods.
There was a problem hiding this comment.
Yeah, I think it could. In that case falling through to the other methods makes sense, I'll adapt.
| if: ${{ github.event.pull_request.head.repo.fork != true }} | ||
|
|
||
| permissions: | ||
| id-token: write # for Trusted Access |
There was a problem hiding this comment.
Shouldn't this be read? It at least feels wrong having to say write when you mean read
There was a problem hiding this comment.
Unfortunately this is GitHub being backwards: the "read" permission does nothing for id-token, while "write" provides read access to the token.
I think they did it that way because it's a powerful permission, and they didn't want people who do read-all to accidentally overexpose themselves. But it's extremely confusing, and I wish they had just called it id-token: access or true or something else.
crates/uv-auth/src/pyx.rs
Outdated
| if url.scheme() != api.scheme() | ||
| || url.host_str() != api.host_str() | ||
| || url.port_or_known_default() != api.port_or_known_default() |
crates/uv-auth/src/pyx.rs
Outdated
| /// - CDN files: `https://files.{cdn}/{workspace}/{path}` | ||
| /// | ||
| /// Returns `None` if the URL does not match any known shape for this store. | ||
| pub fn workspace_for_url<'a>(&self, url: &'a DisplaySafeUrl) -> Option<&'a str> { |
There was a problem hiding this comment.
nit: No need to manage lifetimes here, we can return an owned String
There was a problem hiding this comment.
Will do. Q about idioms: do we generally prefer not to manage lifetimes like this? I tend to default towards borrowing/"view" patterned APIs to avoid allocations, but I can imagine they're not a real issue in these flows given that network calls will dominate performance.
There was a problem hiding this comment.
It's not super consistent in uv, my rule of thumb is to use borrowed types mostly too, but only use lifetime annotations when it matters for performance, otherwise they tend to introduce more complexity to the signature than they are worth.
|
|
||
| // views.{cdn}/v1/{shard}/{workspace}/{view}/... | ||
| if host.strip_prefix("views.") == Some(cdn) { | ||
| if segments.next()? != "v1" { |
There was a problem hiding this comment.
Does this means that when we switch to v2, older clients could lose access?
There was a problem hiding this comment.
Yes, although I think that would be similar to other pyx incompatibilities -- we'd probably have a pretty long transition period to help people off-ramp from the v1 URLs, including adding support for "v2" (or similar) to uv prior to rolling it out on pyx.
| // Derive the workspace from the URL. All known pyx URL shapes — Simple API | ||
| // (`/simple/{workspace}/{view}`) and CDN — encode the workspace name. | ||
| let Some(workspace) = token_store.workspace_for_url(url).map(str::to_owned) | ||
| else { | ||
| // URL is recognized as pyx but workspace isn't extractable (shouldn't | ||
| // happen in practice); fall through to other auth methods. | ||
| debug!("Could not extract workspace from pyx URL {url}"); | ||
| self.cache().fetches.done(key.clone(), None); | ||
| return None; | ||
| }; |
There was a problem hiding this comment.
Could this branch surface a bug, such as update on the pyx side not compatible with the client? Especially since the return None prevents fallthrough to the other authentication methods.
crates/uv-auth/src/middleware.rs
Outdated
| .await | ||
| { | ||
| Ok(Some(token)) => Some(token), | ||
| Ok(None) => None, |
There was a problem hiding this comment.
Preexisting but I noticed it here (potential follow-up PR): What does it mean if bootstrapping return None for the user, will they get an opaque 401/403 later when the actual request runs?
There was a problem hiding this comment.
Yeah, I think it means that they'll get a subsequent opaque error, which isn't ideal. Maybe we should make this a harder error here?
There was a problem hiding this comment.
Sounds good, I'll follow you with your knowledge of the pyx auth flow.
crates/uv-auth/src/pyx.rs
Outdated
| .ok_or_else(|| { | ||
| // This can only happen if we're in an environment that previously | ||
| // offered us an OIDC token, but is no longer detected. | ||
| // This strongly suggests some kind of runtime meddling by the user. | ||
| // TODO: Custom error type here? | ||
| TokenStoreError::Io(io::Error::other("could not refresh Trusted Access token: no ambient OIDC credentials detected")) |
There was a problem hiding this comment.
There's a group of these errors we have in uv: We know it shouldn't happen, but it's an OS API, so it can happen, and due to the heterogeneity of users it might just happen somewhere. (For a server, we could unwrap and only ever look at it if it shows up in sentry.) A similar case is that std::path::absolute fails because it couldn't detect a CWD. We generally just raise to those errors with a basic message, it doesn't make sense to optimize further around them, unless we get an actual user report.
I'd create a custom error variant though that has the TokenStoreError as source.
| "gcp", | ||
| "gemfury", | ||
| "gitlab", | ||
| "pyx", |
There was a problem hiding this comment.
Flagging: I've added a pyx registry test here (with a read-only API key) to ensure that the TA changes don't interfere with normal auth.
| /// so the key is always present. Each workspace gets its own entry so that Trusted Access | ||
| /// tokens — which are minted per-workspace — are cached and refreshed independently. A | ||
| /// missing entry means the token has not yet been fetched for that workspace. | ||
| pyx_token_state: Mutex<FxHashMap<String, Option<AccessToken>>>, |
There was a problem hiding this comment.
Flagging: I'm not super happy with this approach, because (if I'm tracking our layers correctly) it pushes API key and access token auth into the same per-workspace shape as Trusted Access. This won't affect most users, but for the odd user who has multiple pyx workspaces this means that we'll perform multiple access token exchanges for the same logical API key.
I think the more correct thing to do here is go back to a Mutex<TokenState>, where TokenState is something like:
enum TokenState {
/// The token state has not yet been initialized from the store.
Uninitialized,
/// The token state has been initialized, and the store either
/// returned tokens (corresponding to a 'global' authentication method
/// like `uv auth login` or `PYX_API_KEY`) or `None` if the user has
/// not yet authenticated.
Global(Option<AccessToken>),
/// The token state has been initialized, and the store either
/// returned Trusted Access tokens (which are associated with individual
/// pyx workspaces) or `None` if the token has not yet been fetched for
/// a particular workspace.
Workspace(FxHashMap<String, Option<AccessToken>>),Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Summary
WIP.The idea here is to allowuv ...invocations that touch pyx indices to perform a Trusted Access token exchange on the fly, meaning the user doesn't have to bother withuv auth loginor an API token when on a supported platform.The wrinkle with Trusted Access is that it's (currently) keyed to a workspace, so a user who has a dependency topology that contains multiple pyx workspaces will need to perform credential exchange against each. This is accommodated internally by turning
pyx_token_stateinto a map of workspaces to access tokens. However, I'm not convinced this is a good idea -- it complicates the internal authentication model quite a bit (and makes CDN authentication more confusing), all for a pretty rare topology that also doesn't work with API tokens or other auth forms. I'll discuss some alternatives out-of-band.Test Plan
Needs unit and integration tests.I've added an initial integration test, will add more.