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

Fix intoto statement marshal/unmarshal #326

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gillisandrew
Copy link

@gillisandrew gillisandrew commented Oct 31, 2024

This pull request addresses a bug in the JSON marshaling of the in-toto statement in verification results. It currently outputs "predicate_type" instead of "predicateType" required by the spec.

The issue is related to in-toto/attestation#363, this applies the fix mentioned there, defining custom marshaler/unmarshaler on the VerificationResult struct and selectively marshaling/unmarshaling the statement using google.golang.org/protobuf/encoding/protojson

@gillisandrew gillisandrew requested a review from a team as a code owner October 31, 2024 17:11
@gillisandrew gillisandrew force-pushed the fix-intoto-statement-marshal branch 3 times, most recently from 4ed4f90 to f749b9c Compare October 31, 2024 17:20
@gillisandrew
Copy link
Author

For clarity, here is a diff of the output from the example command:

go run cmd/sigstore-go/main.go \
  -artifact-digest 76176ffa33808b54602c7c35de5c6e9a4deb96066dba6533f50ac234f4f1f4c6b3527515dc17c06fbe2860030f410eee69ea20079bd3a2c6f3dcf3b329b10751 \
  -artifact-digest-algorithm sha512 \
  -expectedIssuer https://token.actions.githubusercontent.com \
  -expectedSAN https://github.com/sigstore/sigstore-js/.github/workflows/release.yml@refs/heads/main \
  examples/bundle-provenance.json
diff before.json after.json
4c4
<       "type": "https://in-toto.io/Statement/v0.1",
---
>       "_type": "https://in-toto.io/Statement/v0.1",
13c13
<       "predicate_type": "https://slsa.dev/provenance/v0.2",
---
>       "predicateType": "https://slsa.dev/provenance/v0.2",

@phillmv
Copy link
Member

phillmv commented Oct 31, 2024

Hrm! Thanks for this. Can we get this fixed in intoto/attestation/go, I wonder? Or is this one of those "protobuf code gen knows no gods nor masters" kind of deals?

I'm tickled that we never noticed this, but also would prefer to avoid having custom serialization funcs if we can avoid it. As a stop gap this might be fine 🤔.

@codysoyland
Copy link
Member

codysoyland commented Oct 31, 2024

Oof, protojson is a pain... I don't think it can be easily fixed upstream due to how the protobuf types are compiled. One thing we can do that could simplify this though: What if we wrap VerificationResult.Statement in a custom type that has Unmarshall/Marshall methods, e.g.:

type Statement struct{
	in_toto.Statement
}

func (s *Statement) MarshalJSON() ([]byte, error) {
	return protojson.Marshal(s.Statement)
}

func (s *Statement) UnmarshalJSON(data []byte) error {
...
}

type VerificationResult struct {
	MediaType          string                        `json:"mediaType"`
	Statement          *Statement            `json:"statement,omitempty"`
...

That would simplify the code here a bit.

Could you also add tests?
Thanks!

@gillisandrew
Copy link
Author

@codysoyland I started off by wrapping it but that makes it a breaking change and involves updating types all over.

I'll add some tests tomorrow or over the weekend.

@kommendorkapten
Copy link
Member

What @codysoyland proppses makes a lot of sense, I like that idea.

Signed-off-by: Andrew Gillis <[email protected]>
@codysoyland
Copy link
Member

Thank you @gillisandrew! I like this better, but before merging, I'd like to see if the in_toto maintainers would be open to adding these marshal/unmarshal methods in the upstream. Otherwise this will continue to bite other folks...

It looks like in-toto/attestation#363 was closed erroneously... perhaps one of us can add those methods in a PR to this file? I think that would be ideal and I bet they would accept it.

@gillisandrew
Copy link
Author

@codysoyland Sounds good. I'll open one there and see what they say.

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.

4 participants