Skip to content

Include KindVersion in TLE response#286

Merged
Hayden-IO merged 1 commit intosigstore:mainfrom
Hayden-IO:kind-version
May 6, 2025
Merged

Include KindVersion in TLE response#286
Hayden-IO merged 1 commit intosigstore:mainfrom
Hayden-IO:kind-version

Conversation

@Hayden-IO
Copy link
Contributor

@Hayden-IO Hayden-IO commented May 5, 2025

This adds a missing required field for the kind and version of the entry, so that clients will know how to parse the canonicalized body.

This is a much simpler approach to the type factory in Rekor. At this point, given there are only two types, I don't think we should reimplement the factory and simply use case statements.

Ref: #285

Summary

Release Note

Documentation

This adds a missing required field for the kind and version of the
entry, so that clients will know how to parse the canonicalized body.

This is a much simpler approach to the type factory in Rekor. At this
point, given there are only two types, I don't think we should
reimplement the factory and simply use case statements.

Signed-off-by: Hayden B <8418760+haydentherapper@users.noreply.github.com>
@Hayden-IO Hayden-IO requested review from a team as code owners May 5, 2025 22:02
@codecov
Copy link

codecov bot commented May 5, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 39.33%. Comparing base (7289406) to head (c87e491).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/server/service.go 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   39.11%   39.33%   +0.22%     
==========================================
  Files          39       40       +1     
  Lines        2751     2766      +15     
==========================================
+ Hits         1076     1088      +12     
- Misses       1571     1573       +2     
- Partials      104      105       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Hayden-IO Hayden-IO merged commit 3649260 into sigstore:main May 6, 2025
12 checks passed
@cmurphy
Copy link
Contributor

cmurphy commented May 6, 2025

Could you also update switch statements like these

switch e := entry.(type) {
to use this?

if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "invalid type, must be either hashedrekord or dsse")
}
tle.KindVersion = kv
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that KindVersion is also missing from the CanonicalizedBody.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That part is intentional. The type should already be known by then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appu, how is the type derived for monitors reading each entry of the log? With a full bundle, a client will know how to parse it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized the problem here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loosebazooka But isn't the canonicalized by supposed to have all of the values of TLE? That seems to be how it is in v1's responses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I gotta go look at this again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out Ramon. We do need entry type information for clients that do not have a Sigstore bundle. I've proposed a fix in #287, which wraps the entry in a one-of so that we retain kind/version information.

Another option Appu and I discussed was trying to replicate the Rekor v1 body with apiversion, kind, and spec. While this might let clients get further along in parsing the body, clients will eventually fail because they won't support the new v2 types.

Appu will be reopening the discussion about a TLE v2 message. Clients will need to update their verification path regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also to clarify, KindVersion in the bundle is still needed if CanonicalizedBody, which is optional, is omitted and a client reconstructs the rekor entry from the artifact, signature and verifier. We don't need KindVersion explicitly stated if the message types are retained, which is what 287 proposes.

@Hayden-IO Hayden-IO deleted the kind-version branch May 6, 2025 16:25
@Hayden-IO
Copy link
Contributor Author

Following up on comments in another PR with adding log ID.

@loosebazooka
Copy link
Member

oops, stamped this a little too hastily

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