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

Cosign sign #1597

Merged
merged 1 commit into from Jul 7, 2022
Merged

Cosign sign #1597

merged 1 commit into from Jul 7, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 6, 2022

⚠️ Warning: This is write-only code, as in I haven’t read it after myself, it has never been run, and it has no tests yet. Might be completely broken. It really needs unit and an integration tests, and interoperability testing.

Private key only, no Fulcio/Rekor. Figuring out a sane UI for non-default Fulcio servers is quite a bit of more work than this (probably commits us to defining a new config file format).

Depends on unmerged #1594 and #1596.

signature/cosign/sign.go Outdated Show resolved Hide resolved
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 7, 2022

In manual testing, this produces signatures that Cosign accepts.

So, calling this ready for review. It could definitely do with unit tests, and integration tests in the Skopeo repo.

@mtrmac mtrmac marked this pull request as ready for review July 7, 2022 13:35
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

copy/sign.go Outdated
}
}

c.Printf("Signing manifest\n")
Copy link
Member

Choose a reason for hiding this comment

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

Shall we indicate the signing mechanism?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, done.

(I’ll gladly change the wording to any other names as long as I’m not required to participate in that discussion.)

@@ -1,4 +1,6 @@
4d63.com/gochecknoglobals v0.1.0/go.mod h1:wfdC5ZjKSPr7CybKEcgJhUOgeAQW1+7WcyK8OvUilfo=
Copy link
Member

Choose a reason for hiding this comment

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

Does go mod tidy clean up go.sum? I was surprised about the size of the diff and it seems go.sum changes account for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

go mod tidy does clean up completely unused packages. Some of the added entries are test dependencies (e.g. note the presence of github.com/golangci/golangci-lint)

Still, these are the dependencies that total 10 MB.

Copy link
Member

Choose a reason for hiding this comment

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

@Luap99 FYI, to prevent a heart attack. Much better than 90 MB but I am sure we can trim the size down over time.

Private key only, no Fulcio/Rekor.

The extra dependencies are not ideal, but not too bad
(notably the scary go-tuf addition is only a small subpackage). Still,
it's about extra 10 MB.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@vrothberg
Copy link
Member

LGTM
Feel free to merge

@rhatdan
Copy link
Member

rhatdan commented Jul 7, 2022

LGTM

@rhatdan rhatdan merged commit b61b339 into containers:main Jul 7, 2022
@mtrmac mtrmac mentioned this pull request Jul 7, 2022
16 tasks
@mtrmac mtrmac deleted the cosign-sign branch July 7, 2022 16:32
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.

None yet

3 participants