Add support for enhanced security #692

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

Changes

Adds a auth header check for bearer tokens on the Sidecar Pods
Generates self signed certs on the sidecar pods for SSL security.
Adds a bearer token to the request headers on the manager.(Taken from corresponding SA)
Introduces a new arg in both manager and sidecar named enable-auth(disabled by default. If enabled it will start checking for auth headers in Request objects and enables TLS for grpc connections.

Requirements for the above features to work correctly:

TokenReview create and get access for all sidecar containers

.

Images for testing:

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

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)

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.

internal/kubernetes/token/grpc.go

nixpanic · 2024-12-17T11:37:43Z

/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.

Hi @bipuladh , is this still blocking the PR?

nixpanic · 2024-12-17T15:51:39Z

Aaah! A there is a conflict now 😭

bipuladh · 2024-12-18T07:10:08Z

/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.

Hi @bipuladh , is this still blocking the PR?

Not anymore.

bipuladh · 2024-12-19T16:50:10Z

@nixpanic @Madhu-1 tested with both enabled and disabled mode. Works fine.

internal/kubernetes/token/grpc.go

sidecar/internal/server/server.go

nixpanic

LGTM, many thanks!

Pull request has been modified.

internal/kubernetes/token/grpc.go

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

Pull request has been modified.

Rakshith-R

one small nit,
rest looks good to me.

Rakshith-R · 2024-12-20T10:23:49Z

internal/kubernetes/token/grpc.go

+		return err
+	}
+
+	authCtx := metadata.AppendToOutgoingContext(ctx, authorizationKey, "Bearer "+token)


Suggested change

authCtx := metadata.AppendToOutgoingContext(ctx, authorizationKey, "Bearer "+token)

authCtx := metadata.AppendToOutgoingContext(ctx, authorizationKey, bearerPrefix +token)

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

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

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

nixpanic reviewed Dec 12, 2024

View reviewed changes

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

bipuladh force-pushed the tls branch 3 times, most recently from a48c6e7 to 0b0b082 Compare December 19, 2024 16:46

bipuladh requested a review from nixpanic December 19, 2024 16:49

bipuladh changed the title ~~Add support for TLS~~ Add support for enhanced security Dec 20, 2024

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

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

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

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

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

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

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

sidecar/internal/server/server.go Outdated Show resolved Hide resolved

bipuladh force-pushed the tls branch from 0b0b082 to b5dda50 Compare December 20, 2024 09:02

nixpanic previously approved these changes Dec 20, 2024

View reviewed changes

bipuladh force-pushed the tls branch from b5dda50 to dbf8b45 Compare December 20, 2024 09:59

bipuladh requested a review from Rakshith-R December 20, 2024 10:00

nixpanic previously approved these changes Dec 20, 2024

View reviewed changes

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

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

Adds support for TLS encryption between manager and addons

087042d

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

bipuladh force-pushed the tls branch from dbf8b45 to 087042d Compare December 20, 2024 10:16

Rakshith-R reviewed Dec 20, 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)

	authCtx := metadata.AppendToOutgoingContext(ctx, authorizationKey, "Bearer "+token)
	authCtx := metadata.AppendToOutgoingContext(ctx, authorizationKey, bearerPrefix +token)

Add support for enhanced security #692

Are you sure you want to change the base?

Add support for enhanced security #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

bipuladh commented Nov 14, 2024

bipuladh commented Nov 19, 2024

nixpanic commented Dec 17, 2024

nixpanic commented Dec 17, 2024

bipuladh commented Dec 18, 2024

bipuladh commented Dec 19, 2024

nixpanic left a comment

Choose a reason for hiding this comment

Rakshith-R left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh commented Oct 28, 2024 •

edited

Loading

bipuladh Nov 12, 2024 •

edited

Loading