Add support for TLS #692

bipuladh · 2024-10-28T05:15:30Z

Changes

Adds a requirement for TLS cert for Sidecar Pods
Adds a auth header check for bearer tokens on the Sidecar Pods
Enables TLS by default on Controller Manager client pool
Adds a bearer token to the manager client request headers(Taken from corresponding SA)

Requirements for the above features to work correctly:

TokenReview create access for all sidecar containers
TLS certs are to be created and mounted via either Service(using annotations) or other mechanism on each of the server pods.

Images for testing:

Sidecar: quay.io/badhikar/k8s-sidecar:v1
Controller: quay.io/badhikar/k8s-controller:v1

Test logs

2024-11-11T18:12:00.948Z INFO Successfully connected to sidecar {"controller": "csiaddonsnode", "controllerGroup": "csiaddons.openshift.io", "controllerKind": "CSIAddonsNode", "CSIAddonsNode": {"name":"csi-rbdplugin-gbr85","namespace":"openshift-storage"}, "namespace": "openshift-storage", "name": "csi-rbdplugin-gbr85", "reconcileID": "964eea1e-324b-4957-984d-2611972645ed", "NodeID": "ip-10-0-50-177.us-east-2.compute.internal", "DriverName": "openshift-storage.rbd.csi.ceph.com", "EndPoint": "10.0.50.177:9070"}

nixpanic

left a few minor comments for improving, looks pretty good to me already. Thanks!

internal/kubernetes/namespace.go

tools/go.mod

internal/kubernetes/token/grpc.go

bipuladh · 2024-11-11T13:24:47Z

@nixpanic should we disable it by default for easy opt-in for users?

sidecar/main.go

config/manager/manager.yaml

nixpanic · 2024-11-11T13:50:37Z

@nixpanic should we disable it by default for easy opt-in for users?

If it works on current Kubernetes installations by default, it can be enabled by default IMHO. You may want to add a note in https://github.com/csi-addons/kubernetes-csi-addons/blob/main/docs/deploy-controller.md on the requirements of Kubernetes services, and how to disable it in case those services are unavailable.

bipuladh · 2024-11-11T19:06:11Z

@Madhu-1 my PR fails until I revert code introduced at https://github.com/csi-addons/kubernetes-csi-addons/pull/695/files#diff-03e0410a78ce106bc8ee10f594661c87995994955bfb6b07972d8fde2ec8a555R125-R146
Could you PTAL?

Madhu-1 · 2024-11-12T07:32:34Z

@Madhu-1 my PR fails until I revert code introduced at https://github.com/csi-addons/kubernetes-csi-addons/pull/695/files#diff-03e0410a78ce106bc8ee10f594661c87995994955bfb6b07972d8fde2ec8a555R125-R146 Could you PTAL?

@bipuladh please use latest yamls from master branch in Rook

Madhu-1

Please add a documentation about this feature and as its enabled by default i assume upgraded works as it is (in kubernetes)

Madhu-1 · 2024-11-12T07:34:48Z

internal/connection/connection.go

