sigstore / rekor

Software Supply Chain Transparency Log
https://sigstore.dev
Apache License 2.0
859 stars 162 forks source link

openapi: mark object as actually base64 #2091

Open woodruffw opened 3 months ago

woodruffw commented 3 months ago

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] commented 3 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/sigstore/rekor/pull/2091/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Coverage Δ | | |---|---|---| | [e2etests](https://app.codecov.io/gh/sigstore/rekor/pull/2091/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | `?` | | | [unittests](https://app.codecov.io/gh/sigstore/rekor/pull/2091/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | `48.92% <50.00%> (+1.23%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#carryforward-flags-in-the-pull-request-comment) to find out more.

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

haydentherapper commented 3 months ago

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

woodruffw commented 3 months ago

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 🙂

haydentherapper commented 3 months ago

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

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 commented 3 months ago
  • 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 commented 3 months ago

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

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

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

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

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

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 🙂