ig/gadgetctl: add TLS support #2848

flyth · 2024-05-15T00:12:50Z

This PR adds support for TLS encryption & authentication for ig running as daemon and gadgetctl to connect to it. It supports mutual verification by checking certificates against certificate authorities.

Here's how to test it (this short guide to create a CA and server/client keys is just an example!!!):

# Create CA and self-sign
openssl ecparam -name prime256v1 -genkey -noout -out ca.key
openssl req -new -x509 -subj "/C=DE/CN=CA" -sha256 -key ca.key -out ca.crt

# Create server key, csr & cert (note that you _have_ to use SAN (subjectAltName) for verification of server names later on)
openssl ecparam -name prime256v1 -genkey -noout -out server.key
openssl req -new -sha256 -subj "/C=DE/CN=local" -addext "subjectAltName = DNS:local" -key server.key -out server.csr
openssl x509 -req -in server.csr -CA ca.crt -CAkey ca.key -CAcreateserial -out server.crt -days 1000 -sha256 -copy_extensions copy

# Create client key, csr & cert
openssl ecparam -name prime256v1 -genkey -noout -out client1.key
openssl req -new -sha256 -subj "/C=DE/CN=local" -addext "subjectAltName = DNS:local" -key client1.key -out client1.csr
openssl x509 -req -in client1.csr -CA ca.crt -CAkey ca.key -CAcreateserial -out client1.crt -days 1000 -sha256

In this example both client and server are using the same CA - this is however not necessary.

Copy files ca.crt, server.key & server.cert to ig and another copy of ca.crt, client1.key & client1.cert to gadgetctl and run as follows:

sudo ./ig daemon --group 1000 -v --tls-cert server.crt --tls-key server.key --tls-client-ca ca.crt

(Replace 1000 as --group with your preferred group id to be able to connect from a gadgetctl not running as root.)

./gadgetctl run "trace_exec:latest" --host --tls-cert client1.crt --tls-key client1.key --tls-server-ca ca.crt --verify-image false --tls-server-name local

Since we're connecting locally, we have to specify the --tls-server-name we used as SAN when creating the key pairs above.

The server log will now contain information like this:

...
INFO[0002] [CN=local,C=DE] GetGadgetInfo("trace_exec:latest")
INFO[0002] [CN=local,C=DE] RunGadget("trace_exec:latest")
...

Signed-off-by: Michael Friese <mfriese@microsoft.com>

eiffel-fl

Hi!

I have some initial comments, but this is a great addition!

Best regards.

eiffel-fl · 2024-05-15T13:44:27Z

pkg/runtime/grpc/grpc-runtime.go

+		cert, err := tls.LoadX509KeyPair(tlsCert, tlsKey)
+		if err != nil {
+			return nil, fmt.Errorf("loading TLS keypair: %w", err)
+		}
+
+		ca := x509.NewCertPool()
+		caBytes, err := os.ReadFile(tlsCA)
+		if err != nil {
+			return nil, fmt.Errorf("loading client CA certificate: %w", err)
+		}
+		if ok := ca.AppendCertsFromPEM(caBytes); !ok {
+			return nil, errors.New("failed to parse client CA certificate")
+		}
+
+		purl, _ := url.Parse(target.addressOrPod)
+
+		tlsConfig := &tls.Config{
+			ServerName:   purl.Hostname(),
+			Certificates: []tls.Certificate{cert},
+			RootCAs:      ca,
+		}


This can be factorized with:
https://github.com/inspektor-gadget/inspektor-gadget/pull/2848/files#diff-9d3ae444cb907b9257ef6dfc4124c9b9df6bf3562f3bac164be73b06765292e9R123-R144

eiffel-fl · 2024-05-15T13:45:30Z

cmd/ig/daemon.go

