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

Migrate to a digest field instead of a of sha1 field in Kilnfile.lock #297

Open
crhntr opened this issue Nov 11, 2021 · 2 comments
Open

Comments

@crhntr
Copy link
Member

crhntr commented Nov 11, 2021

We need to download releases whenever we update in order to calculate their sha1 sum. Most release sources support some other more secure algorithm. Usually shas256. We should use that and skip the downloads when we don't need it. Using sha256 might also be useful in creating a standards compliant software bill of materials.

Acceptance Criteria

The algorithm is based on the release source

Given a component release source natively supports a hashing algorithm
When a the component lock is created by GetMatchedRelease
Then a field Digest is set with that a value matching the component spec

Digest field spec

The following spec uses a modified BNF. The spec is a simplified version of (based on OCI image-spec).

 digest                ::= algorithm ":" encoded | encoded
 algorithm             ::= "sha1" | "sha256"
 encoded               ::= [a-zA-Z0-9=_-]+

The code can rely on a set digest field

Given a Kilnfile.lock has a field with key sha1 matching encoded
When the Kilnfile.lock is loaded
Then the cargo.ComponentLock has a field named Digest in Go and yaml key digest
And the value is a digest with algorithm "sha1"

Backwards compatibility of Kiln with older Kilnfile.lock files

Code should not access the Digest field on a cargo.ComponentLock but should use a method returning both the digest and the encoded hash.

The SHA1 field on the Lock should be marked as deprecated and renamed (without IDE support) to something like LegacySHA1. All code accessing the SHA1 field should now use the new method.

Kilnfile.lock Migration Path

  • Kiln will continue to set the sha1 field for all downloaded releases (for at least a few releases)
  • Kiln will use a more secure hashing algorithm, when set, to verify downloaded releases
    • it may warn of insecure hash algorithm when sha1 is the only stored sum
  • kiln update --hash will download all releases calculate their sums and set the digest values with the sha256 algorithm
  • kiln update will calculate and set both the sha1 and digest fields for updated releases

References

We could support the full spec with minimal effort. This is a simplification so we can make an iterative change.

  • diff between the digest spec here and our spec
- digest                ::= algorithm ":" encoded
+ digest                ::= algorithm ":" encoded | encoded
+ algorithm             ::= "sha1" | "sha256"
- algorithm             ::= algorithm-component (algorithm-separator algorithm-component)*
- algorithm-component   ::= [a-z0-9]+
- algorithm-separator   ::= [+._-]
  encoded               ::= [a-zA-Z0-9=_-]+
@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@crhntr crhntr changed the title Use digest instead of sha1 in Kilnfile Use digest instead of sha1 in Kilnfile.lock Nov 11, 2021
@crhntr crhntr changed the title Use digest instead of sha1 in Kilnfile.lock Migrate to a digest instead of just of sha1 in Kilnfile.lock Nov 11, 2021
@crhntr crhntr changed the title Migrate to a digest instead of just of sha1 in Kilnfile.lock Migrate to a digest field instead of a of sha1 field in Kilnfile.lock Nov 11, 2021
@dtimm
Copy link
Contributor

dtimm commented Oct 20, 2022

This looks like a good change to me. The current requirement of Kiln downloading releases in many circumstances causes a lot of headaches in automation, and any reductions we can make in that are good. Deprecating and removing the old field in Kiln 1.0 would be a good, natural place to make the switch, as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants