open-telemetry / opamp-spec

OpAMP Specification
Apache License 2.0
108 stars 34 forks source link

Define that DownloadFile content hash method is SHA256 #68

Closed tigrannajaryan closed 2 years ago

tigrannajaryan commented 2 years ago

Resolves https://github.com/open-telemetry/opamp-spec/issues/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.

andykellr commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

tigrannajaryan commented 2 years ago

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

pmm-sumo commented 2 years ago

@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