+		}
+		tlsConfig := &tls.Config{
+			RootCAs:            caCertPool, // The CA certificates to verify the server
+			InsecureSkipVerify: true,


dont we need to set it to true?

By default setting it to false but making it configurable by users from args

After having discussions we need the ability to configure this to support certificates. We currently resolve the IP address and use that instead of host name. This causes certificate mismatch.

internal/connection/connection.go

Madhu-1 · 2024-11-12T07:47:03Z

internal/kubernetes/token/grpc.go

+		return status.Errorf(codes.Unauthenticated, "missing metadata")
+	}
+
+	authHeader, ok := md["authorization"]
+	if !ok || len(authHeader) == 0 {
+		return status.Errorf(codes.Unauthenticated, "missing authorization token")
+	}
+
+	token := authHeader[0]
+	isValidated, err := validateBearerToken(ctx, token, kubeclient)
+	if !isValidated || (err != nil) {
+		return status.Errorf(codes.Unauthenticated, "invalid token")


i think you can use status.Error instead of status.Errorf

return the err message as well for better debugging

Madhu-1 · 2024-11-12T07:47:49Z

internal/kubernetes/token/grpc.go

+	file, err := os.Open(tokenPath)
+	if err != nil {
+		return "", err
+	}
+	defer file.Close()
+
+	data, err := io.ReadAll(file)
+	if err != nil {
+		return "", err
+	}
+	return string(data), nil


make use of readfile function which is doing the same thing?

ioutil readFile is deprecated.

i meant readFile we have below in this file

Madhu-1 · 2024-11-12T07:49:19Z

sidecar/internal/server/server.go

+		ss.server = grpc.NewServer(grpc.UnaryInterceptor(token.AuthorizationInterceptor(ss.client)), grpc.Creds(creds))
+	}
+	if !ss.enableTLS {
+		ss.server = grpc.NewServer(grpc.UnaryInterceptor(token.AuthorizationInterceptor(ss.client)))


if tls is not enabled why we need to have add UnaryInterceptor here?

I think this is something we need to decide. Should we disable auth token verification if TLS is disabled?

Removing Interceptor when TLS not used.

cmd/manager/main.go

iPraveenParihar · 2024-11-12T07:39:34Z

internal/connection/connection.go

+		}
+		tlsConfig := &tls.Config{
+			RootCAs:            caCertPool, // The CA certificates to verify the server
+			InsecureSkipVerify: true,


Shouldn't it be set to false?

By default setting it to false but making it configurable by users from args

internal/kubernetes/token/grpc.go

iPraveenParihar · 2024-11-12T07:44:40Z

internal/kubernetes/token/grpc.go

+	return nil
+}
+
+func removeBearer(authHeader string) string {


Suggested change

func removeBearer(authHeader string) string {

func extractToken(authHeader string) string {

since the function just extracts the token, this naming is more suitable?

sidecar/internal/server/server.go

sidecar/main.go

bipuladh · 2024-11-12T07:58:57Z

Please add a documentation about this feature and as its enabled by default i assume upgraded works as it is (in kubernetes)

@Madhu-1 @nixpanic for a smoother upgrade experience, should we disable by default? As mentioned in the PR description the sidecar deployer will have to create a certificate and mount it if not done the sidecar would go to CLBO. I can add this setup procedure on the readme files for reference.
I was thinking in the absence of certs we could automatically disable TLS but this could end up becoming a debugging nightmare. Perhaps logging could help, not sure.

nixpanic · 2024-11-12T08:02:24Z

@Madhu-1 @nixpanic for a smoother upgrade experience, should we disable by default? As mentioned in the PR description the sidecar deployer will have to create a certificate and mount it if not done the sidecar would go to CLBO. I can add this setup procedure on the readme files for reference. I was thinking in the absence of certs we could automatically disable TLS but this could end up becoming a debugging nightmare. Perhaps logging could help, not sure.

Yes, users should not need to do anything extra for upgrading. If there are manual steps to configure certificates, disable the feature by default.

nixpanic

Looks good to me. Please squash all the commits together so that the Mergify automation can merge it when all approvals are in.

nixpanic · 2024-11-12T17:13:53Z

internal/kubernetes/token/grpc.go

 	}

 	token := authHeader[0]
 	isValidated, err := validateBearerToken(ctx, token, kubeclient)
 	if !isValidated || (err != nil) {
-		return status.Errorf(codes.Unauthenticated, "invalid token")
+		return status.Error(codes.Unauthenticated, fmt.Sprint("invalid token: %w", err))


this is where status.Errorf() can be used so that fmt.Sprintf() is not needed.

Can you please address this as well?

iPraveenParihar · 2024-11-13T07:50:39Z

cmd/manager/main.go

@@ -92,6 +94,8 @@ func main() {
 	flag.BoolVar(&enableAdmissionWebhooks, "enable-admission-webhooks", false, "[DEPRECATED] Enable the admission webhooks")
 	flag.BoolVar(&showVersion, "version", false, "Print Version details")
 	flag.StringVar(&cfg.SchedulePrecedence, "schedule-precedence", "", "The order of precedence in which schedule of reclaimspace and keyrotation is considered. Possible values are sc-only")
+	flag.BoolVar(&enableTLS, "enable-tls", false, "Enable TLS(enabled by default)")
+	flag.BoolVar(&skipInsecureVerify, "insecure-skip-tls-verify", false, "skip the certificate check")


Suggested change

flag.BoolVar(&skipInsecureVerify, "insecure-skip-tls-verify", false, "skip the certificate check")

flag.BoolVar(&skipInsecureVerify, "insecure-skip-tls-verify", false, "skip server certificate verification")

Madhu-1

Please add/update the documentation on how to enable this functionality

Madhu-1 · 2024-11-13T08:21:32Z

cmd/manager/main.go

@@ -92,6 +94,8 @@ func main() {
 	flag.BoolVar(&enableAdmissionWebhooks, "enable-admission-webhooks", false, "[DEPRECATED] Enable the admission webhooks")
 	flag.BoolVar(&showVersion, "version", false, "Print Version details")
 	flag.StringVar(&cfg.SchedulePrecedence, "schedule-precedence", "", "The order of precedence in which schedule of reclaimspace and keyrotation is considered. Possible values are sc-only")
+	flag.BoolVar(&enableTLS, "enable-tls", false, "Enable TLS(enabled by default)")


need to update the comment as its disabled by default

internal/connection/connection.go

Madhu-1 · 2024-11-13T08:23:11Z

internal/connection/connection.go

 		grpc.WithIdleTimeout(time.Duration(0)),
 	}
+
+	opts = append(opts, token.WithServiceAccountToken())


Do we need to move this inside the if check when TLS is enabled?

Madhu-1 · 2024-11-13T08:23:29Z

internal/connection/connection.go

+
+		caFile, caError := token.GetCACert()
+		if caError != nil {
+			return nil, (fmt.Errorf("failed to get server cert %w", caError))


Suggested change

return nil, (fmt.Errorf("failed to get server cert %w", caError))

return nil, fmt.Errorf("failed to get server cert %w", caError)

Madhu-1 · 2024-11-13T08:24:42Z

internal/kubernetes/token/grpc.go

+	file, err := os.Open(tokenPath)
+	if err != nil {
+		return "", err
+	}
+	defer file.Close()
+
+	data, err := io.ReadAll(file)
+	if err != nil {
+		return "", err
+	}
+	return string(data), nil


i meant readFile we have below in this file

Madhu-1 · 2024-11-13T08:26:32Z

internal/kubernetes/token/grpc.go

+func parseToken(authHeader string) string {
+	if strings.HasPrefix(authHeader, bearerPrefix) {
+		return strings.TrimPrefix(authHeader, bearerPrefix)
+	}
+	return authHeader
+}


If the goal is to just to parse, we can remove the if check and trim it

func parseToken(authHeader string) string { return strings.TrimPrefix(authHeader, bearerPrefix) }

Madhu-1 · 2024-11-13T08:27:11Z

internal/kubernetes/token/grpc.go

+	result, err := kubeclient.AuthenticationV1().TokenReviews().Create(ctx, tokenReview, metav1.CreateOptions{})
+	if err != nil {
+		return false, fmt.Errorf("failed to review token %w", err)
+	}


Dont we need RBAC for this one? am not sure we have this RBAC already

Yes we require. Whoever deploys the sidecar should be responsible to have this.

Madhu-1 · 2024-11-13T08:35:54Z

sidecar/internal/server/server.go

-	// create the gRPC server and register services
-	ss.server = grpc.NewServer()
+	if ss.enableTLS {
+		creds, err := credentials.NewServerTLSFromFile("/etc/tls/tls.crt", "/etc/tls/tls.key")


"/etc/tls/tls.crt", "/etc/tls/tls.key" Are we going to have these two files in all the pods in same location when TLS is enabled?

Ideally yes. But I guess we can make it configurable later on if that is preferred.

adding these as parameters

Madhu-1 · 2024-11-13T08:45:19Z

sidecar/internal/server/server.go

+	if ss.enableTLS {
+		creds, err := credentials.NewServerTLSFromFile("/etc/tls/tls.crt", "/etc/tls/tls.key")
+		if err != nil {
+			klog.Fatalf("Could not find TLS file: %v", err)


Suggested change

klog.Fatalf("Could not find TLS file: %v", err)

klog.Fatalf("failed to load TLS certificate and key: %v", err)

nixpanic

Looks pretty complete to me, thanks for the contribution!

Pull request has been modified.

Madhu-1

some nits, overall LGTM

docs/deploy-controller.md

Madhu-1 · 2024-11-13T13:34:22Z

internal/connection/connection.go

 	opts := []grpc.DialOption{
-		grpc.WithTransportCredentials(insecure.NewCredentials()),


Dont we need to set this incase enableTLS is set to false?

yes. thanks!

@bipuladh wont this be a problem when enableTLS is set we are appending DialOptions with two grpc.WithTransportCredentials? one with insecure and another one with creds?

Madhu-1 · 2024-11-13T13:35:17Z

internal/controller/csiaddons/csiaddonsnode_controller.go

@@ -211,6 +213,10 @@ func (r *CSIAddonsNodeReconciler) resolveEndpoint(ctx context.Context, rawURL st
 	if err != nil {
 		return "", "", err
 	}
+	if r.EnableTLS {
+		// We need to use this name to accept certificates signed for pods
+		return podname, podname + "." + namespace + ".pod" + ":" + port, nil


Is this going to work when we are using host networking?

Removing this.

internal/kubernetes/token/grpc.go

Madhu-1 · 2024-11-13T13:36:51Z

internal/kubernetes/token/grpc.go

 	}

 	token := authHeader[0]
 	isValidated, err := validateBearerToken(ctx, token, kubeclient)
 	if !isValidated || (err != nil) {
-		return status.Errorf(codes.Unauthenticated, "invalid token")
+		return status.Error(codes.Unauthenticated, fmt.Sprint("invalid token: %w", err))


Can you please address this as well?

internal/kubernetes/token/grpc.go

Madhu-1 · 2024-11-13T13:37:51Z

sidecar/internal/server/server.go

 	"net"

+	"github.com/csi-addons/kubernetes-csi-addons/internal/kubernetes/token"
 	"google.golang.org/grpc"


add a new line for separation of imports

sidecar/internal/server/server.go

Madhu-1 · 2024-11-13T14:11:54Z

@bipuladh i tested this PR on the existing cluster without enabling TLS. its not working, can you please ensure that the functionality works with/without TLS?

bipuladh · 2024-11-14T10:11:46Z

@bipuladh i tested this PR on the existing cluster without enabling TLS. its not working, can you please ensure that the functionality works with/without TLS?

Tested with the newly updated code. Working fine now.
Regarding host networking I think instead of relying on Pod IP we might have to create a service on front of it (the operators that deploy sidecar) so that we can have a DNS mapping. These service names can be put by the operator that deploys on the Pods in their annotations. We can then get the Service DNS key by querying it. This way we could update the code for TLS part to look into that and document it. WDYT?

bipuladh · 2024-11-19T09:55:28Z

/hold
ceph/ceph-csi-operator#173
Waiting on the decision of using Service over IP.
And also how the name of the service is to be communicated to the Server.

Signed-off-by: Bipul Adhikari <[email protected]>

nixpanic · 2024-12-12T07:59:27Z

internal/kubernetes/token/grpc.go

+}
+
+func GetCACert() (string, error) {
+	return readFile("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt")


This path is something that should be configurable. Users may want to provide their own certificates for the sidecar, and those need to be verified with a CA cert that trusts the authority that was used to sign the sidecar certificates.

mergify bot requested review from black-dragon74, iPraveenParihar, Madhu-1, nixpanic, Rakshith-R and yati1998 October 28, 2024 05:16

bipuladh force-pushed the tls branch 2 times, most recently from b873181 to b0c9bcd Compare October 28, 2024 11:49

nixpanic reviewed Nov 5, 2024

View reviewed changes

internal/kubernetes/namespace.go Outdated Show resolved Hide resolved

tools/go.mod Outdated Show resolved Hide resolved

internal/kubernetes/token/grpc.go Outdated Show resolved Hide resolved

internal/kubernetes/token/grpc.go Outdated Show resolved Hide resolved

bipuladh force-pushed the tls branch from a11d381 to 16a68cd Compare November 11, 2024 13:06

bipuladh changed the title ~~[WIP] Add support for TLS~~ Add support for TLS Nov 11, 2024

bipuladh requested a review from nixpanic November 11, 2024 13:15

bipuladh force-pushed the tls branch from 16a68cd to a23925b Compare November 11, 2024 13:22

nixpanic reviewed Nov 11, 2024

View reviewed changes

sidecar/main.go Outdated Show resolved Hide resolved

nixpanic reviewed Nov 11, 2024

View reviewed changes

config/manager/manager.yaml Outdated Show resolved Hide resolved

bipuladh requested a review from nixpanic November 11, 2024 18:21

Madhu-1 reviewed Nov 12, 2024

View reviewed changes

iPraveenParihar reviewed Nov 12, 2024

View reviewed changes

nixpanic requested changes Nov 12, 2024

View reviewed changes

iPraveenParihar reviewed Nov 13, 2024

View reviewed changes

Madhu-1 reviewed Nov 13, 2024

View reviewed changes

bipuladh mentioned this pull request Nov 13, 2024

csiaddons: add support for TLS ceph/ceph-csi-operator#172

Open

6 tasks

bipuladh force-pushed the tls branch from ce7da22 to 9ee2d7a Compare November 13, 2024 11:35

mergify bot added the api Change to the API, requires extra care label Nov 13, 2024

mergify bot requested a review from ShyamsundarR November 13, 2024 11:36

bipuladh force-pushed the tls branch 2 times, most recently from 90f849e to 157b439 Compare November 13, 2024 11:44

nixpanic previously approved these changes Nov 13, 2024

View reviewed changes

bipuladh force-pushed the tls branch from 157b439 to 38c0bf0 Compare November 13, 2024 12:37

bipuladh requested review from nixpanic, Madhu-1 and iPraveenParihar November 13, 2024 13:07

nixpanic previously approved these changes Nov 13, 2024

View reviewed changes

bipuladh force-pushed the tls branch from 38c0bf0 to 002d7c6 Compare November 13, 2024 13:31

Madhu-1 requested changes Nov 13, 2024

View reviewed changes

bipuladh force-pushed the tls branch 2 times, most recently from bdc2e10 to 86bd3b3 Compare November 14, 2024 07:07

nixpanic requested a review from Madhu-1 November 14, 2024 17:08

bipuladh mentioned this pull request Nov 15, 2024

docs: add design for adding new arguments for TLS ceph/ceph-csi-operator#173

Open

6 tasks

Adds support for TLS encryption between manager and addons

7c57f05

Signed-off-by: Bipul Adhikari <[email protected]>

bipuladh force-pushed the tls branch from 86bd3b3 to 7c57f05 Compare December 6, 2024 06:02

nixpanic reviewed Dec 12, 2024

View reviewed changes

	func removeBearer(authHeader string) string {
	func extractToken(authHeader string) string {

	flag.BoolVar(&skipInsecureVerify, "insecure-skip-tls-verify", false, "skip the certificate check")
	flag.BoolVar(&skipInsecureVerify, "insecure-skip-tls-verify", false, "skip server certificate verification")

	return nil, (fmt.Errorf("failed to get server cert %w", caError))
	return nil, fmt.Errorf("failed to get server cert %w", caError)

	klog.Fatalf("Could not find TLS file: %v", err)
	klog.Fatalf("failed to load TLS certificate and key: %v", err)

		opts := []grpc.DialOption{
		grpc.WithTransportCredentials(insecure.NewCredentials()),

Add support for TLS #692

Are you sure you want to change the base?

Add support for TLS #692

Conversation

bipuladh commented Oct 28, 2024 • edited Loading

Changes

Requirements for the above features to work correctly:

Images for testing:

Test logs

nixpanic left a comment

Choose a reason for hiding this comment

bipuladh commented Nov 11, 2024

nixpanic commented Nov 11, 2024

bipuladh commented Nov 11, 2024

Madhu-1 commented Nov 12, 2024

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh Nov 12, 2024 • edited Loading

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

bipuladh commented Nov 12, 2024

nixpanic commented Nov 12, 2024

nixpanic 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

Madhu-1 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

nixpanic left a comment

Choose a reason for hiding this comment

Madhu-1 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

Madhu-1 commented Nov 13, 2024

bipuladh commented Nov 14, 2024

bipuladh commented Nov 19, 2024

Choose a reason for hiding this comment

bipuladh commented Oct 28, 2024 •

edited

Loading

bipuladh Nov 12, 2024 •

edited

Loading