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.

Sign up for GitHub

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

Jump to bottom

Add Cosign verification support #1598

Merged
merged 5 commits into from
Jul 11, 2022
Merged

Add Cosign verification support #1598

merged 5 commits into from
Jul 11, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 6, 2022 •
edited
Loading

⚠️ 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.

type: cosignSigned, with the usual keyData/keyPath. Fulcio/Rekor is plausible for the off-line Rekor log entry proofs, but not currently implemented. Tests first.

Note: This only allows a single public key, not a keyring, unlike simple signing. That seems problematic, there are known users of that. But we can fix that later by adding keyDirectory and the like.

Depends on unmerged #1594 and #1596.

@mtrmac mtrmac mentioned this pull request Jul 6, 2022
Closed
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 6, 2022 •
edited
Loading

To do: Needs documentation of the policy.json additions.

@mtrmac mtrmac mentioned this pull request Jul 6, 2022
Merged
@mtrmac mtrmac force-pushed the cosign-verify branch 5 times, most recently from 657eeb5 to 3939152 Compare July 7, 2022 13:05
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 7, 2022

Manual testing results:

  • Seems to be interoperable with Cosign sign #1597.
  • Seems to be interoperable with cosign, as long as:
    • Multi-arch images are signed with--recursive (sign per-arch images as well); that’s not the default, but it’s necessary to be able to refer to per-arch variants by digest. So that’s worth recommending upstream.
    • policy.json uses "signedIdentity": {"type": "matchRepository"}. Note that this allows attackers to substitute older versions if tags are used for versions.

Example policy.json fragment

                {
                    "type": "cosignSigned",
                    "keyPath": "/some/path/to/cosign.pub",
                    "signedIdentity": {
                        "type": "matchRepository"
                    }
                }

So, good? NOTE the sign --recursive requirement.

Still, this must not be merged without unit tests.

@mtrmac mtrmac force-pushed the cosign-verify branch 2 times, most recently from 9ddfefb to 3a4b0f9 Compare July 7, 2022 14:54
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 7, 2022

Added documentation.

mtrmac
mtrmac commented Jul 7, 2022
View reviewed changes
docs/containers-policy.json.5.md
Exactly one of `keyPath` and `keyData` must be present, containing a Cosign public key. Only signatures made by this key is accepted.

The `signedIdentity` field has the same semantics as in the `signedBy` requirement described above.
Note that `cosign`-created signatures only contain a repository, so only `matchRepository` and `exactRepository` can be used to accept them (and that does not protect against substitution of a signed image with an unexpected tag).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

… and remapIdentity, useful for non-registries.conf mirroring, does not work for repo-only signatures like that.

@vrothberg
Copy link
Member

Needs a rebase

@vrothberg
Copy link
Member

I am running out of time today but I will have a look tomorrow morning.

@mtrmac mtrmac mentioned this pull request Jul 7, 2022
Open
16 tasks
mtrmac added 4 commits July 7, 2022 18:34
@mtrmac
Fix a long-standing incorrect comment
959b0f9 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Fix JSON syntax in the policy.json(5) man page
69adefa 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Correctly decode Cosign-generated payloads
c8ba410 
... which can have "optional": null .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Add private.UnparsedImage, use it for signature handling
bdb2613 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
vrothberg
vrothberg reviewed Jul 8, 2022
View reviewed changes
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.

Code LGTM

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 8, 2022

Code changes since the review:

  • Moved the cosignSignature.LoadVerifier to VerifyCosignPayload, so that all crypto is handled in a single place.
  • Defined a cosignHarcodedHashAlgorithm constant.

I’d appreciate a sanity-check of the added tests as well.

@mtrmac
Add Cosign verification support
6475691 
type: cosignSigned, with the usual keyData/keyPath.
Fulcio/Rekor is not currently implemented.

NOTE: This only allows a single public key, not a keyring,
unlike simple signing. That seems problematic, there are
known users of that. But we can fix that later by adding
keyDirectory and the like.

NOTE: Cosign interoperability requires use of
signedIdentity: matchRepository. The fairly useful
signedIdentity: remapIdentity has no repository-match
functionality.

NOTE: Multi-arch images need to be signed by cosign
with --recursive to be accepted; c/image enforces
signatures per platform.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 8, 2022

Now ready for review and possibly merging.

@mtrmac mtrmac marked this pull request as ready for review July 8, 2022 16:40
@mtrmac mtrmac changed the title DO NOT MERGE IRRESPONSIBLE: Cosign verify Add Cosign verification support Jul 8, 2022
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 8, 2022

Code changes since the review:

Fixed a typo in an error message, removed a few FIXME comments.

mtrmac
mtrmac commented Jul 8, 2022
View reviewed changes
signature/policy_eval.go
logrus.Debugf("GetSignaturesWithAcceptedAuthor for image %s", policyIdentityLogName(image.Reference()))
reqs := pc.requirementsForImageRef(image.Reference())

// FIXME: rename Signatures to UnverifiedSignatures
// FIXME: pass context.Context
// FIXME: Use image.UntrustedSignatures, use that to improve error messages (needs tests!)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Outstanding, noted in #1601 .

vrothberg
vrothberg reviewed Jul 11, 2022
View reviewed changes
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

@mtrmac mtrmac merged commit f91dcd1 into containers:main Jul 11, 2022
@mtrmac mtrmac deleted the cosign-verify branch July 11, 2022 12:59
@mtrmac mtrmac mentioned this pull request Jul 18, 2022
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrothberg vrothberg vrothberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mtrmac @vrothberg