Bearer token authentication for downloading remote registries#1356
Bearer token authentication for downloading remote registries#1356jerbly wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1356 +/- ##
=======================================
+ Coverage 81.8% 82.1% +0.2%
=======================================
Files 114 114
Lines 9803 9931 +128
=======================================
+ Hits 8025 8157 +132
+ Misses 1778 1774 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// Optional Bearer token for authenticating HTTP requests when downloading | ||
| /// remote archives and files. Set via `set_http_auth_token()`, typically from | ||
| /// the `WEAVER_HTTP_AUTH_TOKEN` or `GITHUB_TOKEN` environment variable at startup. | ||
| static HTTP_AUTH_TOKEN: Mutex<Option<String>> = Mutex::new(None); |
There was a problem hiding this comment.
Does this work if we start having multi-threaded servers using the weaver engine?
Could we wind up with conflicts for using both HTTP and GITHUB urls at the same time?
There was a problem hiding this comment.
I don't think I follow. You can only have one token with this implementation, do you mean that we may need different tokens to fetch different dependencies?
Regarding URLs, the type is detected for each download.
//! # Disambiguating HTTP(S) URLs
//!
//! An HTTP(S) `source` is classified as follows (in order):
//! 1. `.zip` or `.tar.gz` suffix → remote archive (may carry a `[sub_folder]`).
//! 2. `.git` suffix, or presence of `@refspec` or `[sub_folder]` → Git repo. Once
//! archives are ruled out, a `@refspec` or `[sub_folder]` is a reliable signal
//! of a Git repo, so the `.git` suffix is not required.
//! 3. Otherwise → remote file.
There was a problem hiding this comment.
Yes - the idea we need different for different dependencies, or, e.g. where we're pulling in schema_url dynamically in live-check
There was a problem hiding this comment.
Oof! That will be really ugly - is that realistic? In my case it's company reg depends on otel reg: company needs token, otel does not. This would work today.
We could have company->company->company...->otel too, the same token would be valid in all those private locations.
We'd have to have env vars associated with each private location. I guess that's doable if we think it's needed?? Perhaps defer to another issue/pr later?
There was a problem hiding this comment.
See - https://doc.rust-lang.org/cargo/reference/config.html#credentials or https://maven.apache.org/settings.html#Servers
I think once your weaver.toml stuff lands this would be a good spot for it.
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
I believe there is a non-breaking way to support multiple credentials per-domain you resolve from and this flag at the same time.
Specifically I think we can have a url - token config, similar to cargo, in the future that would "win" over this broad credential config.
Approve-ing but want to confirm you agree with me first, @jerbly. WDYT?
2 participants
Adds HTTP Bearer token authentication for downloading remote registries from private HTTP(S) sources (closes #1344). Primary use case: pulling published registry artifacts (manifest.yaml + resolved.yaml) from private GitHub release assets.
Breaking change: A bare HTTP(S) URL with no .git/.zip/.tar.gz suffix and no refspec or [sub_folder] now parses as RemoteFile instead of GitRepo. Add .git to such URLs to keep the old behaviour.