feat: address missed comments in #705

slsa-framework / slsa-verifier

Verify provenance from SLSA compliant builders

Apache License 2.0

226 stars 48 forks source link

feat: address missed comments in #705 #707

Closed laurentsimon closed 12 months ago

laurentsimon commented 1 year ago

I pressed the merge button by mistake in https://github.com/slsa-framework/slsa-verifier/pull/705. There were unresolved comments:

https://github.com/slsa-framework/slsa-verifier/pull/705#discussion_r1342227323. I think this is covered by https://github.com/slsa-framework/slsa-verifier/blob/main/verifiers/internal/gha/npm_test.go#L801 in unit tests, but it's missing from the regression tests. I think the main problem here is that to tests it, we need to be able to publish a package with the correct version and package name, but a different hash. I don't think it's actually possible. We don't explicitly verifi package name and version in the regression tests, we verify signature mismatch. Ideas?
https://github.com/slsa-framework/slsa-verifier/pull/705#discussion_r1342228689 needs to be updated.

@trishankatdatadog @ianlewis

laurentsimon commented 1 year ago

Any thoughts on the first bullet? I'm not sure we can tests it in the regression part of the test.

trishankatdatadog commented 1 year ago

Any thoughts on the first bullet? I'm not sure we can tests it in the regression part of the test.

Yes, we certainly shouldn't have npm publish a known bad attestation to this effect. We also probably don't want to maintain a test key in the code, and acting conditionally. What about tampering during a test with a known attestation but mocking the signature verification for checking the bad digest alone?

laurentsimon commented 1 year ago

the code currently does not allow mocking / ignoring / passing a custom pubKey for signature verification.

trishankatdatadog commented 1 year ago

the code currently does not allow mocking / ignoring / passing a custom pubKey for signature verification.

Right: maybe our best bet is to parameterise the public key used for verification.

laurentsimon commented 1 year ago

the code currently does not allow mocking / ignoring / passing a custom pubKey for signature verification.

Right: maybe our best bet is to parameterise the public key used for verification.

yes we can do that. It needs to be an internal API and should not be exposed to end-users.

laurentsimon commented 1 year ago

Created https://github.com/slsa-framework/slsa-verifier/issues/709 for tracking the part on mocking sig verification.