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

Upgrade cosign to 2.0.0 in tests #2109

Merged
merged 1 commit into from
Mar 19, 2023
Merged

Upgrade cosign to 2.0.0 in tests #2109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ WORKDIR /go/src/github.com/containerd/nerdctl
VOLUME /tmp
ENV CGO_ENABLED=0
# copy cosign binary for integration test
COPY --from=gcr.io/projectsigstore/cosign:v1.3.1@sha256:3cd9b3a866579dc2e0cf2fdea547f4c9a27139276cc373165c26842bc594b8bd /ko-app/cosign /usr/local/bin/cosign
COPY --from=gcr.io/projectsigstore/cosign:v2.0.0@sha256:728944a9542a7235b4358c4ab2bcea855840e9d4b9594febca5c2207f5da7f38 /ko-app/cosign /usr/local/bin/cosign
# enable offline ipfs for integration test
COPY ./Dockerfile.d/test-integration-etc_containerd-stargz-grpc_config.toml /etc/containerd-stargz-grpc/config.toml
COPY ./Dockerfile.d/test-integration-ipfs-offline.service /usr/local/lib/systemd/system/
Expand Down
12 changes: 12 additions & 0 deletions docs/cosign.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,18 @@ the [fulcio](https://github.com/sigstore/fulcio) root CA. Signatures are stored
the [rekor](https://github.com/sigstore/rekor) transparency log, which automatically provides an attestation as to when
the signature was created.

Cosign would use prompt to confirm the statement below during `sign`. Nerdctl added `--yes` to Cosign command, which says yes and prevents this prompt.
Using Nerdctl push with signing by Cosign means that users agree the statement.


```
Note that there may be personally identifiable information associated with this signed artifact.
This may include the email address associated with the account with which you authenticate.
This information will be used for signing this artifact and will be stored in public transparency logs and cannot be removed later.

By typing 'y', you attest that you grant (or have permission to grant) and agree to have this information stored permanently in transparency logs.
```

You can enable container signing and verifying features with `push` and `pull` commands of `nerdctl` by using `cosign`
under the hood with make use of flags `--sign` while pushing the container image, and `--verify` while pulling the
container image.
Expand Down
1 change: 1 addition & 0 deletions pkg/signutil/cosignutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func SignCosign(rawRef string, keyRef string) error {
cosignCmd.Env = append(cosignCmd.Env, "COSIGN_EXPERIMENTAL=true")
}

cosignCmd.Args = append(cosignCmd.Args, "--yes")
Copy link
Member

Choose a reason for hiding this comment

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

Hi @ningziwen , Is it relevant to expose the prompt feature as nerdctl options ? And make it silent within integration tests .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not to expose --yes, but to add --yes to make it silent by default. So it can be silent within integration tests and in the downstream software that is comsuming nerdctl.

The alternative option is to expose --cosign-yes in nerdctl. But I feel as Cosign is treated as the dependency of nerdctl feature, nerdctl can be kind of opininated to say yes to it, and make users have the expection of saying yes to this. (we can also add some documentation if needed)

Copy link
Member

@fahedouch fahedouch Mar 18, 2023 •
edited

Choose a reason for hiding this comment

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

LGTM but please introduce some documentation for users regarding this default --yes

Copy link
Member

@fahedouch fahedouch Mar 18, 2023 •
edited

Choose a reason for hiding this comment

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

FYI there is also a password prompt that can be disablied by exporting COSIGN_PASSWORD
source
Would you please reflect the prompt that you are targeting here in the PR comment and as I said before add some comments to the cosign doc. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think COSIGN_PASSWORD only disables the prompt of inputting cosign key passphrase. The prompt in this PR is only for private statement, which can't be disabled by COSIGN_PASSWORD.

	PrivacyStatement = `
        Note that there may be personally identifiable information associated with this signed artifact.
        This may include the email address associated with the account with which you authenticate.
        This information will be used for signing this artifact and will be stored in public transparency logs and cannot be removed later.`
	PrivacyStatementConfirmation = "        By typing 'y', you attest that you grant (or have permission to grant) and agree to have this information stored permanently in transparency logs."

--yes is also specifically for privacy statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added documentation.

Copy link
Contributor

@manugupt1 manugupt1 Mar 19, 2023 •
edited

Choose a reason for hiding this comment

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

From a privacy point of view, I think it is better to make it explicit for nerdctl users imo. We should make our users opt-in. It can be extended into the nerdctl config rather than having a cmd arg everytime.

Copy link
Member

Choose a reason for hiding this comment

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

agree regarding privacy point of view purpose, but I think the idea here is to unblock the upgrade to 2.0.0, so let's accept it as default value, and make new PR to make it explicit for nerdctl users

cosignCmd.Args = append(cosignCmd.Args, rawRef)

logrus.Debugf("running %s %v", cosignExecutable, cosignCmd.Args)
Expand Down