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

Consider omitting checksum of the JSON manifest file #81

Open
radeksimko opened this issue Feb 12, 2024 · 2 comments
Open

Consider omitting checksum of the JSON manifest file #81

radeksimko opened this issue Feb 12, 2024 · 2 comments

Comments

@radeksimko
Copy link
Member

There is more context in hashicorp/terraform#33599 but I'd specifically quote this part hashicorp/terraform#33599 (comment)

I think the problem here is that the release process for the official providers seems to be incorrectly including the manifest file in the set of files it considers when calculating the SHA256SUMS file. The fix in that case would be to change the provider release process to fix this problem, although that is not something we'd change in this codebase because the release processes for the official providers belong to their own codebases. Since this slight misbehavior seems common across them all, I assume they all have some shared dependency that we could change to fix this for all of them at once, at least moving forward with new releases.


I do not know enough about the provider ingestion process to the Registry and I expect there is some rationale behind including it, at least from reading the goreleaser config in e.g. https://github.com/hashicorp/terraform-provider-aws/blob/13e14ac684817c4e9254c125055e776f2fadf3cc/.goreleaser.yml#L43-L45

I can understand that there may be need for a way of verifying the integrity of the manifest file during the ingestion.

However, I do agree with the original reporter and Martin as well in that the manifest file does not seem relevant in the context of the lock file.

I am honestly not sure what the best solution here is and I don't have enough visibility into the ingestion process myself but I hope this provides enough context to start the discussion that may lead to something. 😄

@bflad
Copy link
Member

bflad commented Feb 15, 2024

Hey @radeksimko 👋 Thank you for raising this!

My recollection of the introduction of the manifest file handling in the Terraform Registry is that it was treated the same as the other existing release assets during ingress, meaning that if it was included in the assets, it must be part of the checksums. I do think, however, that the newer manifest file was unintentionally not excluded from the Registry Protocol responses.

So therefore I'm guessing we have two questions here to think about:

  • Whether the Terraform Registry protocol responses should include the manifest file checksum
  • Whether the Terraform Registry ingress process should require the manifest file checksum

While the manifest file contents are fairly innocuous today, the file and its ingress process was created this way so it could be possibly extended in the future to support updating other Terraform Registry bits about the provider/release. The entire ecosystem is already required to checksum it properly for ingress today so unless there is a very good reason we want to relax that requirement, I think it should stay to prevent any sort of future-proofing issue for the future.

The protocol responses including the manifest file checksum does seem extraneous though. Terraform core should not care about provider version release assets beyond plugin archives that fit under its trust model. Those changes would need to occur in the Terraform Registry itself.

@rclark
Copy link

rclark commented Feb 15, 2024

I wanted to summarize some discussion we had (and keep my eye on this ticket too)...

Unfortunately this problem isn't one we can solve by only changing some of the Registry API response details. Instead it is likely that a solution would require changes in the process of creating a provider release, changes at publish time & at read time in the Registry API, and also changes to terraform CLI itself.

The SHA256SUMS file that includes the checksum for the manifest.json is signed by the author's private key. That gets used by the Registry to verify the contents at publish-time. But also when terraform requests to download the provider from the Registry:

  • The Registry provides a link to the zipped, binary file, the SHA256SUMs file, and the signature file. These are all links to where those files are hosted in a GitHub release. The registry API also provides the author's public key.
  • Terraform downloads all those files
  • Terraform uses the public key to verify the signature of the SHA256SUMS file, guaranteeing that the contained checksums have not been tampered with.
  • Terraform checks that the zipped, binary file matches the checksum recorded in SHA256SUMS, guaranteeing that the file has not been tampered with.

Were the Registry API to provide a link to some other version of the author's SHA256SUMS file, with manifest.json removed, those signatures would no longer match. So we'd have to change more than just that.

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

3 participants