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

fix: rekor get tlog entry with uuid #2058

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Jul 8, 2022

Signed-off-by: Asra Ali asraa@google.com

Sign blob w/ search otherwise doesn't work, because the UUID in the response of Rekor now includes a prefix shard UUID.

cc @priyawadhwa

Summary

Release Note

Documentation

Fixes #1406

priyawadhwa
priyawadhwa previously approved these changes Jul 11, 2022
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

nice!!

pkg/cosign/tlog.go Outdated Show resolved Hide resolved
@dlorenc
Copy link
Member

dlorenc commented Jul 11, 2022

Codegen or a rebase issue?

@priyawadhwa
Copy link
Contributor

I think an issue with compiling cosign, we might need to update to 1.18:

Error: ../../../go/pkg/mod/sigs.k8s.io/release-utils@v0.7.1/version/version.go:124:25: bi.Settings undefined (type *debug.BuildInfo has no field or method Settings)
note: module requires Go 1.18

@asraa
Copy link
Contributor Author

asraa commented Jul 11, 2022

I think an issue with compiling cosign, we might need to update to 1.18:

yeah, i'm seeing some problems when i run codegen locally as well. is this because rekor is at 1.18?

@priyawadhwa
Copy link
Contributor

I think that sigs.k8s.io/release-util dependency requires 1.18. looks like rekor recently pinned to a specific version of it and updated the go version at the same time -- sigstore/rekor#904

@asraa
Copy link
Contributor Author

asraa commented Jul 11, 2022

ah nice find! i'll open a PR to bump to 1.18

edit: pending #2059

@asraa
Copy link
Contributor Author

asraa commented Aug 3, 2022

Confirmed this fixes goreleaser:

$ COSIGN_EXPERIMENTAL=1 ./cosign verify-blob   --signature https://github.com/goreleaser/goreleaser/releases/download/v1.10.3/checksums.txt.sig   https://github.com/goreleaser/goreleaser/releases/download/v1.10.3/checksums.txt
tlog entry verified with uuid: 6ad8e71d61e4757a7010ba3d31fca287db1e1e3ecdc09ebaeab317b7974f2581 index: 3045145
Verified OK

@asraa
Copy link
Contributor Author

asraa commented Aug 3, 2022

@priyawadhwa @dlorenc @hectorj2f sorry this got lost after the golang bump! but this will fix the recent goreleaser issue so I am reviving it.

@asraa
Copy link
Contributor Author

asraa commented Aug 3, 2022

Once again I have some dep problems... :/

caarlos0
caarlos0 previously approved these changes Aug 3, 2022
@asraa
Copy link
Contributor Author

asraa commented Aug 3, 2022

@imjasonh I think I still require go 1.18 #2093

@dlorenc
Copy link
Member

dlorenc commented Aug 14, 2022

You should be able to rebase now!

@asraa
Copy link
Contributor Author

asraa commented Aug 15, 2022

Updated, thank you!

Note the comment in the dep check: If needed, I'll see if I can split up the sharding package's sharding computation from the stuff that requires Trillian clients.

imjasonh
imjasonh previously approved these changes Aug 15, 2022
Copy link
Member

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

Actually we need to hold on the trillian dep check I think, cc @vaikas and @mattmoor

@asraa
Copy link
Contributor Author

asraa commented Aug 15, 2022

Actually we need to hold on the trillian dep check I think, cc @vaikas and @mattmoor

Yeah, if this can't be WIP for a little time, I can copy/paste the sharding code with a TODO to remove the copy-paste. LMK

@vaikas
Copy link
Contributor

vaikas commented Aug 16, 2022

I'd personally prefer to not pull that in as it has caused (well, not Trillian alone, but things with glog and other huge deps. My preference would be (I can't believe I'm saying this ;) ) cut&paste.

When you say it would be wip for little time, what's the actual fix that we would have to do?

But from this PR I don't quite get why we need to comment out the trillian bit, this doesn't seem to bring in new deps added, so I don't understand why we need to remove it.
My second question is just curiousity about leaking the the implementation details (but we don't have to discuss it here) about the trillian that sits behind rekor.

@vaikas
Copy link
Contributor

vaikas commented Aug 16, 2022

Also, I know folks are sensitive to deps and sizes (I know when I inadvertently pulled in something that had a k8s dep :) )

@imjasonh
Copy link
Member

imjasonh commented Aug 16, 2022

The sharding.GetUUIDFromIDString method contains no trillian deps.

In fact, only needs the Trillian LogClient, and only that to have the method GetLatestSignedLogRoot, which unfortunately takes a trillian struct, otherwise we could replace trillian.TrillianLogClient with interface { GetLatestSignedLogRoot(...) } and remove this dep on trillian.

Another option would be to define a new interface TrillianLogClient with Get(treeID) and a real implementation somewhere in rekor/cosign that depends on trillian, only used by cmd/cosign and the Rekor server. This would remove dependencies from sharding->trillian, and would let us use GetUUIDFromIDString more easily in cosign and its deps. edit: This is a bit complicated because the LogRootV1 type is also responsible for unmarshaling the response bytes: https://github.com/sigstore/rekor/blob/83a4094253c9267d664dfddcf44ad5929a626bee/pkg/sharding/ranges.go#L99

Or, as @vaikas says, copy and paste sharding.GetUUIDFromIDString into cosign.

@asraa
Copy link
Contributor Author

asraa commented Aug 16, 2022

Or, as @vaikas says, copy and paste sharding.GetUUIDFromIDString into cosign.

Yeah exactly. I could either do this, since it's not complicated logic, but my main idea was to separate these sharding utils from the sharding code that manipulates and requires the trillian log client.

I'll resolve it internally. Before GA we will expose verification libraries with few deps in rekor anyway. And that will supersede all this code.

@asraa
Copy link
Contributor Author

asraa commented Aug 16, 2022

All set, PTAL. Added tests for all the cases of whether Rekor will return EntryUUIDs or UUIDs to make sure matching occurs correctly.

@dlorenc
Copy link
Member

dlorenc commented Aug 16, 2022

Looks like lint failures, feel free to either fix or skip for the copy/pasta'd code.

Signed-off-by: Asra Ali <asraa@google.com>

update sharding

Signed-off-by: Asra Ali <asraa@google.com>

lint

Signed-off-by: Asra Ali <asraa@google.com>
@dlorenc dlorenc merged commit 339c0d7 into sigstore:main Aug 16, 2022
@github-actions github-actions bot added this to the v1.11.0 milestone Aug 16, 2022
cldmnky pushed a commit to cldmnky/cosign that referenced this pull request Aug 21, 2022
Signed-off-by: Asra Ali <asraa@google.com>

update sharding

Signed-off-by: Asra Ali <asraa@google.com>

lint

Signed-off-by: Asra Ali <asraa@google.com>

Signed-off-by: Asra Ali <asraa@google.com>
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.

verify-blob not finding tlog on rekor, but it is there
7 participants