sigstore / sigstore-go

Go library for Sigstore signing and verification
Apache License 2.0
48 stars 26 forks source link

Fix intoto statement marshal/unmarshal #326

Open gillisandrew opened 3 weeks ago

gillisandrew commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

codysoyland commented 3 weeks ago

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 https://github.com/in-toto/attestation/issues/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 commented 3 weeks ago

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