Additional verification of required fields in the bundle

[haydentherapper]

Description

We currently check for an inclusion proof or promise in a validation function when constructing the bundle.

Further verification for required fields would be ideal. Using proto reflection, we could use the field annotation field_behavior to determine if the field has been annotated as required.

[vishal-chdhry]

@haydentherapper I was looking into this and I encountered an issue with content field. The protobuf bundle marks all of the field in content as REQUIRED. Validation of content field fails for this test case.

The proposal says that:

The use of REQUIRED indicates that the field must be present (and set to a non-empty value) on the request or resource.

There is something wrong in either the implementation library or the bundle, but I am not sure, what do you think?

[kommendorkapten]

Try with this branch and see if that resolves the issue: sigstore/protobuf-specs#337

[vishal-chdhry]

@kommendorkapten It fixes that test but it makes an empty VerificationMaterial a valid payload

bundle: &ProtobufBundle{
	Bundle: &protobundle.Bundle{
		MediaType:            "application/vnd.dev.sigstore.bundle+json;version=0.3",
		VerificationMaterial: &protobundle.VerificationMaterial{},
		Content:              nil,
	},
},

$ go test -run=Test_BundleValidation .
--- FAIL: Test_BundleValidation (0.00s)
    --- FAIL: Test_BundleValidation/name:Empty_verification_material (0.00s)
        bundle_test.go:182: Protobuf.Bundle() error = <nil>, wantErr bundle validation failed: missing required field: verification_material.public_key
FAIL
FAIL    github.com/sigstore/sigstore-go/pkg/bundle      0.190s
FAIL

I think this test case should fail

[haydentherapper]

I agree that an empty VerificationMaterials should cause a test failure as at least a key hint or certificate should be required. Could this be because we need to add some logic that checks for the one_of behavior extension value?

[vishal-chdhry]

Could this be because we need to add some logic that checks for the one_of behavior extension value?

Yes, it might require some changes in the library I am using.
But since we have only 1 protobuf message to validate, and the message is simple, should I just write a validation function specific to that struct myself. This will cut a dependency to that library and it will be faster as it is specific to the struct and not general purpose.

@haydentherapper @kommendorkapten wdyt?

[kommendorkapten]

@vishal-chdhry I think that makes sense to extend the custom method we have, and avoid reflection. As you said, the number of messages is low, and the so is the general complexity. I'm in favour 👍

[steiza]

At the sigstore-go client meeting, we said this was a "should" (not must) for v1.0.