Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Rekor OID info. #1390

Merged
merged 4 commits into from
Apr 28, 2023
Merged

Add Rekor OID info. #1390

merged 4 commits into from
Apr 28, 2023

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Mar 16, 2023

@haydentherapper Finally circling back to this 🙏

Summary

This defines OIDs for most log entry fields (though not all). This is primarily intended to be used by clients that want to represent Rekor data in different formats and/or cannot use bundles.

For example, gitsign cannot use the verification bundle since it uses PKCS7 signatures to keep compatibility with existing Git clients / signature formats. Since PKCS7 already defines a structure for payload hashes, it implements offline verification by deconstructing bundles into distinct OIDs then reconstructs them for verification. See sigstore/gitsign#220 for more details.

Release Note

Adds OID definitions for LogEntry fields.

Documentation

@wlynch wlynch requested a review from a team as a code owner March 16, 2023 20:54
This defines OIDs for most log entry fields (though not all). This is
primarily intended to be used by clients that want to represent Rekor
data in different formats and/or cannot use bundles.

For example, gitsign cannot use the verification bundle since it uses
PKCS7 signatures to keep compatibility with existing Git clients /
signature formats. Since PKCS7 already defines a structure for payload
hashes, it implements offline verification by deconstructing bundles
into distinct OIDs then reconstructs them for verification.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
oid-info.md Outdated

## Directory

| OID | Name | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

drive-by: would it make sense to use a single OID for a "sigstore bundle" that is using the spec at protobuf-specs?

(That would avoid a whole mess of comments that we've sorted out there: e.g. what encoding is the Log ID, is Verification required, what encoding is the root hash, is the log index the global, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

My only concern is there would be duplicated data that may be in other locations (body/attestation) - for gitsign body is the main concern, but as long as it's a hashed checksum I guess it wouldn't be that much more?

Let me try playing around with this in my draft PR and see how much of a pain it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, do you mean to turn this into the sigstore bundle OID instead of Rekor?

I think what's here should be transferable to the protobuf-specs spec? Though I think some of the naming isn't aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

but as long as it's a hashed checksum I guess it wouldn't be that much more?

Yes, if it's not an attestation it would just be the checksum of the body.

Yes! I was suggesting turning it into a sigstore bundle OID

Copy link
Member Author

@wlynch wlynch Mar 16, 2023

Choose a reason for hiding this comment

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

Okay, tried it out using https://pkg.go.dev/github.com/sigstore/cosign/v2/pkg/cosign/bundle#RekorBundle as a proxy (let me know if there are major differences between this and protobuf-specs). I don't think we want to do this.

There's a few fields that end up being duplicated across the signature with the bundle:

  • content hash
  • signature
  • cert chain
  • rfc3161 signed timestamp
  • canonicalized_body (which if it acts like the cosign RekorBundle, includes the base64 encoded HashedRekord entry which re-encodes the hash, signature, and cert chain again)

This ends up ~doubling (and if we used the full protobuf-spec bundle perhaps tripling) the size of the signature to store data that we already have elsewhere in the PKCS7 signature.

What would be better is if there was a modified version of TransparencyLogEntry, but let the client re-construct the canonicalized_body from other data it already has.

Maybe we could do this by nil-ing out the field and encoding that? Otherwise we can stick the original plan of just defining OIDs for the individual values. Open to ideas here - let me know what you think!

(aside - long term I'd love to offer a --- BEGIN SIGSTORE BUNDLE--- signature type and encode the proto bundle as is, but I don't think we have that luxury based on https://git-scm.com/docs/signature-format)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if @haydentherapper is OK with adding Rekor specific OID, that's fine - if something changes with the semantics then I worry this won't be maintained, but if GitSign is one of the only consumers that's OK

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a way to do this for the protos, definitely on board.

Another option is to move these OIDs back to Gitsign and not have this in Rekor, if Gitsign really is the only client.

Copy link
Contributor

Choose a reason for hiding this comment

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

The more I think about it, maybe that is the best way to proceed. GitSign uses .3, defines its own child OIDs, we concurrently define OIDs for the bundle, and then at some point, GitSign adopts the bundle and these new OIDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the likelihood of detangling canonicalized_body from the rest of the TransparencyLogEntry in protobuf-specs? I think that would be my preference since that would probably be easiest to maintain long term. If you need me to open an issue elsewhere for discussion lmk!

For the OIDs, the initial idea was to put this in Rekor since we're describing Rekor types. Functionally it's fine if this lives in Gitsign, but I'd worry these may diverge over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened sigstore/protobuf-specs#72 for more discussion

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #1390 (565ed27) into main (f9f0a07) will increase coverage by 21.25%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #1390       +/-   ##
===========================================
+ Coverage   43.06%   64.32%   +21.25%     
===========================================
  Files          74       82        +8     
  Lines        7458     7874      +416     
===========================================
+ Hits         3212     5065     +1853     
+ Misses       3910     2161     -1749     
- Partials      336      648      +312     
Flag Coverage Δ
e2etests 47.71% <ø> (?)
unittests 43.19% <ø> (+0.12%) ⬆️

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

see 55 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

oid-info.md Outdated Show resolved Hide resolved
oid-info.md Outdated Show resolved Hide resolved
oid-info.md Outdated Show resolved Hide resolved
oid-info.md Outdated Show resolved Hide resolved
@wlynch wlynch requested review from haydentherapper and asraa and removed request for haydentherapper and asraa March 16, 2023 23:25
Signed-off-by: Billy Lynch <billy@chainguard.dev>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

LGTM for the OIDs!

oid-info.md Outdated Show resolved Hide resolved
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Un-lgtming just so we don't accidentally merge. Having a single OID and using the bundle format sounds good with me

@wlynch wlynch force-pushed the oid branch 2 times, most recently from 0fb6d0f to f0fd815 Compare April 7, 2023 17:23
@wlynch
Copy link
Member Author

wlynch commented Apr 7, 2023

Updated for the single OID based on sigstore/protobuf-specs#72

I wasn't sure what the best tag type would be here (i.e. bit string vs octet string vs utf8string) - let me know if there's a better fit!

Signed-off-by: Billy Lynch <billy@chainguard.dev>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Thanks for making this update!

oid-info.md Outdated Show resolved Hide resolved
Signed-off-by: Billy Lynch <billy@chainguard.dev>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

@bobcallaway This is ready to merge

@bobcallaway bobcallaway merged commit 39bd69b into sigstore:main Apr 28, 2023
@github-actions github-actions bot added this to the v1.1.0 milestone Apr 28, 2023
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.

5 participants