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

Don't hardcode Cosign attest type #1533

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Conversation

Nirusu
Copy link
Contributor

@Nirusu Nirusu commented Feb 2, 2023

Fixes #1532.

Since this seems to be intentional, I also raised the issue to describe the issue and maybe keep the discussion there?

Depending on how the discussion goes, maybe merge or close this PR. I don't know - the fix is trivial though :)

Signed-off-by: Nils Hanke <nils.hanke@outlook.de>
@kzantow
Copy link
Contributor

kzantow commented Feb 2, 2023

@spiffcs probably has the most context here, but I believe the issue is we can no longer verify attestations if they are not using the custom type

@Nirusu
Copy link
Contributor Author

Nirusu commented Feb 2, 2023

Yes, see the linked issue with a longer description :)

From what I gathered is that the original issue is
a) a missing type in the verification command
and/or
b) Cosign itself - not Syft.

Cosign defaults to custom if no type is specified anyway, so I don't know why this has been hard coded?

@spiffcs
Copy link
Contributor

spiffcs commented Feb 2, 2023

👋 Hi @Nirusu see this issue we filed with cosign upstream:
sigstore/cosign#2494

As of this feature the only way I could get verify-attestation to behave correctly was by using a custom type - this is why it's been hardcoded this way.

@spiffcs
Copy link
Contributor

spiffcs commented Feb 2, 2023

If you shelled out and ran the command without the custom type previously it would not pass validation. If this is still not the case we can take a look and update the command string to use a value input by the user

@spiffcs
Copy link
Contributor

spiffcs commented Feb 2, 2023

I read the issue you linked - I do agree that we probably should just expose the type at this point and let the downstream commands work as expected giving the user the most freedom

@spiffcs
Copy link
Contributor

spiffcs commented Feb 2, 2023

I'll add that change - thanks for the report!

@Nirusu
Copy link
Contributor Author

Nirusu commented Feb 2, 2023

I saw your issue (see my chain of thoughts in #1532), but that's what I don't get.

You ran:

$COSIGN_EXPERIMENTAL=1 cosign attest --predicate test_spdx_scratch.json --type spdxjson caphill4/scratch:latest 
COSIGN_EXPERIMENTAL=1 cosign verify-attestation caphill4/scratch:latest                                                                                                                                    

The attest is specified with a predicate, the verify-attestation without.

That doesn't make sense to me.
If you specify the predicate in attest, why not add it to verify-attestation, too? It does not auto-detect the predicate, that's actually broken.
If you want custom as a type anyway, why specify it in the first place? Cosign defaults to custom if nothing is specified (hence this PR).

So I don't really see what you were going here for - not specifying it should already give you the same custom behavior.
Specifying it as a predicate in both commands - attest and verify-attestation - should also work (not 100% sure now, though).

But only in one command? Yes, auto-detection is broken, but I would still pass that as user error if you define another type but ultimately don't care about the type.

Hardcoding doesn't make sense imo since Syft is not bound to a specific version of Cosign anymore if it's executed externally.

@spiffcs spiffcs merged commit a1b82c9 into anchore:main Feb 2, 2023
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
Signed-off-by: Nils Hanke <nils.hanke@outlook.de>
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.

Syft hardcodes custom attestation type
3 participants