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

Pluggable KMS integrations #386

Open
mattmoor opened this issue Apr 12, 2022 · 12 comments
Open

Pluggable KMS integrations #386

mattmoor opened this issue Apr 12, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@mattmoor
Copy link
Member

Description

This is something I've been marinating on for some time, but was driven to open a tracking issue by #384

Problem: baking in every KMS provider under the sun into sigstore/sigstore itself incurs a significant overhead on all downstream projects regardless of whether they even use this functionality.

I'll sketch out some ideas in a follow up comment.

@mattmoor mattmoor added the enhancement New feature or request label Apr 12, 2022
@mattmoor
Copy link
Member Author

For the current integrations, what I was thinking is to adopt a pattern similar to our cosign OIDC integrations which:

  1. Defines an interface that implementations should implement,
  2. Provides a registration hook for implementations to register themselves,
  3. Allows the binary consumers to link the implementations they want to include.

If sigstore/sigstore ONLY defines the interface here, then what we can do is:

  1. Move each KMS implementation of this interface into sigstore/kms-FOO,
  2. Have the cosign binary entrypoint link the current implementations.

This way tooling that leverages cosign as an SDK can more selectively pull in implementations of the various providers.

@mattmoor
Copy link
Member Author

mattmoor commented Apr 12, 2022

We could take the above a step further by allowing a "credential helper"-like fallback option.

Presumably the registration phase above would allow implementations to register the "scheme" that they support, e.g. awskms://, but suppose no built-in provider is found for a particular scheme, e.g. mattmoor://?

We could do the equivalent of which sigstore-kms-mattmoor, and (if it is found) invoke it with a standard CLI surface.

As exemplars the sigstore/kms-FOO repos above could define binary entrypoints implementing the above, likely with a shim around some standard entrypoint defined in sigstore/sigstore. If we want, we can even use this as a path to deprecate and remove the built-in integrations over time.

@mattmoor
Copy link
Member Author

@dlorenc @lukehinds @bobcallaway for thoughts ☝️

I can probably drive this, if folks think it would be a good refactor?

cc @vaikas @jdolitsky who I have been talking about this with.

@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

I think @imjasonh already did the interface/registration thing: https://github.com/sigstore/sigstore/blob/main/pkg/signature/kms/gcp/client.go#L43

@mattmoor
Copy link
Member Author

Looks like they are still linked from fairly core places:
https://github.com/sigstore/cosign/blob/c8e152ae907747604a3f85290c4f397bbef16276/pkg/signature/keys.go#L39-L43

I'm going to start by moving these out to the binary entrypoint (further out than most folks still use as a library) to try and avoid the MPL problem in the linked issue.

@nsmith5
Copy link

nsmith5 commented Apr 12, 2022

@haydentherapper and I discussed something like this quite recently here: sigstore/fulcio#496 (comment)

Nit: I prefer having imports with side effects (e.g loading database drivers and plugins etc) in package main. You can see an example of this behaviour here in distribution/distribution. This makes it really easy for folks to build a slimmed down version of the registry (or fulcio in our case) by just customizing that package main themselves.

Example

package main

import (
	"github.com/distribution/distribution/v3/registry"

	_ "github.com/distribution/distribution/v3/registry/auth/token"          # Only token auth
	_ "github.com/distribution/distribution/v3/registry/storage/driver/gcs"  # Only GCS storage


func main() {
	registry.RootCmd.Execute()
}

So yeah big +1 to moving all imports with side-effects out to the main entrypoint!

@nsmith5
Copy link

nsmith5 commented Apr 12, 2022

Just a case study of the how the pattern distribution/distribution used helped me: this made is extremely easy for me to create a new registry auth pattern and build my own OCI registry with just the drivers I needed at one point. Very powerful ❤️

mattmoor added a commit to mattmoor/cosign that referenced this issue Apr 12, 2022
Related: sigstore/sigstore#384
Related: sigstore/sigstore#386
Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

@haydentherapper and I discussed something like this quite recently here: sigstore/fulcio#496 (comment)

Nit: I prefer having imports with side effects (e.g loading database drivers and plugins etc) in package main. You can see an example of this behaviour here in distribution/distribution. This makes it really easy for folks to build a slimmed down version of the registry (or fulcio in our case) by just customizing that package main themselves.
Example

package main

import (
	"github.com/distribution/distribution/v3/registry"

	_ "github.com/distribution/distribution/v3/registry/auth/token"          # Only token auth
	_ "github.com/distribution/distribution/v3/registry/storage/driver/gcs"  # Only GCS storage


func main() {
	registry.RootCmd.Execute()
}

So yeah big +1 to moving all imports with side-effects out to the main entrypoint!

Moving the imports up will be a big win. I'm not sure we can delete these ones from here without some kind of backwards compatibility story because of semver, but we could always just cut a version 2.

@mattmoor
Copy link
Member Author

@dlorenc we can make them their own modules in-place here?

@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

@dlorenc we can make them their own modules in-place here?

idk, does that make semver mad?

@mattmoor
Copy link
Member Author

Moving the side effect is already breaking to some folks downstream. I was actually going to suggest we scramble to move these out before my PR lands in a release to avoid breaking folks twice, but making these modules in place here could be a nice middle ground.

This feels like a gray area of semver, since 1.x would have both in a single module and 1.x+1 would have both in different modules (but with identical names). Given how go mod solves versions based on package presence (vs. function signatures), my intuition says it would work, but I'm not sure I'm smart enough to answer definitively based on go mod semantics around semver.

@dlorenc
Copy link
Member

dlorenc commented Apr 12, 2022

I'm a habitual line stepper when it comes to semver. Let's give it a try!

mattmoor added a commit to sigstore/cosign that referenced this issue Apr 12, 2022
* Move the KMS integration imports into the binary entrypoints

Related: sigstore/sigstore#384
Related: sigstore/sigstore#386
Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

* Remove the fake import

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this issue May 6, 2022
…e#1744)

* Move the KMS integration imports into the binary entrypoints

Related: sigstore/sigstore#384
Related: sigstore/sigstore#386
Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

* Remove the fake import

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
mtrmac pushed a commit to mtrmac/sigstore that referenced this issue Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants