openapi: mark object as actually base64

[woodruffw]

~~This is a psuedo-fix: format: byte is not actually valid for type: object per Swagger 2.0, but other fields in the OpenAPI schema for Rekor use this convention (e.g. LogEntry.attestation) and our downstream tooling (in sigstore-apis) understands it and knows how to work around it.~~

This corrects body from type: object to type: string, and removes format: byte where misleading (i.e. from type: object in LogEntry.attestation).

Edit: downstream hotfix: https://github.com/trailofbits/sigstore-apis/pull/5

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 48.92%. Comparing base (488eb97) to head (2377d73). Report is 89 commits behind head on main.

Files	Patch %	Lines
pkg/api/entries.go	0.00%	4 Missing :warning:
cmd/rekor-cli/app/get.go	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2091       +/-   ##
===========================================
- Coverage   66.46%   48.92%   -17.54%     
===========================================
  Files          92       80       -12     
  Lines        9258     6635     -2623     
===========================================
- Hits         6153     3246     -2907     
- Misses       2359     2985      +626     
+ Partials      746      404      -342     

Flag	Coverage Δ
e2etests	?
unittests	48.92% <50.00%> (+1.23%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Hayden-IO]

Would type:string and format:base64 be a breaking change? I assume this PR isn't breaking because objects are effectively format:byte?

[woodruffw]

Would type:string and format:base64 be a breaking change? I assume this PR isn't breaking because objects are effectively format:byte?

type: object always means JSON object with no encapsulation in OpenAPI, so technically this isn't breaking since the schema definition here was incorrect in the first place (format: byte is just invalid here, but gets ignored).

I think changing it to type: string is technically a breaking change in the sense that it fixes a thing that's currently broken, but I suspect it wouldn't actually break anything (since other languages using the current spec seemingly haven't hit this condition before). So I could change this PR to the more correct type: string if you prefer 🙂

[Hayden-IO]

If I understand correctly, there's two things we should do:

• Removing format from LogEntry.attestation, because a format for the object is meaningless (and its underlying data field correctly has format:byte)
• Change body to a base64 string since it's not an object.

Is that accurate? Did you see any other non-compliances?

The other thing I don't know is why we set additionalProperties:true in attestation. Removing this would be "breaking", but a) Rekor doesn't set extra properties in the body field, and b) I don't think any client would handle this correctly anyways.

I'm good with making these correctness changes, though would you be able to double check if any sigstore clients are leveraging the openapi specs? I don't believe so.

[woodruffw]

• Removing format from LogEntry.attestation, because a format for the object is meaningless (and its underlying data field correctly has format:byte)
• Change body to a base64 string since it's not an object.

Is that accurate? Did you see any other non-compliances?

Those two sound correct to me! I've noticed one other thing, which I've fixed with 5b4eba2 -- the hashedrekord schema was using the same $id as the rekord schema despite being slightly distinct, causing mis-generation in OpenAPI clients that assume unique IDs.

I'll make the body/format changes above as well.

[bobcallaway]

I vaguely remember a bug where if additionalProperties wasn't specified, validation failed somewhere because we don't name all potential keys/properties of the JSON object within the schema. https://json-schema.org/understanding-json-schema/reference/object#additionalproperties says it is not strictly required, but not sure it hurts to be explicit?

[woodruffw]

I vaguely remember a bug where if additionalProperties wasn't specified, validation failed somewhere because we don't name all potential keys/properties of the JSON object within the schema

I think I've seen that bug, but I think these changes won't trip it -- the additionalProperties @haydentherapper is talking about is on body, which is now type: string. So the Go codegen is now rejecting it outright 🙂

[woodruffw]

Oh whoops, I misunderstood -- yeah, additionalProperties might be required in attestation but is no longer required in body, since we've corrected the type of body to type: string.

[woodruffw]

This ended up being a bit more invasive 😅 -- changing body to type: string means that LogEntryAnon.Body is no longer an interface in the generated Go, causing a cascade of internal changes.

[woodruffw]

I've broken out https://github.com/sigstore/rekor/pull/2092 for the non-type changes, since those are still needed for our downstream bindings 🙂

[woodruffw]

Opened #2097 to track the underlying issue here. I'm going to re-draft this for the time being, since it isn't immediately actionable 🙂