Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing Auth With Redirects #25385

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CauhxMilloy
Copy link

This addresses an issue where HTTP redirects for ctx.download()/ctx.download_and_extract() do not properly forward auth headers after an HTTP redirect (3** status code). This was due to auth headers being tied to the exact URL given in the urls parameter.

This PR addresses this by additionally tying auth data to the host name (matching what is given in auth). This is backwards compatible with existing "exact URL matching" functionality by having the host auth functionality largely function as a fallback.

For more detail see: #25068

Changes:

  • Updating use_netrc() in tools/build_defs/repo/utils.bzl.
    • The resultant dict contains keys for both exact urls (just as it did before) and the protocol + hosts to match to patterns dict.
      • Must include protocol because Java-side converts these strings to URLs (requires protocol).
      • This does not include hosts from patterns which do not appear in the urls at all (so redirects would need to be still among the same set of hosts).
    • Updating Returns section of doc string to mention the pattern hosts.
    • This will affect/fix functionality for repo rules such as http_archive() and others which use use_netrc() (possibly through get_auth()) for generating auth data.
  • Updating HttpConnectorMultiplexer.getHeaderFunction() in src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java.
    • Adding host-based fallback checks.
      • This will check the given credentials for both the exact URI and a URI of only the host, where the host is a fallback (overridden by the exact URI).
      • The host fallback URI must include protocol, due to elsewhere converting String->URL->URI (URL requires protocol).
    • This will pick up the auth data originating from use_netrc() (and passed through various functions/transformations).
    • If the host name is not given (for some other call to download functionality), then the exact URI will be used only (just like the existing flow).
  • Updating download() and download_and_extract() doc strings in src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java.
  • Updating tests.
    • Updating test_use_netrc test case in src/test/shell/bazel/starlark_repository_test.sh.
      • Adding hosts (with protocol) into expected dict auth data.
    • Adding test assertions for auth header logic for testHeaderComputationFunction() in src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java.

Fixes #25068.

This addresses an issue where HTTP redirects for `ctx.download()`/`ctx.download_and_extract()` do not properly forward auth headers after an HTTP redirect (3** status code).
This was due to auth headers being tied to the exact URL given in the `urls` parameter.
This PR addresses this by additionally tying auth data to the host name (matching what is given in `auth`).
This is backwards compatible with existing "exact URL matching" functionality by having the host auth functionality largely function as a fallback.

For more detail see: bazelbuild#25068

Changes:
* Updating `use_netrc()` in `tools/build_defs/repo/utils.bzl`.
  * The resultant dict contains keys for both exact urls (just as it did before) and the protocol + hosts to match to `patterns` dict.
    * Must include protocol because Java-side converts these strings to `URL`s (requires protocol).
    * This does not include hosts from `patterns` which do not appear in the `urls` at all (so redirects would need to be still among the same set of hosts).
  * Updating `Returns` section of doc string to mention the pattern hosts.
  * This will affect/fix functionality for repo rules such as `http_archive()` and others which use `use_netrc()` (possibly through `get_auth()`) for generating auth data.
* Updating `HttpConnectorMultiplexer.getHeaderFunction()` in `src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java`.
  * Adding host-based fallback checks.
    * This will check the given `credentials` for both the exact URI and a URI of only the host, where the host is a fallback (overridden by the exact URI).
    * The host fallback URI must include protocol, due to elsewhere converting String->URL->URI (`URL` requires protocol).
  * This will pick up the auth data originating from `use_netrc()` (and passed through various functions/transformations).
  * If the host name is not given (for some other call to download functionality), then the exact URI will be used only (just like the existing flow).
* Updating `download()` and `download_and_extract()` doc strings in `src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java`.
* Updating tests.
  * Updating `test_use_netrc` test case in `src/test/shell/bazel/starlark_repository_test.sh`.
    * Adding hosts (with protocol) into `expected` dict auth data.
  * Adding test assertions for auth header logic for `testHeaderComputationFunction()` in `src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java`.

Fixes bazelbuild#25068.
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repo Auth and URL Redirects Not Compatible
1 participant