+
+			options = append(options, grpc.Creds(credentials.NewTLS(tlsConfig)))
+		} else {
+			log.Warnf("no TLS configuration provided, running in insecure mode")


This is too vague:

Suggested change

log.Warnf("no TLS configuration provided, running in insecure mode")

log.Warnf("no TLS configuration provided, communication between daemon and CLI will not be ciphered")

eiffel-fl · 2024-05-15T13:46:22Z

pkg/runtime/grpc/grpc-runtime.go

+	ParamTLSKey        = "tls-key"
+	ParamTLSCert       = "tls-cert"
+	ParamTLSServerCA   = "tls-server-ca"
+	ParamTLSServerName = "tls-server-name"


Cannot you use these in the CLI too?

You mean sharing the consts? They would only share ParamTLSKey and ParamTLSCert, so I'd rather keep them local to the pkg using them.

It is still two you can share.
Moreover, if some are specific to this file, they should not be public.

Regarding keeping them local but exported:
Those Params can also be set when using the IG Go API for connecting to a remote - in that case IMHO it already helps if you're typing grpc.Param (when using that runtime) and your IDE can show you the available options. We've done the same with params for other operators (and should sooner or later do it for the remaining ones - but this is still in progress and discussion (also in regards to where to put their documentation)), so you can more easily find and use them.

Reusing them when being local to that package, though, would mean to introde a dependency between the server component and the client - which I think isn't a good option.

Also, even if the strings match, the meaning is different - in one case it's the client's keypair, in the other it's the servers'. Their meaning is implied from the context they're used in.

But I'll add dedicated consts to the cmd/ig package as well to keep it a bit cleaner at least.

/cc @mauriciovasquezbernal regarding the handling of Params

Regarding keeping them local but exported: Those Params can also be set when using the IG Go API for connecting to a remote - in that case IMHO it already helps if you're typing grpc.Param (when using that runtime) and your IDE can show you the available options. We've done the same with params for other operators (and should sooner or later do it for the remaining ones - but this is still in progress and discussion (also in regards to where to put their documentation)), so you can more easily find and use them.

Reusing them when being local to that package, though, would mean to introde a dependency between the server component and the client - which I think isn't a good option.

Indeed, let's avoid this.

Also, even if the strings match, the meaning is different - in one case it's the client's keypair, in the other it's the servers'. Their meaning is implied from the context they're used in.

Hum...
I rather find this confusing, but OK.

But I'll add dedicated consts to the cmd/ig package as well to keep it a bit cleaner at least.

/cc @mauriciovasquezbernal regarding the handling of Params

eiffel-fl · 2024-05-15T13:47:23Z

pkg/gadget-service/service-oci.go

+		if ok {
+			subject := tlsInfo.State.VerifiedChains[0][0].Subject
+			s.logger.Infof("[%s] GetGadgetInfo(%q)", subject, req.ImageName)
+		}


Should we explode if !ok?

No - it'll always be ok for TLS connections, as they're always verified. This will only be !ok if the connection is insecure (which is a valid and right now the default use case.)

What if it is no OK for whatsoever reason?
This is a bit paranoid, but I definitely would like to have a case where people think we are using TLS and we just end not using it.

Hmm... It's pretty much a rabbit hole with the question "To what degree do we trust the imported fundamental packages like gRPC to work"? But if there's ever a good time to be paranoid, this might be it - hehe. I'll add that check.

eiffel-fl · 2024-05-15T13:47:59Z

pkg/gadget-service/service-oci.go

+		if ok {
+			subject := tlsInfo.State.VerifiedChains[0][0].Subject
+			s.logger.Infof("[%s] RunGadget(%q)", subject, ociRequest.ImageName)
+		}


Same.
By the way, the code can be factorized.

mauriciovasquezbernal

It's looking very good. Some initial comments.

I think there is missing a openssl ecparam -name prime256v1 -genkey -noout -out server.key step in the description.

mauriciovasquezbernal · 2024-05-16T14:52:40Z

pkg/runtime/grpc/grpc-runtime.go

+		{
+			Key:          ParamTLSKey,
+			Description:  "TLS client key",
+			DefaultValue: "",


nit: remove? (same in other places)

mauriciovasquezbernal · 2024-05-16T14:59:19Z

cmd/ig/daemon.go

+			}
+
+			if ok := ca.AppendCertsFromPEM(caBytes); !ok {
+				return errors.New("failed to parse client CA certificate")


parsing ...?

(Not sure why semgrep doesn't complain about it)

I wondered the same: it only complains when wrapping errors - this is the last message in the chain, so it's fine. (e.g. "doing this: doing that: doing another thing: failed to blah")

Hmm, just ran into a similar situation on another PR without wrapping and it complained. 🤷

mauriciovasquezbernal · 2024-05-16T15:03:45Z

cmd/ig/daemon.go

+	daemonCmd.PersistentFlags().StringVarP(
+		&serverKey,
+		"tls-key",
+		"",
+		"",
+		"Path to tls key file")
+
+	daemonCmd.PersistentFlags().StringVarP(
+		&serverCert,
+		"tls-cert",
+		"",
+		"",
+		"Path to tls cert file")
+
+	daemonCmd.PersistentFlags().StringVarP(
+		&clientCA,
+		"tls-client-ca",
+		"",
+		"",
+		"Path to CA certificate for client validation")
+


Use StringVar to avoid passing an extra empty string.

mauriciovasquezbernal · 2024-05-16T15:06:29Z

cmd/ig/daemon.go

+
+		var options []grpc.ServerOption
+
+		if serverKey != "" || serverCert != "" || clientCA != "" {


I think we should we add a more specific validation logic to fail if "one but not all" flags are defined instead of relying on the functions below failing. It'll also print a better error message to the user.

mauriciovasquezbernal · 2024-05-16T15:10:32Z

pkg/runtime/grpc/grpc-runtime.go

+		grpc.WithReturnConnectionError(),
+	}
+
+	if tlsKey, tlsCert, tlsCA := r.globalParams.Get(ParamTLSKey).String(), r.globalParams.Get(ParamTLSCert).String(), r.globalParams.Get(ParamTLSServerCA).String(); tlsKey != "" || tlsCert != "" || tlsCA != "" {


I think this sets the new longest line record.

So where is my trophy, then? 😆

mauriciovasquezbernal · 2024-05-16T15:22:27Z

cmd/ig/daemon.go

+
+			options = append(options, grpc.Creds(credentials.NewTLS(tlsConfig)))
+		} else {
+			log.Warnf("no TLS configuration provided, running in insecure mode")


Do we need to print this message even when unix sockets are used? I'd argue it's not needed as in that case the client needs access to the unix socket file to contact the server.

mauriciovasquezbernal · 2024-05-16T15:33:25Z

pkg/runtime/grpc/grpc-runtime.go

+			return nil, errors.New("failed to parse client CA certificate")
+		}
+
+		purl, _ := url.Parse(target.addressOrPod)


I think we need to check the return code. Running the following makes it explode:

$ gadgetctl run trace_open:latest --remote-address tcp://127.0.0.1:1234 --tls-cert client1.crt --tls-key client1.key --tls-server-ca ca.crt --tls-server-name local INFO[0000] Experimental features enabled panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x2c373d0] goroutine 1 [running]: net/url.(*URL).Hostname(...) /usr/local/go/src/net/url/url.go:1151 github.com/inspektor-gadget/inspektor-gadget/pkg/runtime/grpc.(*Runtime).dialContext(0xc0002a2b70, {0x4018548, 0xc000276b90}, {{0x7ffc9f23228b?, 0xc0006c3850?}, {0x7ffc9f23228b?, 0x14a1f9b?}}, 0x12a05f200) /home/mauriciov/kinvolk/ebpf/inspektor-gadget/pkg/runtime/grpc/grpc-runtime.go:422 +0x550 github.com/inspektor-gadget/inspektor-gadget/pkg/runtime/grpc.(*Runtime).getConnToRandomTarget(0xc0002a2b70, {0x4018548, 0xc000276b90}, 0x17?) /home/mauriciov/kinvolk/ebpf/inspektor-gadget/pkg/runtime/grpc/grpc-runtime.go:347 +0x216 github.com/inspektor-gadget/inspektor-gadget/pkg/runtime/grpc.(*Runtime).GetGadgetInfo(0xc0002a2b70, {0x4065ce0, 0xc0005fd500}, 0xd?, 0xc0005618c0) /home/mauriciov/kinvolk/ebpf/inspektor-gadget/pkg/runtime/grpc/oci.go:39 +0x17d github.com/inspektor-gadget/inspektor-gadget/cmd/common.NewRunCommand.func1(0xc0004f4f08, {0xc00045fd90, 0xb, 0xb}) /home/mauriciov/kinvolk/ebpf/inspektor-gadget/cmd/common/oci.go:107 +0x4e3 github.com/spf13/cobra.(*Command).execute(0xc0004f4f08, {0xc00045fd90, 0xb, 0xb}) /home/mauriciov/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:968 +0xa44 github.com/spf13/cobra.(*Command).ExecuteC(0xc000160f08) /home/mauriciov/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1115 +0x3ff github.com/spf13/cobra.(*Command).Execute(...) /home/mauriciov/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1039 main.main() /home/mauriciov/kinvolk/ebpf/inspektor-gadget/cmd/gadgetctl/main.go:92 +0x745 $ ``

Hmm yeah, I should definitely add that - it shouldn't fail for the given address, nevertheless.

mauriciovasquezbernal · 2024-05-16T15:35:16Z

pkg/runtime/grpc/grpc-runtime.go

+		{
+			Key:          ParamTLSKey,
+			Description:  "TLS client key",
+			DefaultValue: "",
+			TypeHint:     params.TypeString,
+		},
+		{
+			Key:          ParamTLSCert,
+			Description:  "TLS client certificate",
+			DefaultValue: "",
+			TypeHint:     params.TypeString,
+		},
+		{
+			Key:          ParamTLSServerCA,
+			Description:  "TLS server CA certificate",
+			DefaultValue: "",
+			TypeHint:     params.TypeString,
+		},
+		{
+			Key:          ParamTLSServerName,
+			Description:  "override TLS server name (if omitted, using target server name)",
+			DefaultValue: "",
+			TypeHint:     params.TypeString,
+		},


Shouldn't they be moved under case ConnectionModeDirect to avoid showing them in the kubernetes case?

At least until we also support it for ig-k8s, yes.

flyth added 2 commits May 15, 2024 01:48

ig/daemon: add support for TLS

fa77d8c

Signed-off-by: Michael Friese <mfriese@microsoft.com>

runtime/grpc: add support for TLS

5f89be4

Signed-off-by: Michael Friese <mfriese@microsoft.com>

flyth requested review from mauriciovasquezbernal and blanquicet as code owners May 15, 2024 00:12

gadget-service: report credentials from TLS on gRPC requests

3998aa7

Signed-off-by: Michael Friese <mfriese@microsoft.com>

flyth force-pushed the michael/grpc-secure branch from a6d62ec to 3998aa7 Compare May 15, 2024 00:19

eiffel-fl reviewed May 15, 2024

View reviewed changes

mauriciovasquezbernal reviewed May 16, 2024

View reviewed changes

flyth mentioned this pull request May 28, 2024

add otel-metrics operator for image-based gadgets #2900

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ig/gadgetctl: add TLS support #2848

ig/gadgetctl: add TLS support #2848

flyth commented May 15, 2024 •

edited

eiffel-fl left a comment

eiffel-fl May 15, 2024

eiffel-fl May 15, 2024

eiffel-fl May 15, 2024

flyth May 15, 2024 •

edited

eiffel-fl May 16, 2024

flyth May 16, 2024

eiffel-fl May 16, 2024

eiffel-fl May 15, 2024 •

edited

flyth May 15, 2024

eiffel-fl May 16, 2024

flyth May 16, 2024

eiffel-fl May 15, 2024

mauriciovasquezbernal left a comment

mauriciovasquezbernal May 16, 2024

mauriciovasquezbernal May 16, 2024

flyth May 16, 2024

flyth May 16, 2024

mauriciovasquezbernal May 16, 2024

mauriciovasquezbernal May 16, 2024

mauriciovasquezbernal May 16, 2024

flyth May 16, 2024

mauriciovasquezbernal May 16, 2024

mauriciovasquezbernal May 16, 2024

flyth May 16, 2024

mauriciovasquezbernal May 16, 2024

flyth May 16, 2024

	log.Warnf("no TLS configuration provided, running in insecure mode")
	log.Warnf("no TLS configuration provided, communication between daemon and CLI will not be ciphered")


		var options []grpc.ServerOption

		if serverKey != "" \|\| serverCert != "" \|\| clientCA != "" {

ig/gadgetctl: add TLS support #2848

Are you sure you want to change the base?

ig/gadgetctl: add TLS support #2848

Conversation

flyth commented May 15, 2024 • edited

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyth May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyth commented May 15, 2024 •

edited

flyth May 15, 2024 •

edited

eiffel-fl May 15, 2024 •

edited