fix: adds envelope hash to in-toto entries in tlog entry creation #2118

nkreiger · 2022-08-02T07:10:26Z

Signed-off-by: Noah Kreiger noahkreiger@gmail.com

Summary

By not using the types.NewProposedEntry the in-toto rekor function was not assiging the hash key, which caused a downstream error when querying by the hash to find the log entry after it was added, making it appear as if it did not get added.

This fix will add the envelope hash to in-toto entries during tlog entry creation that will allow the entries to be queried by the hash downstream.

Release Note

Bugfix adds envelope hash to in-toto entries in tlog entry creation

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

hectorj2f · 2022-08-02T08:27:11Z

pkg/cosign/tlog.go

-	returnVal := models.Intoto{
-		APIVersion: swag.String(e.APIVersion()),
-		Spec:       e.IntotoObj,
+	e, err := types.NewProposedEntry(context.Background(), "intoto", "0.0.1", types.ArtifactProperties{


Wouldn’t you use the context from the input argument as it ? It sounds like a better option to me.

Also I don’t feel hardcoding the intoto version as text is an improvement. Perhaps that is something to get from the api type.

@hectorj2f correct sorry I addressed the comments by isolating it to the specific intoto function, and allowing it to propagate the default version...maybe we want to keep it a specific version though to reduce the risk of dependency changes?

codecov-commenter · 2022-08-02T11:29:38Z

Codecov Report

Merging #2118 (a1b8b7b) into main (e4a1dff) will increase coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2118      +/-   ##
==========================================
+ Coverage   26.23%   26.26%   +0.03%     
==========================================
  Files         130      130              
  Lines        7617     7610       -7     
==========================================
+ Hits         1998     1999       +1     
+ Misses       5362     5355       -7     
+ Partials      257      256       -1

Impacted Files	Coverage Δ
pkg/cosign/tlog.go	`31.22% <0.00%> (+1.15%)`	⬆️

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

asraa · 2022-08-02T12:44:17Z

By not using the types.NewProposedEntry the in-toto rekor function was not assiging the hash key, which caused a downstream error when querying by the hash to find the log entry

What do you mean by hash key? I'm curious if you have a reproducer or diff on the change. Before and after your change I ran

COSIGN_EXPERIMENTAL=1 ./cosign attest --predicate pred.json --type slsaprovenance gcr.io/asra-ali/busybox/demo

and I was able to search both by payload hash. The entry contents themselves populate the same fields. (including content.hash.value and payloadHash.value)

If you meant hash key as in the merkle leaf hash then both have entry hashes that you can get log entry by UUID with.

nkreiger · 2022-08-02T15:24:39Z

By not using the types.NewProposedEntry the in-toto rekor function was not assiging the hash key, which caused a downstream error when querying by the hash to find the log entry

What do you mean by hash key? I'm curious if you have a reproducer or diff on the change. Before and after your change I ran
COSIGN_EXPERIMENTAL=1 ./cosign attest --predicate pred.json --type slsaprovenance gcr.io/asra-ali/busybox/demo
and I was able to search both by payload hash. The entry contents themselves populate the same fields. (including content.hash.value and payloadHash.value)

If you meant hash key as in the merkle leaf hash then both have entry hashes that you can get log entry by UUID with.

Hi @asraa...this is how I re-create the issue with the current cosign binary

COSIGN_EXPERIMENTAL=1 cosign attest --predicate test.json --type custom $someimage:0.0.1

...log entry created with index:...(mine was 3089316)

GET https://rekor.sigstore.dev/api/v1/log/entries?logIndex=3089316

which returns your log, then I base64 decode the "body", which gives you the hash, and payloadHash

in this case:

"content": {
            "hash": {
                "algorithm": "sha256",
                "value": "5fcf6b25abdc0a0becf5855d43361c4fad5f10290cdea213359526228bf540e8"
            },
            "payloadHash": {
                "algorithm": "sha256",
                "value": "9791b5658e1e832c889cf6518f600283365f819295916b27d0c2da0830cc975c"
            }
        },

then I attempt to retrieve the entry by the hash

curl --location --request POST 'https://rekor.sigstore.dev/api/v1/index/retrieve' \
--header 'Content-Type: application/json' \
--data-raw '{
    "hash": "sha256:5fcf6b25abdc0a0becf5855d43361c4fad5f10290cdea213359526228bf540e8"
}'

which returns []

with my changes that fix that bug, you will get an array of UUID back running this same flow

nkreiger · 2022-08-02T15:33:03Z

actually on further review @asraa you are right, you can use the payloadHash, with my changes it would let you search by the Hash or the PayloadHash, I was doing something hacky and was on a different version of rekor (cosign is on 0.4 I was on 0.10), so it looked like a bug to me from my local machine...if that is not desired behavior I can close this PR

also these changes would only work on upgrading to the latest rekor, which I am not sure if you want to do right now, lmk

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

asraa · 2022-08-02T15:41:52Z

with my changes it would let you search by the Hash or the PayloadHash, I was doing something hacky and was on a different version of rekor (cosign is on 0.4 I was on 0.10), so it looked like a bug to me from my local machine...if that is not desired behavior I can close this PR

I believe that there was an old bug that prevented old versions of Rekor from searching by content: sigstore/rekor#870 or sigstore/rekor#876

So I think it was just Rekor versions that "fixes" the bug.

That being said, you could consider this PR unifying the entry types and upload using types.NewProposedEntry, but I originally thought we avoided that to avoid too many deps. I don't have a preference, just wanted to jump in to fix the PR description in case anyone searches a similar issue!

nkreiger · 2022-08-02T16:02:18Z

@asraa thanks for posting that! That issue shows why that the changes I'm making still only allow you to search by the payloadHash on rekor 0.4, but allow you to search on either in 0.10

however, I still believe this is a bug on the cosign side (I tested the current cosign implementation against the latest rekor to confirm) due to this:

current implementation creates the intoto model, without the hash

https://github.com/sigstore/cosign/blob/main/pkg/cosign/tlog.go#L202

my proposed fix:

https://github.com/sigstore/rekor/blob/main/pkg/types/intoto/v0.0.1/entry.go#L293,L301

as you can see it is assigning the hash here, which allows you to query by it downstream

asraa · 2022-08-02T16:08:49Z

however, I still believe this is a bug on the cosign side (I tested the current cosign implementation against the latest rekor to confirm) due to this:

Thanks Noah! This is great. Definitely +1 to add the hash in this PR. Clarify the PR description to something like "Add envelope hash to in-toto entries in tlog entry creation"?

nkreiger · 2022-08-02T16:10:48Z

however, I still believe this is a bug on the cosign side (I tested the current cosign implementation against the latest rekor to confirm) due to this:

Thanks Noah! This is great. Definitely +1 to add the hash in this PR. Clarify the PR description to something like "Add envelope hash to in-toto entries in tlog entry creation"?

sure, updated

dlorenc · 2022-08-02T19:36:09Z

Not sure why tests aren't running here, but it looks like you need to rebase now.

pkg/cosign/tlog.go

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger · 2022-08-02T19:59:05Z

Not sure why tests aren't running here, but it looks like you need to rebase now.

@dlorenc sorry this is my first PR for a project this big, can you elaborate on what I need to do to run the tests?

asraa

sg! intoto entry change lgtm

dlorenc · 2022-08-03T10:52:46Z

Not sure why tests aren't running here, but it looks like you need to rebase now.

@dlorenc sorry this is my first PR for a project this big, can you elaborate on what I need to do to run the tests?

I don't think it's your fault - GitHub isn't running them for some reason. Can you rebase and force push? It might re-trigger them.

bobcallaway · 2022-08-03T14:40:31Z

I'm not sure we need to change anything in cosign, as the hash is computed and added server-side before making the entry into the log: https://github.com/sigstore/rekor/blob/102dc64d4f217013dfcf789837c3581d3f03b3d7/pkg/types/intoto/v0.0.1/entry.go#L202

Are you really seeing client behavior differences between omitting it and specifying it?

nkreiger · 2022-08-03T15:41:51Z

I'm not sure we need to change anything in cosign, as the hash is computed and added server-side before making the entry into the log: https://github.com/sigstore/rekor/blob/102dc64d4f217013dfcf789837c3581d3f03b3d7/pkg/types/intoto/v0.0.1/entry.go#L202

Are you really seeing client behavior differences between omitting it and specifying it?

@bobcallaway yes, it does add the hash on the server side, I am not seeing that the hash doesn't exist...only that the current implementation of cosign does not add the hash before sending to the server, and it makes it so you can not query on it downstream:

#2118 (comment)

see this example earlier in the PR

the only difference as far as I could tell was cosign doesn't use the types.NewProposedEntry for intoto, which adds the hash, and doing that fixes downstream query issue (if you are on latest rekor)

do you think its an issue on the rekor server side? I don't have the bandwidth to look into that today, but I can investigate if you think that is necessary

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

asraa · 2022-08-03T15:46:40Z

Are you really seeing client behavior differences between omitting it and specifying it?

I believe it is happening. IndexKeys happens BEFORE canonicalization. sigstore/rekor#854

I guess the clarification here is that this prevented searching by envelope hash:

you can use the payloadHash, with my changes it would let you search by the Hash or the PayloadHash,

bobcallaway · 2022-08-03T16:13:50Z

Are you really seeing client behavior differences between omitting it and specifying it?

I believe it is happening. IndexKeys happens BEFORE canonicalization. sigstore/rekor#854

So I don't think the order of the calls matters (in the insertion flow, canonicalization happens before IndexKeys() is called); but I do see the issue that is being reported here. The V001Entry.IntotoObj isn't ever updated to include the computed hash, but is included in the byte array that gets inserted into the log.

IMO this should be fixed in rekor, not in cosign.

I guess the clarification here is that this prevented searching by envelope hash:

you can use the payloadHash, with my changes it would let you search by the Hash or the PayloadHash,

nkreiger · 2022-08-03T16:35:10Z

IMO this should be fixed in rekor, not in cosign.

agreed, however, I think there is still value in the PR which "unifies the entry types and upload using types.NewProposedEntry" to quote @asraa because it translates to use the

https://github.com/sigstore/rekor/blob/main/pkg/types/intoto/v0.0.1/entry.go#L264 CreateFromArtifactProperties and on the rekor-server side on upload of an entry of intoto type it is using Canocalize, Unmarshal, validate ... all off the same intoto entry struct, so I don't see the dependency issue that was the original concern?

lmk if you want me to close the PR though and open an issue on the rekor side

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger · 2022-08-12T17:28:36Z

thoughts? @bobcallaway

lmk if I should close the PR and move the issue, so I can stop bothering y'all :)

bobcallaway · 2022-08-12T23:29:42Z

pkg/cosign/tlog.go

+	e, err := types.NewProposedEntry(ctx, intoto.KIND, intoto_v001.APIVERSION, types.ArtifactProperties{
+		ArtifactBytes:  signature,
+		PublicKeyBytes: pubKey,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return e, err


Suggested change

e, err := types.NewProposedEntry(ctx, intoto.KIND, intoto_v001.APIVERSION, types.ArtifactProperties{

ArtifactBytes: signature,

PublicKeyBytes: pubKey,

})

if err != nil {

return nil, err

}

return e, err

return types.NewProposedEntry(ctx, intoto.KIND, intoto_v001.APIVERSION, types.ArtifactProperties{

ArtifactBytes: signature,

PublicKeyBytes: pubKey,

})

bobcallaway

one nit but otherwise LGTM

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger · 2022-08-13T02:26:40Z

one nit but otherwise LGTM

@bobcallaway good catch, resolved, all commits signed off, lmk if there is anything else I need to do!

dlorenc · 2022-08-13T10:25:36Z

Should be good now, thanks for your patience!

dlorenc · 2022-08-13T10:30:36Z

Oh whoops, it looks like you might have to drop the go.mod changes, now we're getting a build failure due to downgrading rekor.

nkreiger · 2022-08-13T19:14:08Z

@dlorenc you're right, looks like it is in the release-utils pkg bumpt to 0.7.3 that came with the rekor bump

#2102 (there is a PR open actually to bump that pkg), maybe I could wait for that to go through? or should I revert the go.mod changes?

regardless, I added the replace command in the go.mod which fixed all the tests without having to rollback the rekor version, is that fine?

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

dlorenc · 2022-08-14T01:32:33Z

No idea what's wrong with that one, I'll poke dependabot and see if that helps. If that doesn't get fixed by tomorrow or so we can merge this and fix after. Thanks!

dlorenc · 2022-08-14T03:11:43Z

OK, #2102 is in, can you drop the replace and rebase?

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger · 2022-08-14T18:55:56Z

@dlorenc dropped

dlorenc · 2022-08-14T19:40:13Z

Awesome, let's get this in!

dlorenc · 2022-08-14T20:12:28Z

Thank you so much for the patience here!

…gstore#2118) * bufix resoled Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * clean up unused Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * needs to be upgraded Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * rm test file Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * clean up for comments Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * accidentally added test Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * force rebase Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * fix linting issue Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * deleted image Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * restore gif Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * see if this fixes the build issue Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> * merge in main branch with release utils changes Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

lcarva · 2022-08-23T18:42:35Z

FYI, I'm seeing issues with verify-attestation starting with this change: #2197

csullivanupgrade · 2022-09-13T16:28:20Z

govulncheck is reporting a type mismatch when using sigstore/rekor types.NewProposedEntry:

govulncheck: Packages contain errors:
/Users/csullivan/go/pkg/mod/github.com/sigstore/cosign@v1.11.1/pkg/cosign/tlog.go:89:19: cannot use pubKey (variable of type []byte) as [][]byte value in struct literal

csullivanupgrade · 2022-09-13T16:30:04Z

It looks like this was just recently changed on Rekor side:
sigstore/rekor#973

nkreiger added 2 commits August 2, 2022 02:06

bufix resoled

55a167c

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

clean up unused

9963b76

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

hectorj2f reviewed Aug 2, 2022

View reviewed changes

nkreiger added 2 commits August 2, 2022 10:33

needs to be upgraded

9af85b4

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

rm test file

8d5f300

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger changed the title ~~fix: rekor in-toto issue with tlog creation~~ fix: adds envelope hash to in-toto entries in tlog entry creation Aug 2, 2022

asraa reviewed Aug 2, 2022

View reviewed changes

pkg/cosign/tlog.go Outdated Show resolved Hide resolved

pkg/cosign/tlog.go Outdated Show resolved Hide resolved

nkreiger added 2 commits August 2, 2022 14:56

clean up for comments

f019a63

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

accidentally added test

44f884d

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

asraa reviewed Aug 2, 2022

View reviewed changes

force rebase

8a3cffb

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger added 3 commits August 5, 2022 13:26

sync back to main branch

da3fce5

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

fix linting issue

076a6b4

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

merge go.sum

b86723e

bobcallaway reviewed Aug 12, 2022

View reviewed changes

nkreiger added 3 commits August 12, 2022 18:33

address proposed change, sync to main

07caaef

deleted image

ffc816d

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

restore gif

371bdee

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

see if this fixes the build issue

415ba9d

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

nkreiger added 2 commits August 14, 2022 11:43

Merge branch 'main' of github.com:sigstore/cosign

32285ae

merge in main branch with release utils changes

a1b8b7b

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>

dlorenc approved these changes Aug 14, 2022

View reviewed changes

dlorenc merged commit 734869c into sigstore:main Aug 14, 2022

github-actions bot added this to the v1.11.0 milestone Aug 14, 2022

asraa mentioned this pull request Aug 24, 2022

fix: fix typo that caused attestation verification failure #2199

Merged

fix: adds envelope hash to in-toto entries in tlog entry creation #2118

fix: adds envelope hash to in-toto entries in tlog entry creation #2118

Conversation

nkreiger commented Aug 2, 2022 • edited Loading

Summary

Release Note

hectorj2f Aug 2, 2022

Choose a reason for hiding this comment

hectorj2f Aug 2, 2022

Choose a reason for hiding this comment

nkreiger Aug 2, 2022

Choose a reason for hiding this comment

codecov-commenter commented Aug 2, 2022 • edited Loading

Codecov Report

asraa commented Aug 2, 2022 • edited Loading

nkreiger commented Aug 2, 2022

nkreiger commented Aug 2, 2022 • edited Loading

asraa commented Aug 2, 2022 • edited Loading

nkreiger commented Aug 2, 2022

asraa commented Aug 2, 2022

nkreiger commented Aug 2, 2022

dlorenc commented Aug 2, 2022

nkreiger commented Aug 2, 2022

asraa left a comment

Choose a reason for hiding this comment

dlorenc commented Aug 3, 2022

bobcallaway commented Aug 3, 2022

nkreiger commented Aug 3, 2022

asraa commented Aug 3, 2022 • edited Loading

bobcallaway commented Aug 3, 2022

nkreiger commented Aug 3, 2022

nkreiger commented Aug 12, 2022

bobcallaway Aug 12, 2022

Choose a reason for hiding this comment

bobcallaway left a comment

Choose a reason for hiding this comment

nkreiger commented Aug 13, 2022

dlorenc commented Aug 13, 2022

dlorenc commented Aug 13, 2022

nkreiger commented Aug 13, 2022 • edited Loading

dlorenc commented Aug 14, 2022

dlorenc commented Aug 14, 2022

nkreiger commented Aug 14, 2022

dlorenc commented Aug 14, 2022

dlorenc commented Aug 14, 2022

lcarva commented Aug 23, 2022

csullivanupgrade commented Sep 13, 2022

csullivanupgrade commented Sep 13, 2022

nkreiger commented Aug 2, 2022 •

edited

Loading

codecov-commenter commented Aug 2, 2022 •

edited

Loading

asraa commented Aug 2, 2022 •

edited

Loading

nkreiger commented Aug 2, 2022 •

edited

Loading

asraa commented Aug 2, 2022 •

edited

Loading

asraa commented Aug 3, 2022 •

edited

Loading

nkreiger commented Aug 13, 2022 •

edited

Loading