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

remote_asset: link digest_function & checksum qual #294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sluongng
Copy link
Contributor

Ensure that the digest function in requests is consistent with the
digest function used in checksum.sri qualifier, this way server
implementations do not have to support replicating blob from one digest
function to another.

Such functionality is not the intended use case of Remote Asset APIs
and, instead, could be achieved via a different client-side API call.

Ensure that the digest function in requests is consistent with the
digest function used in `checksum.sri` qualifier, this way server
implementations do not have to support replicating blob from one digest
function to another.

Such functionality is not the intended use case of Remote Asset APIs
and, instead, could be achieved via a different client-side API call.
Copy link
Collaborator

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

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

A checksum that is provided as part of the qualifiers has absolutely nothing to do with the digest function that's used at the Bytestream transport level. It should be perfectly reasonable for a client to say "Please download this file for me through HTTP and here is its SHA-256 sum", while all REv2 traffic uses BLAKE3.

@juergbi
Copy link
Contributor

juergbi commented Apr 18, 2024

I agree with Ed. And being able to fetch assets from servers that are not tied to the CAS server at all (and thus, may make their own choice of checksum algorithm) absolutely is a valid use case for the Remote Asset API as it was envisioned.

this way server implementations do not have to support replicating blob from one digest function to another.

Remote Asset API servers are not required to support fetching arbitrary files from arbitrary (HTTP) servers. I.e., it can be acceptable to only have limited support for HTTP servers, as long as that's clearly documented to potential clients/users. However, we shouldn't restrict the protocol in such a way, in my opinion.

@sluongng
Copy link
Contributor Author

sluongng commented Apr 18, 2024

Our current draft implementation does support that behavior: fetching with a SHA256 checksum qualifier while expecting a BLAKE3 hash.

However, such logic is a bit harder to maintain and such a use case could be relatively rare in reality. It would require a client to know and use both digest functions at the same time: Requested blob with SHA256 hash, downloaded blob using BLAKE3 digest, and re-hash blob twice to validate both the BLAKE3 and SHA256 hash on the client side. Such a workflow looks like a convoluted footgun, making it harder for other client/server implementations to follow the spec.

@EdSchouten
Copy link
Collaborator

EdSchouten commented Apr 18, 2024

Our current draft implementation does support that behavior: fetching with a SHA256 checksum qualifier while expecting a BLAKE3 hash.

However, such logic is a bit harder to maintain and such a use case could be relatively rare in reality.

Is it really that rare? Bazel WORKSPACE/MODULE.bazel files often contain just SHA-256 hashes. But the user has the freedom to invoke Bazel with any remote digest function it wants.

@juergbi
Copy link
Contributor

juergbi commented Apr 18, 2024

It would require a client to know and use both digest functions at the same time

The checksum.sri might effectively be user input to a client. And if the client/user trusts the Remote Asset server, there is no need for double verification on the client side.

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