-
Notifications
You must be signed in to change notification settings - Fork 186
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
Sign image based gadgets #2678
Sign image based gadgets #2678
Conversation
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.
This greatly enhances security for critical environments, thanks! A couple of comments from a first pass.
pkg/oci/oci.go
Outdated
@@ -478,16 +490,85 @@ func newAuthClient(repository string, authOptions *AuthOptions) (*oras_auth.Clie | |||
}, nil | |||
} | |||
|
|||
func prepareCheckOpts(ctx context.Context, publicKey string) (*cosign.CheckOpts, error) { | |||
pubKey, err := signature.LoadPublicKeyRaw([]byte(publicKey), crypto.SHA256) |
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.
Is it always gonna be crypto.SHA256
or do we have to make that customizable somehow?
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.
I do not think so.
But our key uses this algorithm and the only way to specify the algorithm would be by adding another flag which I would like to avoid at the moment.
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.
Yeah, it's fine for now but, would you mind creating an issue to make it configurable in the future?
pkg/operators/oci-handler/oci.go
Outdated
{ | ||
Key: verifyImage, | ||
Title: "Verify image", | ||
Description: "Verify image using the provided public key", | ||
DefaultValue: "true", | ||
TypeHint: api.TypeBool, | ||
}, | ||
{ | ||
Key: publicKey, | ||
Title: "Public key", | ||
Description: "Public key used to verify the image based gadget", | ||
DefaultValue: resources.InspektorGadgetPublicKey, | ||
TypeHint: api.TypeString, | ||
}, |
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 think it's okay to have these in here for testing (and also because it's not 100% cleanly implemented otherwise), those should eventually become GlobalParams
that are initialized from values set in the deployment/config file instead of being transferred using gRPC from the client (which all these InstanceParams currently are) to ensure safety. So maybe add a TODO for now so we don't forget.
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.
those should eventually become GlobalParams that are initialized from values set in the deployment/config file instead of being transferred using gRPC from the client (which all these InstanceParams currently are) to ensure safety.
Please share more details as I am indeed concerned by having the client sending this and I would like to avoid as much as possible stuff happening on the client.
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.
Sure!
Operators have two config parts - a global and a per-gadget one (instance params). The global part should be initialized with configuration values from the host itself (e.g. params given to the server binary on the host or a config file on that machine), while all the per-gadget ones are forwarded to the client and can be filled out from there and sent back. So everything concerning security should be coming from the global part or at least be restricted from there (e.g. "a user may override this or that setting" - where user would refer to a client connection).
On ig
, both params are treated similarly, as it's both client and server/host.
We sadly still don't have support for config files / generic params from the deployment, so the global params don't get used a lot right now. IMHO, this should definitely be handled before removing the experimental flag from image based gadgets.
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.
Probably we should add a signing job in the gadget-template's CI and a guide to describe how to use it properly.
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.
Probably we should add a signing job in the gadget-template's CI and a guide to describe how to use it properly.
This is clearly out of scope of this PR and #2707.
Particularly because this gadget-template
has absolutely no CI and the first step would be to build the gadget and push it somewhere before even thinking to sign it.
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.
What about creating a new file under docs/reference with this information?
Why not, but I will not deal with it in this PR, let's focus on the signing here.
I can drop the last commit if needed.
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.
There is definitely something I do not understand with the GlobalParams()
, as their values seem to never change:
$ DLV=../../../go/bin/dlv make debug-ig francis/verify-img-based-gadgets *% u=
...
(dlv) b localmanager.Init
Breakpoint 1 set at 0x32d0316 for github.com/inspektor-gadget/inspektor-gadget/pkg/operators/localmanager.(*LocalManager).Init() ./pkg/operators/localmanager/localmanager.go:172
(dlv) r --docker-socketpath /foo/bar run --verify-image=false trace_exec
Process restarted with PID 20568
(dlv) c
...
(dlv)
> github.com/inspektor-gadget/inspektor-gadget/pkg/operators/localmanager.(*LocalManager).Init() ./pkg/operators/localmanager/localmanager.go:184 (PC: 0x32d05d0)
179: socketPath := ""
180: namespace := ""
181:
182: switch runtimeName {
183: case types.RuntimeNameDocker:
=> 184: socketPath = operatorParams.Get(DockerSocketPath).AsString()
185: case types.RuntimeNameContainerd:
186: socketPath = operatorParams.Get(ContainerdSocketPath).AsString()
187: namespace = operatorParams.Get(ContainerdNamespace).AsString()
188: case types.RuntimeNameCrio:
189: socketPath = operatorParams.Get(CrioSocketPath).AsString()
...
(dlv) n
> github.com/inspektor-gadget/inspektor-gadget/pkg/operators/localmanager.(*LocalManager).Init() ./pkg/operators/localmanager/localmanager.go:197 (PC: 0x32d086a)
192: default:
193: return commonutils.WrapInErrInvalidArg("--runtime / -r",
194: fmt.Errorf("runtime %q is not supported", p))
195: }
196:
=> 197: for _, r := range rc {
198: if r.Name == runtimeName {
199: log.Infof("Ignoring duplicated runtime %q from %v",
200: runtimeName, parts)
201: continue partsLoop
202: }
(dlv) p socketPath
"/run/docker.sock"
I am wondering if I will really implement everything related to these GlobalParams
in the existing PRs.
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.
I can drop the last commit if needed.
I'd prefer to drop it.
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.
There is definitely something I do not understand with the
GlobalParams()
, as their values seem to never change:
Operators' global params will not work because we are always using default values:
inspektor-gadget/pkg/gadget-context/run.go
Lines 44 to 51 in 162b48b
// Lazily initialize operator | |
// TODO: global params should be filled out from a config file or such; maybe it's a better idea not to | |
// lazily initialize operators at all, but just hand over the config. The "lazy" stuff could then be done | |
// if the operator is instantiated and needs to do work | |
err := op.Init(apihelpers.ToParamDescs(op.GlobalParams()).ToParams()) | |
if err != nil { | |
return nil, fmt.Errorf("initializing operator %q: %w", op.Name(), err) | |
} |
So, I suggest you to keep the verification params as instance params for this PR, until we properly implement support for setting operators' global params.
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.
The documentation in docs/devel/hello-world-gadget.md should be updated to explain how to sign third-party gadgets (next to the section "Pushing the gadget image to a container registry").
Do we really want to continue adding more information to the hello-world guide? IMO it's becoming too complex. What about creating a new file under
docs/reference
with this information?
IMO, we should add one sentence saying that we support signing and then a link to another document where we explain everything about it:
- By default, IG gadgets are signed.
- By default, IG will verify gadgets using the IG pub key.
- Verification ca be skipped using flag.
- People can use and create their own key-pair. Add an example of how to do it.
- And whatever I missed here 🙂
pkg/operators/oci-handler/oci.go
Outdated
}, | ||
{ | ||
Key: publicKey, | ||
Title: "Public key", |
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.
I think later on we need to support multiple public keys and/or provide a tool to quickly re-sign our binaries.
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.
Definitely no:
- I do not see the point of having multiple public keys, if people want to use several keys, they use
run
as many times as they have different key, i.e. if you want to check several software with different keys withgpg
, you just callgpg
several times. - I will not implement signing in Inspektor Gadget, as this is not the goal of the project. If people want to use sign, the world is full of excellent tools to do so (
cosign
,gpg
).
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.
Hmm, I think you're missing the client/server architecture in the picture. "Use run
as many times as they have different keys" isn't the case there. You would deploy IG and configure it to "only trust gadgets signed by X" and can't change that with just running another process on your client (see above comment regarding global params vs instance params as well.)
So I think configuring it as "only trust IG official gadgets + our own" could be a thing that might be asked for. If we only want to support a single public key at a time, we should at least provide some help explaining how to quickly resign binaries (I'm not suggesting we build our own binary/app for that, but at least docs and maybe a script.)
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.
Hmm, I think you're missing the client/server architecture in the picture. "Use
run
as many times as they have different keys" isn't the case there. You would deploy IG and configure it to "only trust gadgets signed by X" and can't change that with just running another process on your client (see above comment regarding global params vs instance params as well.)
In the context of GlobalParams
, this makes sense.
I will take a look at it, in the end this is just a matter of looping on an array to call the verifying function.
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.
At the moment, this is not possible to have several keys as the API does not support having array of strings as parameters:
inspektor-gadget/pkg/gadget-service/api/consts.go
Lines 46 to 63 in 42182cc
TypeUnknown = "" | |
TypeBool = "bool" | |
TypeString = "string" | |
TypeBytes = "bytes" | |
TypeInt = "int" | |
TypeInt8 = "int8" | |
TypeInt16 = "int16" | |
TypeInt32 = "int32" | |
TypeInt64 = "int64" | |
TypeUint = "uint" | |
TypeUint8 = "uint8" | |
TypeUint16 = "uint16" | |
TypeUint32 = "uint32" | |
TypeUint64 = "uint64" | |
TypeFloat32 = "float32" | |
TypeFloat64 = "float64" | |
TypeDuration = "duration" | |
TypeIP = "ip" |
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.
Let's keep it as TODO in an issue for another PR.
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.
Awesome! 💯
pkg/operators/oci-handler/oci.go
Outdated
{ | ||
Key: verifyImage, | ||
Title: "Verify image", | ||
Description: "Verify image using the provided public key", | ||
DefaultValue: "true", | ||
TypeHint: api.TypeBool, | ||
}, | ||
{ | ||
Key: publicKey, | ||
Title: "Public key", | ||
Description: "Public key used to verify the image based gadget", | ||
DefaultValue: resources.InspektorGadgetPublicKey, | ||
TypeHint: api.TypeString, | ||
}, |
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.
Probably we should add a signing job in the gadget-template's CI and a guide to describe how to use it properly.
pkg/oci/oci.go
Outdated
@@ -478,16 +490,85 @@ func newAuthClient(repository string, authOptions *AuthOptions) (*oras_auth.Clie | |||
}, nil | |||
} | |||
|
|||
func prepareCheckOpts(ctx context.Context, publicKey string) (*cosign.CheckOpts, error) { | |||
pubKey, err := signature.LoadPublicKeyRaw([]byte(publicKey), crypto.SHA256) |
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.
Yeah, it's fine for now but, would you mind creating an issue to make it configurable in the future?
e635626
to
1772698
Compare
ff56396
to
364cd3c
Compare
Hi! I split this PR in two parts:
I tested the current part on my fork: And everything work as expected: $ cosign verify --key eiffel-fl.pub ghcr.io/eiffel-fl/gadget/trace_exec:latest
Verification for ghcr.io/eiffel-fl/gadget/trace_exec:latest --
The following checks were performed on each of these signatures:
- The cosign claims were validated
- Existence of the claims in the transparency log was verified offline
- The signatures were verified against the specified public key
[{"critical":{"identity":{"docker-reference":"ghcr.io/eiffel-fl/gadget/trace_exec"},"image":{"docker-manifest-digest":"sha256:3bba250435a88c734b180e59b7750d082133ba4d58812ddfdf358c97050626d1"},"type":"cosign container image signature"},"optional":{"Bundle":{"SignedEntryTimestamp":"MEQCIBx+1R16CYqRjfw9VEgZrxBdgq31bxQqXr5/4Stvw22XAiBp6ECxfwPUDX3SdZnjMzC2jdmb3CrnvY0tcARchXoVtA==","Payload":{"body":"eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaGFzaGVkcmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiI5ZmE0MTY1MDM1ZDVjN2JhMDZkYWExODBjNjAyZThlNTM5MThhZmI2MWMwM2QxODYxMjdkMGY1MmI2MDgyOGRhIn19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FVUNJRUJZQ1JqSWppRklpcUgvVUQ3cUhXdncwV2FXSEwxS1NCVW52OThMR0EyQ0FpRUEwUjIzSDJZTVExRXZXWnNwbEt2NVZnRWVxZ3QwL0pSbDJkL0VPVGpEamNZPSIsInB1YmxpY0tleSI6eyJjb250ZW50IjoiTFMwdExTMUNSVWRKVGlCUVZVSk1TVU1nUzBWWkxTMHRMUzBLVFVacmQwVjNXVWhMYjFwSmVtb3dRMEZSV1VsTGIxcEplbW93UkVGUlkwUlJaMEZGVEU1NU1YcEdOVVptYTNoWFQzTm5UR3BXY1dSUVYwTktOSEV5YVFwUmRrMXVSVk5qYW5WblRXZHZkMHRDVkRGQmIwazJhRTB4UnpsRlQzWXZPSGMyTDFGTkszVkJkVEpWVUcxYWJFSmFWemxCVEZwV1lYbFJQVDBLTFMwdExTMUZUa1FnVUZWQ1RFbERJRXRGV1MwdExTMHRDZz09In19fX0=","integratedTime":1712747894,"logIndex":84629524,"logID":"c0d23d6ad406973f9559f3ba2d1ca01f84147d8ffc5b8445c224f98b9591801d"}}}}] Best regards. |
364cd3c
to
c1f39ce
Compare
c1f39ce
to
3fb3529
Compare
3fb3529
to
42b0bfa
Compare
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
42b0bfa
to
c2ad3d7
Compare
fe74111
to
20fb0a5
Compare
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.
This looks good to me, apart of a couple of small comments I added above.
But since I'm not so familiar with the signing, I would like someone else to review as well.
0714b90
to
38f9171
Compare
docs/devel/hello-world-gadget.md
Outdated
@@ -207,6 +207,28 @@ Pushing ghcr.io/my-org/mygadget:latest... | |||
Successfully pushed ghcr.io/my-org/mygadget:latest@sha256:dd3f5c357983bb863ef86942e36f4c851933eec4b32ba65ee375acb1c514f628 | |||
``` | |||
|
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.
(Optional) ... ?
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.
I am not sure about this one, particularly as in next PR I set --verify-image
as true by default, so it does not really seem optional.
pkg/operators/oci-handler/oci.go
Outdated
{ | ||
Key: verifyImage, | ||
Title: "Verify image", | ||
Description: "Verify image using the provided public key", | ||
DefaultValue: "true", | ||
TypeHint: api.TypeBool, | ||
}, | ||
{ | ||
Key: publicKey, | ||
Title: "Public key", | ||
Description: "Public key used to verify the image based gadget", | ||
DefaultValue: resources.InspektorGadgetPublicKey, | ||
TypeHint: api.TypeString, | ||
}, |
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.
The documentation in docs/devel/hello-world-gadget.md should be updated to explain how to sign third-party gadgets (next to the section "Pushing the gadget image to a container registry").
Do we really want to continue adding more information to the hello-world guide? IMO it's becoming too complex. What about creating a new file under docs/reference
with this information?
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
38f9171
to
3a36d9c
Compare
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.
I have two comments but they're not blocking.
LGTM!
sign: | ||
@echo "Signing all gadgets" | ||
for GADGET in $(GADGETS); do \ | ||
digest=$$(sudo -E IG_EXPERIMENTAL=true $(IG) image list --no-trunc | grep "$$GADGET " | awk '{ print $$3 }') ; \ |
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.
I hit an issue with this. I rebuilt all the images using my personal container registry. So I have some duplicated image names:
$ sudo -E ig image list | grep trace_dns
INFO[0000] Experimental features enabled
trace_dns latest sha256:85ee296475d177ac8dc4a8c1376f497d4f53e9fdc8a8e7e19adf8fd97d109f80 2024-04-17T14:53:35-05:00
trace_dns v0.27.0 sha256:30c117c41b626b3289c053b34f812c981605c3ccaf2fe26eb4e037abdf490a2b 2024-04-02T08:02:25Z
trace_dns latest sha256:4df44b97f3a8413ebea56aa2739339f7dfd8dfda102adcfe8276f9ecbdca6cf6 2024-04-17T12:21:51Z
ghcr.io/mauriciovasquezbernal/gadget/trace_dns francis-sign-img-based-gadgets sha256:d1ca16a055ff984283ebcd86e28e6fea5f4573fbb412667f0ea010770d891124 2024-04-23T08:09:43-05:00
The sign fails because it gets the digest from the first image it finds:
s$ make sign
Signing all gadgets
for GADGET in trace_dns trace_exec trace_malloc trace_mount trace_oomkill trace_open trace_signal trace_sni trace_tcp trace_tcpconnect trace_tcpdrop trace_tcpretrans top_file snapshot_process snapshot_socket ci/sched_cls_drop ; do \
digest=$(sudo -E IG_EXPERIMENTAL=true ig image list --no-trunc | grep "$GADGET " | awk '{ print $3 }') ; \
cosign sign --key env://COSIGN_PRIVATE_KEY --yes --recursive ghcr.io/mauriciovasquezbernal/gadget/$GADGET@$digest || exit 1 ; \
done
INFO[0000] Experimental features enabled
Enter password for private key:
Error: signing [ghcr.io/mauriciovasquezbernal/gadget/trace_dns@sha256:85ee296475d177ac8dc4a8c1376f497d4f53e9fdc8a8e7e19adf8fd97d109f80 sha256:30c117c41b626b3289c053b34f812c981605c3ccaf2fe26eb4e037abdf490a2b sha256:4df44b97f3a8413ebea56aa2739339f7dfd8dfda102adcfe8276f9ecbdca6cf6 sha256:d1ca16a055ff984283ebcd86e28e6fea5f4573fbb412667f0ea010770d891124]: accessing entity: entity not found in registry, error: GET https://ghcr.io/v2/mauriciovasquezbernal/gadget/trace_dns/manifests/sha256:85ee296475d177ac8dc4a8c1376f497d4f53e9fdc8a8e7e19adf8fd97d109f80: MANIFEST_UNKNOWN: manifest unknown
main.go:74: error during command execution: signing [ghcr.io/mauriciovasquezbernal/gadget/trace_dns@sha256:85ee296475d177ac8dc4a8c1376f497d4f53e9fdc8a8e7e19adf8fd97d109f80 sha256:30c117c41b626b3289c053b34f812c981605c3ccaf2fe26eb4e037abdf490a2b sha256:4df44b97f3a8413ebea56aa2739339f7dfd8dfda102adcfe8276f9ecbdca6cf6 sha256:d1ca16a055ff984283ebcd86e28e6fea5f4573fbb412667f0ea010770d891124]: accessing entity: entity not found in registry, error: GET https://ghcr.io/v2/mauriciovasquezbernal/gadget/trace_dns/manifests/sha256:85ee296475d177ac8dc4a8c1376f497d4f53e9fdc8a8e7e19adf8fd97d109f80: MANIFEST_UNKNOWN: manifest unknown
make: *** [Makefile:52: sign] Error 1
Possible solutions could be:
--no-trunc
also prints the full url of the image. Then we can use the GADGET_REGISTRY in the grep expression- Implement json support (it should print the full url)
- Implement a
ig image show <foo>
command that prints all details about a specific image
I don't think we need to block this PR on that, we can open an issue once it's merged.
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.
I also had this issue in the CI and I added the extra space in "$$GADGET "
to cope with this.
Sadly, this --no-trunc
is really a workaround as we do not have JSON and I did not want to add extra dependencies on this PR.
The show/inspect
could also be welcomed.
I will think about it, but clearly I will not let this --no-trunc
here forever.
pkg/operators/oci-handler/oci.go
Outdated
{ | ||
Key: verifyImage, | ||
Title: "Verify image", | ||
Description: "Verify image using the provided public key", | ||
DefaultValue: "true", | ||
TypeHint: api.TypeBool, | ||
}, | ||
{ | ||
Key: publicKey, | ||
Title: "Public key", | ||
Description: "Public key used to verify the image based gadget", | ||
DefaultValue: resources.InspektorGadgetPublicKey, | ||
TypeHint: api.TypeString, | ||
}, |
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.
I can drop the last commit if needed.
I'd prefer to drop it.
3a36d9c
to
458a348
Compare
The CI is failing due to a CVE in |
Thank you for the reviews! |
Hi.
In this PR, I added signing of image based gadgets.
Best regards.