-
Notifications
You must be signed in to change notification settings - Fork 309
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
feat: ability to sign and verify with cosign #647
feat: ability to sign and verify with cosign #647
Conversation
ba2521a
to
5bc66a5
Compare
go.mod
Outdated
@@ -23,6 +24,8 @@ require ( | |||
github.com/opencontainers/image-spec v1.0.2 | |||
github.com/pkg/errors v0.9.1 | |||
github.com/shteou/go-ignore v0.3.0 | |||
github.com/sigstore/cosign v1.3.1 | |||
github.com/sirupsen/logrus v1.8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm no conftest maintainer, it seems rather odd to introduce an entire logging system for use in only two packages. Since conftest does not run as a server, I'm not sure a logging library would make sense even if introduced consistently in all packages, or WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I removed the logrus modules for logging.
e671895
to
101ac24
Compare
47ec0b5
to
ba5e810
Compare
Signed-off-by: Batuhan Apaydın <[email protected]> Co-authored-by: Furkan Türkal <[email protected]> Signed-off-by: Batuhan Apaydın <[email protected]>
ba5e810
to
1e12e51
Compare
Signed-off-by: Batuhan Apaydın <[email protected]>
Hi @developer-guy! Thanks for taking a look at implementing this feature. I don't think we should go the route of shelling out to I think there are two options here. First, is we import cosign as a library and use its functions, and the second is we update conftest to write the OCI out to disk via |
hello @jalseth, sorry for the late reply, I have been so busy these days, missed this one sorry 🤦♂️ cosign has lots of dependencies, one of the downsides of using it as a Go module is that it might increase the binary size, also make code more complex, or also, it might cause some dependency conflicts. Similar work is already being done for nerdctl project1 that's why I'm proposing that I mean, using cosign as an executable. Btw, we did all the tests on nerdctl cosign to make sure that everything works fine, we can do the same tests for conftest too. WDYT? Footnotes |
@developer-guy I would lean towards what @jalseth has proposed as well. Assuming it is possible to import the signing capability as a library, I think it's worth seeing if conflicts exist and how much bigger were talking. Maybe it's negligible. If that fails horribly, instead of shelling out, I'd rather see this as a plugin e.g. https://github.com/open-policy-agent/conftest/blob/master/contrib/plugins/kubectl/kubectl-conftest.sh |
Any additional thoughts on this @developer-guy ? |
Yep, I agree with you. I can convert that code to use cosign as a Go package to adapt signing/verifying capabilities 🙋🏻♂️ Does it make sense? Then, we can talk about which method we want to continue with. |
I honestly think the plugin route would be easiest, at least to start. The trade off is that the user needs the cosign binary, but given the original proposal was to shell out to it -- that seems acceptable. As you've mentioned, importing cosign could cause bloat which hurts users who don't want to use it. Also given that the project is relatively new, dealing with changes could be more difficult in code. |
Hello @developer-guy. Friendly ping, please provide an update on the status of this PR. |
imho: +1 to consume it as an external plug-in, without adding a dependency |
AFAIK, cosign side is planning to reduce the dependency size, then, at that time, we might start using cosign packages. But I guess in the meantime, the best option using cosign would be calling it's executable like @boranx said above. |
kindly ping 🙋🏻♂️ |
Are there anymore status updates for this? I think we've all agreed to introduce this functionality as a plugin, so the next step would be to update this PR to add a cosign plugin. |
Sorry If I'm misunderstood, but what do you mean by a plugin? |
@developer-guy conftest has the ability to support plugins (docs here: https://www.conftest.dev/plugins/) One example is the kubectl plugin (https://github.com/open-policy-agent/conftest/tree/master/contrib/plugins/kubectl) So this would be similar in that you would add a plugin to the plugins directory, and call |
Closing this due to inactivity. Please feel free to reopen or create a new PR if you decide to pick this up again. |
|
||
log.Printf("verifying image: %s\n", rawRef) | ||
|
||
cosignExecutable, err := exec.LookPath("cosign") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although the issue is closed, I wanted to raise a point: would it be possible making this via lib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh seems it has already been discussed, is there any plan to use signing/verifying capabilities as external lib? or an issue that can be linked also here?
Signed-off-by: Batuhan Apaydın [email protected]
Co-authored-by: Furkan Türkal [email protected]
Fixes #642
cc: @Dentrax @jpreese