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

.bazelrc generated range.sh credential helper invalidates other types of authorization at the host level #42

Closed
sfc-gh-ptabor opened this issue Dec 11, 2023 · 3 comments

Comments

@sfc-gh-ptabor
Copy link
Collaborator

sfc-gh-ptabor commented Dec 11, 2023

We discovered (thanks to @danielszot) that there is serious design flaw in range.sh approach to partial file-fetches.
It impossible to use on a single host (even different URLs) both range.sh .bazelrc solution and any form of credentials authorization. In particular from single service providing both: docker containers and *.apk packages, we were not able to serve them both.

What happens.

  1. rules_apko generates .bazelrc file that install's credential helper that for each URL that ends with #_apk_range_bytes_ adds additional header:
Range: {from}-{to}
  1. Credential helper is registered for entire host (domain or family of domaims). You cannot bind it to more specifc path like:
    my.artifacts.host.com/alpine_repository/... (https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md)

  2. Bazel tries to avoid making too many requests to credentials helper, so caches (at the bazel-server level) credentials for URL that is being fetched. For consecutive requests (multiple build for the same target), the credentials are provided directly from cache.

  3. If the credential helper is installed, the Headers/Credentials/Auth passed directly to the bazel.download call are ignored.
    Credential helper takes precedence, even if it returns empty output and wipes Authorization.

  4. The artifacts that need authorization (from the host where credential helper is used) cannot be fetched.


Solutions:

  1. rules_apko stops using credential_helper. E.g. it can download the whole artifact and checksum whole artifact without doing partial fetches. Usage of credential_helper to override not-credentials headers is not supported by bazel.
    Would like to learn more why we do partial fetches: @thesayyn

  2. rules_apko stops using bazel's downloader, but starts using custom download implementation like this one from rules_oci:
    No credential helper's complexity needed any longer on the user's side.

  3. We expand Bazel protocol of credential_helper to be able to signal keeping original creds, rather than override. [
    @Yannic @tjgq @Wyverald @bergsieker PTAL] i.e.
    a) if credential helper will return empty value (empty set of hearders) -> use the original creds [this can be breaking change]
    b) if credential helper set's in GetCredentialsResponse a bit -> the original creds are used (output gets `merged').
    c) original credentials are passed as part of GetCredentialRequest [this might be security risky as allows credentials to escape]

  4. We allow to bind credential_helper to a specific parent URL rather than whole domain:
    The current code is here.
    So we would accept e.g. following syntax:

common --credential_helper="myartifacts.company.com/repositories/alpine=%workspace%/.apko/range.sh"
or
common --credential_helper="*.myartifacts.company.com/repositories/alpine=%workspace%/.apko/range.sh"

@imjasonh @alexeagle @imjasonh FYI

@thesayyn
Copy link
Collaborator

Credential helper is a temporary solution to the problem of being able to "send arbitrary headers" with HTTP request which i already fixed upstream. credential helpers won't be necessary soon.

As for partial fetches, apks consist of three segments,

signature -> not guaranteed to be stable as signatures might change for various reasons. (eg, if the alpine private key gets leaked)
control -> guaranteed to be stable
data -> guaranteed to be stable

to avoid hard fails, in case the signature change, we only cache control and data and let signature to be fetched as needed. that's the reason why we don't specify checksum for signature fetches.

@thesayyn
Copy link
Collaborator

bazelbuild/bazel#19501

@sfc-gh-ptabor
Copy link
Collaborator Author

Thank you for explanation @thesayyn . Will wait for Bazel 7.1.0.
If that will take too long to get public, I would vote for solution number 2 as a stop-gap.

Closing.

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

No branches or pull requests

2 participants