Add support for enhanced security by bipuladh · Pull Request #692 · csi-addons/kubernetes-csi-addons

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!

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

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

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

+		}
+		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.

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

+		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

+	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

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

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

+		}
+		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

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

+	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?

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

 	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

 	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

 	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

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

 		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

+
+		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

+	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

+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

+	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

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

+	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-12-19T16:50:10Z

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

nixpanic

LGTM, many thanks!

Pull request has been modified.

Rakshith-R

one small nit,
rest looks good to me.

Signed-off-by: Bipul Adhikari <badhikar@redhat.com>

Rakshith-R

Thanks

Rakshith-R · 2024-12-20T12:28:23Z

@Mergifyio queue

mergify · 2024-12-20T12:28:28Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at ae2ede2

requested changes are addressed

Rakshith-R · 2024-12-20T12:32:00Z

@Mergifyio refresh

mergify · 2024-12-20T12:32:04Z

refresh

✅ Pull request refreshed

mergify Bot requested review from Madhu-1, Rakshith-R, black-dragon74, iPraveenParihar, nixpanic 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

Comment thread internal/kubernetes/namespace.go Outdated

Comment thread tools/go.mod Outdated

Comment thread internal/kubernetes/token/grpc.go Outdated

Comment thread internal/kubernetes/token/grpc.go Outdated

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

Comment thread sidecar/main.go Outdated

nixpanic reviewed Nov 11, 2024

View reviewed changes

Comment thread config/manager/manager.yaml Outdated

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

Closed

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

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

Comment thread internal/kubernetes/token/grpc.go Outdated

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

Comment thread internal/kubernetes/token/grpc.go Outdated

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

Comment thread internal/kubernetes/token/grpc.go Outdated

Rakshith-R reviewed Dec 20, 2024

View reviewed changes

Comment thread sidecar/internal/server/server.go Outdated

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

Comment thread internal/kubernetes/token/grpc.go Outdated

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

Comment thread internal/kubernetes/token/grpc.go Outdated

Adds support for TLS encryption between manager and addons

80273e8

Signed-off-by: Bipul Adhikari <badhikar@redhat.com>

bipuladh force-pushed the tls branch from 087042d to 80273e8 Compare December 20, 2024 10:33

Rakshith-R approved these changes Dec 20, 2024

View reviewed changes

nixpanic approved these changes Dec 20, 2024

View reviewed changes

mergify Bot merged commit ae2ede2 into csi-addons:main Dec 20, 2024

bipuladh mentioned this pull request Jan 15, 2025

Add TokenReview RBAC to support CSI addons security enhancements ceph/ceph-csi-operator#197

Merged

6 tasks

nixpanic mentioned this pull request Jan 20, 2025

TLS handshake failure in csi-addons-controller-manager #750

Closed

bipuladh mentioned this pull request Jan 27, 2025

csi: updates RBAC allow tokenreview creation rook/rook#15326

Merged

6 tasks

bipuladh mentioned this pull request Mar 17, 2025

Fix RBAC permissions for CSI addons security enhancements ceph/ceph-csi-operator#215

Merged

6 tasks

	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)

Conversation

bipuladh commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Requirements for the above features to work correctly:

Images for testing:

Test logs

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bipuladh commented Nov 11, 2024

Uh oh!

Uh oh!

Uh oh!

nixpanic commented Nov 11, 2024

Uh oh!

bipuladh commented Nov 11, 2024

Uh oh!

Madhu-1 commented Nov 12, 2024

Uh oh!

Madhu-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bipuladh Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bipuladh commented Nov 12, 2024

Uh oh!

nixpanic commented Nov 12, 2024

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Madhu-1 left a comment

Choose a reason for hiding this comment

Uh oh!

bipuladh commented Oct 28, 2024 •

edited

Loading

bipuladh Nov 12, 2024 •

edited

Loading