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

Incorporate "keyless" verification of base images. #436

Closed
wants to merge 1 commit into from

Conversation

mattmoor
Copy link
Collaborator

This change enables folks to start verifying their base images are signed against Fulcio by specifying COSIGN_EXPERIMENTAL=true, which will ensure each digest we intend to base an image on is verified.

Unless COSIGN_EXPERIMENTAL=true is enabled, this change will have no effect.

When COSIGN_EXPERIMENTAL=true is enabled, this change will verify base image digests, and emit a WARNING: when they are not signed (the gcr.io/distroless images are now signed!).

When COSIGN_EXPERIMENTAL=true is enabled AND --strict is passed, then failure to verify a base image digest will fail the ko command.

Related: #356
Related: #433

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #436 (a0be4cf) into main (501111b) will decrease coverage by 0.74%.
The diff coverage is 3.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   53.46%   52.71%   -0.75%     
==========================================
  Files          36       36              
  Lines        1835     1863      +28     
==========================================
+ Hits          981      982       +1     
- Misses        703      729      +26     
- Partials      151      152       +1     
Impacted Files Coverage Δ
pkg/commands/config.go 46.80% <3.57%> (-10.72%) ⬇️

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 501111b...a0be4cf. Read the comment docs.

@jonjohnsonjr
Copy link
Collaborator

I would like to take a chainsaw to the cosign dependency graph.

@mattmoor
Copy link
Collaborator Author

Ok cool, as expected with --strict the windows leg fails because the mcr..... image for Windows isn't signed, duh...

@mattmoor
Copy link
Collaborator Author

I would like to take a chainsaw to the cosign dependency graph.

Do it!

@imjasonh
Copy link
Member

Should take into account whatever guidance comes out of sigstore/cosign#677

@mattmoor
Copy link
Collaborator Author

... and sigstore/cosign#666 which seems very popular 😅

@imjasonh
Copy link
Member

Needs rebase.

This change enables folks to start verifying their base images are signed against Fulcio by specifying `COSIGN_EXPERIMENTAL=true`, which will ensure each digest we intend to base an image on is verified.

Unless `COSIGN_EXPERIMENTAL=true` is enabled, this change will have no effect.

When `COSIGN_EXPERIMENTAL=true` is enabled, this change will verify base image digests, and emit a `WARNING:` when they are not signed (the `gcr.io/distroless` images are now signed!).

When `COSIGN_EXPERIMENTAL=true` is enabled *AND* `--strict` is passed, then failure to verify a base image digest will fail the `ko` command.

Related: ko-build#356
@mattmoor
Copy link
Collaborator Author

Needs rebase.

Done

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

I think the main remaining hurdle from last week was excising unnecessary cosign deps so this doesn't explode the graph. Is that mainly done at this point, or is there more we should wait for?

@@ -55,6 +58,7 @@ var (
// getBaseImage returns a function that determines the base image for a given import path.
// If the `bo.BaseImage` parameter is non-empty, it overrides base image configuration from `.ko.yaml`.
func getBaseImage(platform string, bo *options.BuildOptions) build.GetBase {
v := &verifier{entries: make(map[name.Digest]*entry, 10)}
Copy link
Member

Choose a reason for hiding this comment

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

Why 10 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seemed like a small enough number to be reasonable, yet avoid resizing. I don't particularly care.

Copy link
Member

Choose a reason for hiding this comment

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

Same. I don't think there's any reason to care about the initial size of the map, so let's just YOLO-make it, or map[name.Digest]*entry{}.

result error
}

func (v *verifier) verify(ctx context.Context, digest name.Digest) error {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have a better idea how verification will look in the fullness of time. For now, it sounds like it's just "verify this image has any signature" -- does it also look it up in Rekor? Eventually folks might want to disable checking Rekor, or express other signature requirements.

Even just a sketch of how we envision that looking (in .ko.yaml?) would be helpful to me.

v.m.Lock()
defer v.m.Unlock()
// See if there's already an entry for this digest.
if e, ok := v.entries[digest]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

nit: You could hide v.entries initialization in here, instead of making it the responsibility of the verifier instantiator, above.

@mattmoor mattmoor closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants