Skip to content

feat: implement provider for token based auth - #56

Open
cedric-appdirect wants to merge 14 commits into
rolandjitsu:mainfrom
cedric-appdirect:feat/provider-token-auth
Open

cedric-appdirect wants to merge 14 commits into
rolandjitsu:mainfrom
cedric-appdirect:feat/provider-token-auth

Conversation

@cedric-appdirect

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

  • fix
  • feat
  • refactor
  • perf
  • docs
  • test
  • build / ci
  • chore

Summary
This add support for checking access against github/gitlab/... for each git request. This is implementing the feature request from #40 .

Tests

  • Added / updated tests (unit, plus integration where it fits)
  • Not relevant, because: ...

Checklist

  • CI is green locally: cargo fmt --all --check, cargo clippy --all-targets --all-features --locked -- -D warnings, cargo test --all-features
  • Commits follow Conventional Commits; AI-assisted commits carry an Assisted-by: trailer (see CONTRIBUTING.md / AGENTS.md)
  • Preserves the read-only, pull-only invariant (no push or proactive replication to upstream)
  • Docs / README updated if behavior or flags changed

Breaking change?
This should not be introducing any breaking change and it is an opt-in feature.

@codecov

codecov Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.45875% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.83%. Comparing base (623148c) to head (a4b5f7d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/auth.rs 98.29% 14 Missing ⚠️
src/server.rs 98.24% 2 Missing ⚠️
src/lfs.rs 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   97.12%   97.83%   +0.70%     
==========================================
  Files           6        9       +3     
  Lines        2333     3322     +989     
==========================================
+ Hits         2266     3250     +984     
- Misses         67       72       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cedric-appdirect
cedric-appdirect force-pushed the feat/provider-token-auth branch from fe85681 to 813bb0f Compare October 26, 2026 20:11
@armas-mk

Copy link
Copy Markdown

I'm planning to come up with a design to drop the shared upstream token entirely and have the proxy clone/fetch with the requesting client's credential (e.g. CI_JOB_TOKEN), so no long-lived group-level token is needed. Every upstream call is already request-triggered and gated by your check, so this layers on top of this PR with little change. I will open a separate issue with the design, so it does not have to block this PR.

The upstream-shaped credential is built inline in Provider::request and then dropped: Verdict carries no payload, and gate only inserts Route. Would you be open to:

  1. Extracting the Check::Upstream header construction (Basic as sent, Bearer -> Basic oauth2: / x-access-token:) into its own function that Provider::request calls, and
  2. Exposing it, e.g. pub fn Auth::upstream_credential(&self, headers: &HeaderMap) -> Option<HeaderValue> (sensitive, None outside github/gitlab).

Nothing would consume it yet, so behavior is unchanged. The benefit is a single definition of "the header upstream sees for this client", which the follow-up can reuse without forking the conversion, and the check and the fetch can never disagree. Happy to send a commit for it if that is easier.

A related note for the follow-up, not a change request here: rewrite_download_hrefs replaces upstream's expires_at with the 15 minute grant expires_in. That is right while the proxy re-batches with its own credential. Without a static credential, the advertised expiry would need to be capped by upstream's, so I may need the grant callback to see it later.

@rolandjitsu rolandjitsu changed the title Feat/provider token auth feat: implement provider for token based auth Oct 31, 2026

@rolandjitsu rolandjitsu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feature contribution @cedric-appdirect ! I left a few minor comments.

Also, 55d2dd0, needs the agent authoring footer.

Comment thread tests/http.rs Outdated
use tower::ServiceExt; // for `oneshot`

fn state(serve_token: Option<String>) -> AppState {
fn anon() -> Arc<Auth> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have helper fns after the tests - I read the tests before I check the helpers used in them.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, anon_auth might be a better name for the method. Similarly, shared_token_auth for the other.

Comment thread tests/helpers/mod.rs

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just be a helpers.rs in the root of tests/.

Comment thread src/config.rs
Path,
/// `owner%2Frepo`: strip `.git`, then encode each `/` (single-parameter
/// style, as GitLab's `/projects/{id}` wants).
Percent,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to call this EncodedPath ?

Comment thread src/metrics.rs Outdated
TextEncoder,
};

use crate::auth::Verdict;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the types used b/t metrics and auth into a separate mod?

Comment thread Cargo.lock

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this commit carries 2 coding agent footers:

Assisted-by: Cursor:glm-5.3
Co-authored-by: Cursor <cursoragent@cursor.com>

Let's remove Co-authored-by ....

Comment thread src/auth.rs
/// nothing. Only a credential `check` already allowed should be forwarded, and
/// it is distinct from the proxy's own `--upstream-auth-header`.
#[must_use]
pub fn forwarded_credential(&self, headers: &HeaderMap) -> Option<HeaderValue> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in tests, is the intent to be used only in tests?

@cedric-appdirect cedric-appdirect Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was to serve @armas-mk request. Let me know if you want this moved away.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #68 ([FEATURE] Fetch upstream with the client's credential instead of a shared token) for more context

Comment thread README.md Outdated
| `--upstream-auth-header` | `GITCACHEPROXY_UPSTREAM_AUTH_HEADER` | - | Full HTTP header injected on upstream clone/fetch, e.g. `Authorization: Basic <base64>` (see Auth model); injected via env so it stays out of argv |
| `--serve-token` | `GITCACHEPROXY_SERVE_TOKEN` | - | If set, clients must send `Authorization: Bearer <token>` |
| `--auth-provider` | `GITCACHEPROXY_AUTH_PROVIDER` | `none` | Client-auth mode: `none`, `shared`, `github`, `gitlab` (see Auth model). Passing only `--serve-token` keeps the legacy shared-secret behavior |
| `--auth-timeout-seconds` | `GITCACHEPROXY_AUTH_TIMEOUT_SECONDS` | `10` | Total budget for one token-validation request |

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout applies independently to the permit wait and the HTTP call, so the worst case is ~2x of this?

Comment thread src/auth.rs Outdated
AuthProvider::None | AuthProvider::Shared => match (serve_token, provider) {
// Legacy shape: only `--serve-token` given, no explicit mode.
(Some(token), _) => Mode::Shared { token },
(None, AuthProvider::None) => Mode::Anonymous,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we warn here with something like serve token is blank; serving anonymously?

@rolandjitsu

rolandjitsu commented Oct 31, 2026 •

Copy link
Copy Markdown
Owner

Also note that I took the CVE fix commit into #63. You'll need a rebase.

Model client auth as an `Auth` value outside server.rs, so additional
client-auth modes can live beside the shared-secret one.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
Auth then checks exactly the repo that is served. Pushes and dumb-http
info/refs are now refused before auth, so they get 403 or 400, not 401.

Assisted-by: Cursor:glm-5.3
A shared serve token exposes every repo the upstream credential can read.
The github and gitlab providers check each request's own credential for its
repo through upstream's info/refs, which only answers a credential that can
clone. LFS object GETs are refused in these modes for now.

Assisted-by: Cursor:glm-5.3
Provider modes check the batch request against upstream, then hand out an
HMAC grant bound to repo, oid, and size, so object GETs stay local and
cannot reach another repo's objects in the shared cache.

Assisted-by: Cursor:glm-5.3
For upstreams whose git endpoint cannot answer the check, `custom` asks any
HTTP endpoint, with the token header and repo encoding it wants.

Assisted-by: Cursor:glm-5.3
@cedric-appdirect
cedric-appdirect force-pushed the feat/provider-token-auth branch from 4f016d0 to b0f3793 Compare November 3, 2026 16:05
Provider::request built the upstream Authorization header inline. Move
it to Credential::forwarded, sensitive on the way out, so a caller
cannot forward a token in the clear, and expose it as
Auth::forwarded_credential. It reshapes only: no probe, no verdict, so a
caller must still have passed check.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
@cedric-appdirect
cedric-appdirect force-pushed the feat/provider-token-auth branch from b0f3793 to 01b15e9 Compare November 3, 2026 16:07
cedric-appdirect and others added 6 commits November 3, 2026 10:16
The CI `commits` job runs `crate-ci/committed`, whose default body line
limit is 72, so a body wrapped to a wider width fails the check. State
the limit and the unwrappable-line exemption where the commit guidance
already lives.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
Read the tests first, then the helpers they use. `anon`/`shared` also
read as verbs; `anon_auth`/`shared_token_auth` name the mode each one
builds.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
Co-authored-by: Cursor <cursoragent@cursor.com>
"helpers" says what the module is. It stays a `mod.rs` rather than a
bare `tests/helpers.rs`, which Cargo auto-discovers as its own test
binary and so compiles twice.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
`RepoForm::Path` reads as "the path form is encoded", which it is not.
Both the type and `--auth-repo-encoding` are new in this PR, so the
rename costs no migration.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
The only thing metrics needed from auth was one From impl, which an impl
may live anywhere in the crate. Moving it beside Verdict makes the two
modules one-way: auth depends on metrics, never the reverse.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
The slot wait and the HTTP call each got the full --auth-timeout budget,
so one check could take twice the configured value. Share a single
deadline instead, and fail closed when the wait leaves nothing: reqwest
reads a zero timeout as no timeout at all.

Also warn when --serve-token is set but blank, which otherwise switches
to anonymous silently.

Assisted-by: Cursor:deepseek-v4.1-flash-1017
@cedric-appdirect
cedric-appdirect force-pushed the feat/provider-token-auth branch from 90166d5 to a4b5f7d Compare November 3, 2026 17:11
@cedric-appdirect

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I have rebased the PR and I think addressed all your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants