Add COSE support to Rekor #867

kommendorkapten · 2022-06-09T08:37:04Z

Summary

Basic support for COSE envelopes.
Added is functionality to upload COSE records, optionally signed with AAD. The AAD is not stored in Rekor.
The COSE envelope is stored as an attestation if attestation storage is enabled, and can so be retrieved via Rekor. The returned envelope is returned in the attestation field, not within the body field.

What's stored is the hash of the payload and the entire envelope. If the content type of the COSE envelope is an in-toto statement, the payload is parsed and the subject hashes are extracted and indexed for searching.

Ticket Link

This pull requests completes #731.

Release Note

Added new request/response type: COSE envelopes. When uploading a COSE envelope, only the hash of the payload and the hash of the envelope is stored, unless attestation storage is enabled, then the entire envelope is stored as an attestation.
A new command line parameter `--aad` is added. This parameter provides extra Additional Authenticated Data to be used during COSE signature verification (using AAD with COSE is optional).

dlorenc · 2022-06-09T12:55:30Z

Looks like there's a merge conflict, and github isn't running for some reason. Maybe fixing the merge conflict will poke actions?

kommendorkapten · 2022-06-09T13:12:01Z

Yes, will do. Some merge conflicts I have to resolve first 👍

dlorenc

One main question on algorithm checking.

dlorenc · 2022-06-10T13:46:15Z

pkg/types/cose/v0.0.1/entry.go

+	switch t := cryptoPub.(type) {
+	case *rsa.PublicKey:
+		alg = gocose.AlgorithmPS256
+	case *ecdsa.PublicKey:


Do we need to be a bit more careful here to avoid curve mismatches? Is there a possibility someone actually used PS384 or PS512?

Yeah, good catch. I can play around a bit more early next week to see if we can tighten up this.

dlorenc · 2022-06-10T13:47:50Z

pkg/types/cose/v0.0.1/entry.go

+	}
+
+	// If payload is an in-toto statement, let's grab the subjects.
+	if rawContentType, ok := v.sign1Msg.Headers.Protected[gocose.HeaderLabelContentType]; ok {


This part is interesting, I'm not sure I've seen in-toto statements embedded into COSE envelopes before. I think it makes sense overall though.

codecov-commenter · 2022-06-13T13:46:06Z

Codecov Report

Merging #867 (142be5b) into main (45da1af) will increase coverage by 0.69%.
The diff coverage is 59.92%.

@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
+ Coverage   46.24%   46.93%   +0.69%     
==========================================
  Files          60       62       +2     
  Lines        5138     5414     +276     
==========================================
+ Hits         2376     2541     +165     
- Misses       2487     2590     +103     
- Partials      275      283       +8

Impacted Files	Coverage Δ
cmd/rekor-cli/app/root.go	`20.68% <ø> (ø)`
pkg/types/entries.go	`6.25% <ø> (ø)`
pkg/types/test_util.go	`0.00% <0.00%> (ø)`
pkg/types/cose/v0.0.1/entry.go	`54.22% <54.22%> (ø)`
cmd/rekor-cli/app/pflag_groups.go	`87.20% <100.00%> (+0.87%)`	⬆️
cmd/rekor-cli/app/pflags.go	`85.33% <100.00%> (+0.82%)`	⬆️
pkg/types/cose/cose.go	`100.00% <100.00%> (ø)`
pkg/types/alpine/v0.0.1/entry.go	`54.54% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45da1af...142be5b. Read the comment docs.

dlorenc · 2022-06-13T14:10:46Z

Overall LGTM. I'll wait for @bobcallaway to take a look before merging.

dlorenc · 2022-06-13T14:11:21Z

Overall LGTM. I'll wait for @bobcallaway to take a look before merging.

bobcallaway

looking good overall, just a few questions/nits

cmd/rekor-cli/app/pflag_groups.go

pkg/types/cose/cose.go

go.mod

pkg/types/cose/cose.go

bobcallaway · 2022-06-13T15:02:09Z

pkg/types/cose/v0.0.1/entry.go

+
+	alg, pk, err := getPublicKey(v.keyObj)
+	if err != nil {
+		return nil


did you mean to return err here?

Of course, will go over some test coverage here to make sure we got it covered.

bobcallaway · 2022-06-13T15:02:58Z

pkg/types/cose/v0.0.1/entry.go

+	v.sign1Msg = gocose.NewSign1Message()
+	if err := v.sign1Msg.UnmarshalCBOR(v.CoseObj.Message); err != nil {
+		return err
+	}
+


should we ensure unmarshaling is successful before assigning the output to v.sign1Msg?

You mean like this:

coseMsg := gocose.NewSign1Message() if err := coseMsg.UnmarshalCBOR(v.CoseObj.Message); err != nil { return err } v.sign1Msg = coseMsg

I think that makes sense, we can even wait until the signature is verified until we assign it to v.sign1Msg.

yes, ideally I wouldn't want to partially change the pointer receiver unless all validation is successful.

pkg/types/entries.go

bobcallaway

LGTM, would like one more test case to ensure we have coverage for non-base64 encoded input on the aad cmd line argument

cmd/rekor-cli/app/pflags_test.go

kommendorkapten · 2022-06-14T12:03:20Z

The sharding e2e test failed (curl: (22) The requested URL returned error: 422 Unprocessable Entity). Is there any documentation on running this locally?

kommendorkapten · 2022-06-14T12:04:27Z

Oh, found a script, will try that.

kommendorkapten · 2022-06-14T12:23:27Z

I have run the sharding-e2e test now locally three times and am not able to reproduce, the execution succeeds all the time for me. Anyone having an idea?

kommendorkapten · 2022-06-14T12:49:23Z

Hm, this seems to be the issue:

++ printf %x 7051007558037933689
++ Awk '{printf "%016s", $0}'
+ HEX_INITIAL_TREE_ID=61da360425720e79
++ printf %x 151938258151519870
++ awk '{printf "%016s", $0}'
+ HEX_INITIAL_SHARD_ID=' 21bcb0a70e9b67e'
+ ENTRY_ID_1=61da360425720e79d2f305428d7c222d7b77f56453dd4b6e6851752ecacc78e5992779c8f9b61dd9
+ ENTRY_ID_2=' 21bcb0a70e9b67e59a575f157274702c38de3ab1e1784226f391fb79500ebf9f02b4439fb77574c'
++ curl -f http://localhost:3000/api/v1/log/entries/retrieve -H 'Content-Type: application/json' -H 'Accept: application/json' -d '{ "entryUUIDs": ["61da360425720e79d2f305428d7c222d7b77f56453dd4b6e6851752ecacc78e5992779c8f9b61dd9", " 21bcb0a70e9b67e59a575f157274702c38de3ab1e1784226f391fb79500ebf9f02b4439fb77574c"]}'
++ jq '. | length'

The shard id gets a white space ( ) as the first character, which is then input to the UUIDs: " 21bcb0a70e9b67e59a575f157274702c38de3ab1e1784226f391fb79500ebf9f02b4439fb77574c".

Running the printf/awk on my machine (macOS) I get a different output

 printf %x 151938258151519870 | awk '{printf "%016s", $0}'
021bcb0a70e9b67e

Running same command on Ubuntu:

# printf %x 151938258151519870 | awk '{printf "%016s", $0}'
 21bcb0a70e9b67e

I'll prepare a patch for the awk script.

bobcallaway · 2022-06-14T12:58:55Z

I'll push up a separate PR to fix that, and also add log collection to sharding-e2e-test.sh since if there is a failure right now we don't capture the docker-compose logs.

…

On Tue, Jun 14, 2022 at 8:49 AM Fredrik Skogman ***@***.***> wrote: Hm, this seems to be the issue: ++ printf %x 7051007558037933689 ++ Awk '{printf "%016s", $0}' + HEX_INITIAL_TREE_ID=61da360425720e79 ++ printf %x 151938258151519870 ++ awk '{printf "%016s", $0}' + HEX_INITIAL_SHARD_ID=' 21bcb0a70e9b67e' + ENTRY_ID_1=61da360425720e79d2f305428d7c222d7b77f56453dd4b6e6851752ecacc78e5992779c8f9b61dd9 + ENTRY_ID_2=' 21bcb0a70e9b67e59a575f157274702c38de3ab1e1784226f391fb79500ebf9f02b4439fb77574c' ++ curl -f http://localhost:3000/api/v1/log/entries/retrieve -H 'Content-Type: application/json' -H 'Accept: application/json' -d '{ "entryUUIDs": ["61da360425720e79d2f305428d7c222d7b77f56453dd4b6e6851752ecacc78e5992779c8f9b61dd9", " 21bcb0a70e9b67e59a575f157274702c38de3ab1e1784226f391fb79500ebf9f02b4439fb77574c"]}' ++ jq '. | length' The shard id gets a white space ( ) as the first character, which is then input to the UUIDs: " 21bcb0a70e9b67e59a575f157274702c38de3ab1e1784226f391fb79500ebf9f02b4439fb77574c" . Running the printf/awk on my (mac I get a different error) printf %x 151938258151519870 | awk '{printf "%016s", $0}' 021bcb0a70e9b67e Running same command on Ubuntu: # printf %x 151938258151519870 | awk '{printf "%016s", $0}' 21bcb0a70e9b67e I'll prepare a patch for the awk script. — Reply to this email directly, view it on GitHub <#867 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVWTJKRWNAPOKIW33RKHVDVPB5V5ANCNFSM5YJHEAIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kommendorkapten · 2022-06-14T13:11:33Z

From the GNU awk manual (https://www.gnu.org/software/gawk/manual/html_node/Format-Modifiers.html): (emphasis mine)

A leading ‘0’ (zero) acts as a flag indicating that output should be padded with zeros instead of spaces. This applies only to the numeric output formats. This flag only has an effect when the field width is wider than the value to print.

kommendorkapten · 2022-06-14T13:16:44Z

Shamelessly copied this from stackoverflow (some minor modification to work with printf instead of echo):

printf abc123 | awk '{ for(c = 0; c < 16 ; c++) s = s"0"; s = s$1; print substr(s, 1 + length(s) - 16);}'
0000000000abc123

Tested on both Ubuntu and macOS

bobcallaway · 2022-06-14T13:29:21Z

Shamelessly copied this from stackoverflow (some minor modification to work with printf instead of echo):
printf abc123 | awk '{ for(c = 0; c < 16 ; c++) s = s"0"; s = s$1; print substr(s, 1 + length(s) - 16);}'
0000000000abc123
Tested on both Ubuntu and macOS

@priyawadhwa @lkatalin is the intent for the value to be left-padded with zeros?

lkatalin · 2022-06-14T14:42:28Z

@priyawadhwa @lkatalin is the intent for the value to be left-padded with zeros?

@bobcallaway
Still going through trying to understand this error, but the padding is something I had included so that we can get a consistent length, probably so that we can easily parse the TreeID from the UUID.

This commit adds COSE Sign1 support to rekor via a new data type. COSE is defined in RFC8152, and provides a signing message envelope. This is supported in rekor using the veraison/go-cose library. The new API type requires the signed content, the signature envelope, and the public key. The public key is in the standard rekor PKI format, at the moment only ECDSA P256 with SHA256 is supported. The signed message only supports in-line bodies (no URL fetching), and there is no support for pre-hashed entries. Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

This adds some more functionality related to COSE enveleopes. Features added are: - Support for specifying Additional Authentincated Data (AAD) - The entire CBOR envelope is stored as an attestation - If the payload type is an in-toto statement, subject is indexed What's not optimal is that the COSE envelope is using the regular `Attestion()` functionality, which means that rekor cli tries to print it during `rekor-cli get` and the response record from Rekor looks a bit awkward. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

The biggest change is adapting the new interface where attestation func is split to two, one to get the key and a nother to get key/val. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…stdlib errors package. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Also increas the general test coverage. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…ing. This gives the caller the possibility to decide how to decode the data. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…dation is done. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten requested review from bobcallaway and a team as code owners June 9, 2022 08:37

kommendorkapten force-pushed the add_cose branch from 21c2202 to fc37fba Compare June 9, 2022 12:02

kommendorkapten changed the title ~~Add cose~~ Add COSE support to Rekor Jun 9, 2022

kommendorkapten force-pushed the add_cose branch 2 times, most recently from 30fcf43 to c3180ed Compare June 10, 2022 07:59

dlorenc reviewed Jun 10, 2022

View reviewed changes

bobcallaway reviewed Jun 13, 2022

View reviewed changes

bobcallaway reviewed Jun 14, 2022

View reviewed changes

cmd/rekor-cli/app/pflags_test.go Show resolved Hide resolved

bobcallaway previously approved these changes Jun 14, 2022

View reviewed changes

dlorenc and others added 6 commits June 21, 2022 07:40

Updated the documentation for COSE envelopes.

496d5e7

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Resolved merge conflicts with main.

fe8f53b

The biggest change is adapting the new interface where attestation func is split to two, one to get the key and a nother to get key/val. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Ran go mod tidy after resolving merge committs.

8ed2c98

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added check to see that provided EC key uses the P256 curve.

a6ec0b7

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten added 8 commits June 21, 2022 07:40

Spelled out aad when printing the help message.

4ef4d78

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Updated copyright notice to have current (2022) year.

c6c2de7

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Removed direct dependency on github.com/pkg/errors and replaced with …

7a393eb

…stdlib errors package. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Fixed a bug where nil was wrongfully returned instead of err.

afd7ace

Also increas the general test coverage. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Changed aad in artifact properties struct to be []byte instead of str…

4c65fcc

…ing. This gives the caller the possibility to decide how to decode the data. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Fixed a warning from the linter.

19834df

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added test case for malformed base64 aad parameter.

14d83fb

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

During signature validation, do not store any state until entire vali…

9e8f869

…dation is done. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten dismissed bobcallaway’s stale review via 9e8f869 June 21, 2022 05:41

kommendorkapten force-pushed the add_cose branch from 142be5b to 9e8f869 Compare June 21, 2022 05:41

dlorenc approved these changes Jun 23, 2022

View reviewed changes

dlorenc merged commit 3aec76f into sigstore:main Jun 23, 2022

github-actions bot added this to the v1.0.0 milestone Jun 23, 2022

kommendorkapten deleted the add_cose branch August 4, 2022 09:09

This was referenced Mar 7, 2023

Vast Terminology Overhaul ietf-wg-scitt/draft-ietf-scitt-architecture#16

Merged

Document rationale for COSE vs DSSE etc. ietf-wg-scitt/draft-ietf-scitt-architecture#57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add COSE support to Rekor #867

Add COSE support to Rekor #867

kommendorkapten commented Jun 9, 2022

dlorenc commented Jun 9, 2022

kommendorkapten commented Jun 9, 2022

dlorenc left a comment

dlorenc Jun 10, 2022

kommendorkapten Jun 10, 2022

dlorenc Jun 10, 2022

codecov-commenter commented Jun 13, 2022 •

edited

Loading

dlorenc commented Jun 13, 2022

dlorenc commented Jun 13, 2022

bobcallaway left a comment

bobcallaway Jun 13, 2022

kommendorkapten Jun 14, 2022

bobcallaway Jun 13, 2022

kommendorkapten Jun 14, 2022

bobcallaway Jun 14, 2022

bobcallaway left a comment •

edited

Loading

kommendorkapten commented Jun 14, 2022 •

edited

Loading

kommendorkapten commented Jun 14, 2022

kommendorkapten commented Jun 14, 2022

kommendorkapten commented Jun 14, 2022 •

edited

Loading

bobcallaway commented Jun 14, 2022 via email

kommendorkapten commented Jun 14, 2022

kommendorkapten commented Jun 14, 2022 •

edited

Loading

bobcallaway commented Jun 14, 2022

lkatalin commented Jun 14, 2022

Add COSE support to Rekor #867

Add COSE support to Rekor #867

Conversation

kommendorkapten commented Jun 9, 2022

Summary

Ticket Link

Release Note

dlorenc commented Jun 9, 2022

kommendorkapten commented Jun 9, 2022

dlorenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 13, 2022 • edited Loading

Codecov Report

dlorenc commented Jun 13, 2022

dlorenc commented Jun 13, 2022

bobcallaway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcallaway left a comment • edited Loading

Choose a reason for hiding this comment

kommendorkapten commented Jun 14, 2022 • edited Loading

kommendorkapten commented Jun 14, 2022

kommendorkapten commented Jun 14, 2022

kommendorkapten commented Jun 14, 2022 • edited Loading

bobcallaway commented Jun 14, 2022 via email

kommendorkapten commented Jun 14, 2022

kommendorkapten commented Jun 14, 2022 • edited Loading

bobcallaway commented Jun 14, 2022

lkatalin commented Jun 14, 2022

codecov-commenter commented Jun 13, 2022 •

edited

Loading

bobcallaway left a comment •

edited

Loading

kommendorkapten commented Jun 14, 2022 •

edited

Loading

kommendorkapten commented Jun 14, 2022 •

edited

Loading

kommendorkapten commented Jun 14, 2022 •

edited

Loading