Skip to content

fetcher: Reject requests without any checksum#65

Open
DolceTriade wants to merge 1 commit intobuildbarn:mainfrom
DolceTriade:noqual
Open

fetcher: Reject requests without any checksum#65
DolceTriade wants to merge 1 commit intobuildbarn:mainfrom
DolceTriade:noqual

Conversation

@DolceTriade
Copy link
Copy Markdown
Contributor

No checksum being sent which means that the
request is not guaranteed reproducible. Bazel's downloader can be used as a general general purpose GET client, and many rulesets abuse this (rules_oci notably) to fetch things like credentials and things that are not reproducible and are dangerous to cache.

However, these things generally lack any checksum (as they downloading dynamic content), so if we require all asset fetch requests to have a checksum, we can filter out these requests and allow the user to have the option to fallback to the local downloader for these requests.

Otherwise, we will cache these, which is wrong.

No checksum being sent which means that the
request is not guaranteed reproducible. Bazel's downloader can be used
as a general general purpose GET client, and many rulesets abuse this
(rules_oci notably) to fetch things like credentials and things that are
not reproducible and are dangerous to cache.

However, these things generally lack any checksum (as they downloading
dynamic content), so if we require all asset fetch requests to have a
checksum, we can filter out these requests and allow the user to have
the option to fallback to the local downloader for these requests.
@vtsao-openai
Copy link
Copy Markdown

Hey - wondering if we can get this merged?

@vtsao-openai
Copy link
Copy Markdown

Actually I found bazelbuild/bazel#23932 and I see that Bazel itself will not accept cached items w/o a checksum that are over an hour old (https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java#L287). So this change to asset service may be unnecessary.

@DolceTriade
Copy link
Copy Markdown
Contributor Author

From our experience, it is necessary, though we're not testing with bazel 8 yet. Maybe when we move that we will re-evaluate whether this patch is necessary.

@vtsao-openai
Copy link
Copy Markdown

From our experience, it is necessary, though we're not testing with bazel 8 yet. Maybe when we move that we will re-evaluate whether this patch is necessary.

FWIW that logic was also back ported to a Bazel 7 version I think.

@DolceTriade
Copy link
Copy Markdown
Contributor Author

Interesting. If you end up trying, let me know if this patch is still required. At the very least, we still see the fallback warnings as part of our build which means this patch is being utilized...

(21:15:11) WARNING: Remote Cache: INVALID_ARGUMENT: FetchBlob does not support requests without any Qualifiers specified.

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.

2 participants