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

Define that DownloadFile content hash method is SHA256 #68

Merged

Conversation

tigrannajaryan
Copy link
Member

Resolves #67

Without specifying the hashing method it was useless for the purposes it claims to exist for

The hash of the file content. Can be used by the Agent to verify that the file was downloaded correctly.

You can't verify that the hash matches the content if you don't know the hashing method.

Now it is precisely defined to be SHA256 of the content.

@tigrannajaryan tigrannajaryan requested a review from a team March 2, 2022 17:32
@andykellr
Copy link
Contributor

This seems reasonable, but I had assumed that the exact hashing algorithm was shared between the client and server, similar to your comment in #69 that, "The exact signing and verification method is Agent specific."

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Mar 8, 2022

As discussed during the meeting, I think it might be more flexible to have name of the hashing method provided separately rather than fixing on SHA-256. So e.g. string hashing_algorithm = 3;

@tigrannajaryan
Copy link
Member Author

I think the idea that we may need other hash methods is valid. I would not want to add support for other methods right now, but we need to make it possible in the future. To enable this I will rename content_sha256 to content_hash, but will keep the notice that it must be SHA256. In the future we can add hashing_algorithm and make "sha256" the default value for it, so it will be a backward compatible addition. I think this is a good compromise to avoid adding too much to the spec right now.

Resolves open-telemetry#67

Without specifying the hashing method it was useless for the purposes it claims to exist for
>The hash of the file content. Can be used by the Agent to verify that the file was downloaded correctly.

You can't verify that the hash matches the content if you don't know the hashing method.

Now it is precisely defined to be SHA256 of the content.
@tigrannajaryan
Copy link
Member Author

@pmm-sumo PTAL, I made changes as described above.

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Mar 22, 2022

@pmm-sumo PTAL, I made changes as described above.

Sorry for the delay, I must have missed the update. I think it's a fair approach

@tigrannajaryan tigrannajaryan merged commit 410dc86 into open-telemetry:main Mar 22, 2022
@tigrannajaryan tigrannajaryan deleted the hash-to-be-sha256 branch March 22, 2022 17:57
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.

content_hash of Downloadable file needs to define hashing method
3 participants