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

Support podman image sign #2040

Merged
merged 1 commit into from
Jan 9, 2019
Merged

Conversation

QiWang19
Copy link
Contributor

podman image sign [-d, --directory] [--sign-by] IMAGE
Generate signatures claim for the image using user keyring (--sign-by). The signature file will be stored in simple json format under the default or the given directory (--directory or yaml file in /etc/containers/registries.d/).

Signed-off-by: Qi Wang qiwan@redhat.com

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2018
@rhatdan
Copy link
Member

rhatdan commented Dec 21, 2018

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2018
},
}

signDescription = "Create a signature for an image which can be used later to verify it"
Copy link
Member

Choose a reason for hiding this comment

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

Create a signature file ...

Copy link
Member

Choose a reason for hiding this comment

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

We need to support --latest. Should we support --all?

Copy link
Member

Choose a reason for hiding this comment

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

perhaps: "Create a signature file that can be used later to verify the image"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now --latest is the default, only need to add --all?

cmd/podman/sign.go Show resolved Hide resolved
}
defer runtime.Shutdown(false)

signby := c.String("sign-by")
Copy link
Member

Choose a reason for hiding this comment

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

Can we default this to the current user? Can we figure out the default signature of the current user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Atomic set this default in a config file /etc/atomic.conf. Does it need to be added to some config file here?
the value of sign-by flag should be a keyring generated using gpg, rather than $USER, right?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a default signer in gpg?

Copy link
Member

Choose a reason for hiding this comment

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

Probably be username

Copy link
Member

Choose a reason for hiding this comment

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

gpgconf --list-options gpg | grep default-key

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 also found the gpg command to generate the key pair without user interaction. But cannot avoid user interaction if I want to execute podman image sign command in the test file 🤔

}

systemRegistriesDirPath := trust.RegistriesDirPath(runtime.SystemContext())
registryConfigs, err := trust.LoadAndMergeConfig(systemRegistriesDirPath)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a hidden registriesdirpath option to do 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.

yes, I need! But still wondering how to write the test for this. What sign-by value should I put there, or I may need to run gpg command to generate some key first. 😅

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think you will need to do this. Was their any atomic test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I can't find test for sign in atomic

Copy link
Contributor Author

@QiWang19 QiWang19 Jan 3, 2019

Choose a reason for hiding this comment

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

no, I can't find atomic test for this

cmd/podman/sign.go Outdated Show resolved Hide resolved
cmd/podman/sign.go Show resolved Hide resolved
cmd/podman/sign.go Outdated Show resolved Hide resolved
cmd/podman/sign.go Show resolved Hide resolved
docs/podman-image-sign.1.md Outdated Show resolved Hide resolved
docs/podman-image-sign.1.md Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member

rhatdan commented Dec 21, 2018

Have to update commands.md and bash completions.

@rhatdan
Copy link
Member

rhatdan commented Jan 3, 2019

/retest
bot, retest this please

@jwhonce jwhonce added this to the 1.0 milestone Jan 7, 2019
@rhatdan rhatdan changed the title [WIP]Support podman image sign Support podman image sign Jan 8, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2019
@rhatdan
Copy link
Member

rhatdan commented Jan 8, 2019

@QiWang19 Could you answer the last few questions. We can add tests later. Want to get this into V1.0.

@QiWang19
Copy link
Contributor Author

QiWang19 commented Jan 8, 2019

@rhatdan 👌 , can we also consider --all later?

Generate a signature claim for an image using user keyring (--sign-by). The signature file will be stored in simple json format under the default or the given directory (--directory or yaml file in /etc/containers/registries.d/).

Signed-off-by: Qi Wang <qiwan@redhat.com>
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

verified it works with rootless mode.

Could you split the commit message line (max 80 columns)?

docs/podman-image-sign.1.md Show resolved Hide resolved
logrus.Errorf("error creating sigstore file: %v", err)
continue
}
err = ioutil.WriteFile(sigStoreDir+"/"+sigFilename, newSig, 0644)
Copy link
Member

Choose a reason for hiding this comment

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

let's use filepath.Join here

return errors.Wrapf(err, "error creating new signature")
}

sigStoreDir = fmt.Sprintf("%s/%s", sigStoreDir, strings.Replace(repos[0][strings.Index(repos[0], "/")+1:len(repos[0])], ":", "=", 1))
Copy link
Member

Choose a reason for hiding this comment

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

same here, let's use filepath.Join

# SYNOPSIS
**podman image sign**
[**-h**|**--help**]
[**-d**, **--directory**]
Copy link
Member

Choose a reason for hiding this comment

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

Can you show the long form of the option first please?
`[--help|-h]

**-h** **--help**
Print usage statement.

**-d** **--directory**
Copy link
Member

Choose a reason for hiding this comment

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

ditto long form first, short form second.

configuration files in /etc/containers/registries.d/. When you sign
an image, podman will use those configuration files to determine
where to write the signature based on the the name of the originating
registry or a default storage value unless overriden with the -d
Copy link
Member

Choose a reason for hiding this comment

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

s/-d/--directory/

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

missing bash completions and tests aren't looking happy atm.

@rhatdan
Copy link
Member

rhatdan commented Jan 9, 2019

Ok I am going to merge and then we can fix the issues in sumplimental PR's
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2019
@rhatdan
Copy link
Member

rhatdan commented Jan 9, 2019

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit c37f731 into containers:master Jan 9, 2019
@QiWang19 QiWang19 deleted the signimg branch June 26, 2020 15:09
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants