Add cosign-based signing, attestation, and verification for modelkits#1023
Add cosign-based signing, attestation, and verification for modelkits#1023cr3ativ3cod3r wants to merge 6 commits intokitops-ml:mainfrom
Conversation
| ) | ||
|
|
||
| func RunSign(ctx context.Context, options *signOptions) error { | ||
| cmd := exec.CommandContext(ctx, "cosign", options.cosignArgs...) |
There was a problem hiding this comment.
Commands will panic with "executable file not found" when cosign binary is not installed on the system. This needs pre-flight checks before making these calls. Also is there a possibility to use cosign as a library instead of an external CLI ?
There was a problem hiding this comment.
Looked into use of cosign as a library. Found cosign(github.com/sigstore/cosign/v2/pkg/cosign). But it doesn't have a high level api to sign or attest. Actual signing happens in internal packages. Reviewed sigstore-go, which currently doesn't have attestation creation support.
I have added a check for cosign binary before execution of command.
pkg/cmd/attest/attest.go
Outdated
| "os/exec" | ||
| ) | ||
|
|
||
| func RunAttest(context context.Context, options *attestOptions) any { |
There was a problem hiding this comment.
Why does this return any and not error?
pkg/cmd/verify/cmd.go
Outdated
| return cmd | ||
| } | ||
|
|
||
| func runCommand(opts []verifyOptions) func(cmd *cobra.Command, args []string) error { |
There was a problem hiding this comment.
While this code works due to reassignment of opts it's confusing. The function signature runCommand([]verifyOptions{})suggests it takes an initialized slice, but it's always called with an empty slice and immediately populated. Refactor to use local variable for opts
There was a problem hiding this comment.
Made opts local to runCommand instead of passing it as input.
pkg/cmd/sign/sign.go
Outdated
|
|
||
| err := cmd.Run() | ||
| if err != nil { | ||
| return fmt.Errorf("signing failed %s", err) |
pkg/cmd/verify/cmd.go
Outdated
| func VerifyCommand() *cobra.Command { | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "verify [FLAGS]", |
pkg/cmd/attest/cmd.go
Outdated
|
|
||
| func AttestCommand() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "attest [FLAGS]", |
pkg/cmd/verify/cmd.go
Outdated
| return output.Fatalf("Failed to %s: %s", commands[i], err) | ||
| } | ||
| } | ||
| output.Infof("Modelkit signed") |
There was a problem hiding this comment.
Looks like a copy/paste error should be output.Infof("Modelkit verification successful")
Signed-off-by: Keerthan KK <tkthulasimandiram@gmail.com>
…cific flags Signed-off-by: Keerthan KK <tkthulasimandiram@gmail.com>
Signed-off-by: Keerthan KK <tkthulasimandiram@gmail.com>
Signed-off-by: Keerthan KK <tkthulasimandiram@gmail.com>
Signed-off-by: Keerthan KK <tkthulasimandiram@gmail.com>
Signed-off-by: Keerthan KK <tkthulasimandiram@gmail.com>
cr3ativ3cod3r
force-pushed
the
Implement-cosign-based-signing
branch
from
bcca243 to
0848136
Compare
amisevsk
left a comment
amisevsk
left a comment
There was a problem hiding this comment.
High level, this PR appears to be a very thin wrapper on just calling the cosign CLI without any additional functionality. As it stands I don't think it's something worth adding to the project.
Additionally, I haven't built and tested it locally yet, but I have a feeling signing won't work as a user expects -- what happens if the ModelKit has not yet been pushed to a remote repository?
Lastly, a small nitpick, but adding 3 subcommands to the root command here in my opinion complicates the CLI more than is necessary. They could all be subcommands of one sign command to keep the main help page simpler.
| func RunAttest(context context.Context, options *attestOptions) error { | ||
| _, err := exec.LookPath("cosign") | ||
| if err != nil { | ||
| fmt.Println() | ||
| return fmt.Errorf("cosign not found, please install cosign") | ||
| } | ||
| cmd := exec.CommandContext(context, "cosign", options.cosignArgs...) | ||
|
|
||
| cmd.Stdin = os.Stdin | ||
| cmd.Stderr = os.Stderr | ||
| cmd.Stdout = os.Stdout | ||
|
|
||
| err = cmd.Run() | ||
| if err != nil { | ||
| return fmt.Errorf("attestation failed: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The implementations for RunAttest, RunSign, and RunVerify appear to be identical.
3 participants
Description
This PR adds support for signing, attesting, and verifying modelkits using the kit commands directly. These commands use cosign internally. The users doesn't have to switch between multiple tools. The verify command enables the user to run both verify and verify attestation using a single command.
Linked issues
closes #857
AI-Assisted Code