From 06506ed95d5492cf48c95f8a0852ebbfc358c8e1 Mon Sep 17 00:00:00 2001
From: Diamon Wiggins <38189728+diamonwiggins@users.noreply.github.com>
Date: Thu, 7 Nov 2024 10:07:27 -0500
Subject: [PATCH] Fix remote host collection RBAC checks (#1672)

* fix remote host collection rbac checks

* move saveNodeList into collectRemoteHost function

* fix resource attribute list and retrieve namespace from kubeconfig

* revert change to set a default namespace from kubeconfig

* remove duplicate code
---
 pkg/supportbundle/collect.go       | 30 ++++++++++
 pkg/supportbundle/rbac.go          | 89 ++++++++++++++++++++++--------
 pkg/supportbundle/supportbundle.go | 21 -------
 3 files changed, 95 insertions(+), 45 deletions(-)

diff --git a/pkg/supportbundle/collect.go b/pkg/supportbundle/collect.go
index 87ff2fcfb..d77989398 100644
--- a/pkg/supportbundle/collect.go
+++ b/pkg/supportbundle/collect.go
@@ -213,6 +213,10 @@ func collectRemoteHost(ctx context.Context, collectSpecs []*troubleshootv1beta2.
 	opts.KubernetesRestConfig.Burst = constants.DEFAULT_CLIENT_BURST
 	opts.KubernetesRestConfig.UserAgent = fmt.Sprintf("%s/%s", constants.DEFAULT_CLIENT_USER_AGENT, version.Version())
 
+	if err := saveNodeList(opts, bundlePath); err != nil {
+		return err
+	}
+
 	// Run remote collectors sequentially
 	for _, spec := range collectSpecs {
 		collector, ok := collect.GetHostCollector(spec, bundlePath)
@@ -340,3 +344,29 @@ func getGlobalRedactors(additionalRedactors *troubleshootv1beta2.Redactor) []*tr
 	}
 	return []*troubleshootv1beta2.Redact{}
 }
+
+func saveNodeList(opts SupportBundleCreateOpts, bundlePath string) error {
+	result := make(collect.CollectorResult)
+
+	clientset, err := kubernetes.NewForConfig(opts.KubernetesRestConfig)
+	if err != nil {
+		return errors.Wrap(err, "failed to create kubernetes clientset to run host collectors in pod")
+	}
+
+	nodeList, err := getNodeList(clientset, opts)
+	if err != nil {
+		return errors.Wrap(err, "failed to get remote node list")
+	}
+
+	nodeListBytes, err := json.MarshalIndent(nodeList, "", "  ")
+	if err != nil {
+		return errors.Wrap(err, "failed to marshal remote node list")
+	}
+
+	err = result.SaveResult(bundlePath, constants.NODE_LIST_FILE, bytes.NewBuffer(nodeListBytes))
+	if err != nil {
+		return errors.Wrap(err, "failed to write remote node list")
+	}
+
+	return nil
+}
diff --git a/pkg/supportbundle/rbac.go b/pkg/supportbundle/rbac.go
index 0ed58a9fa..a958226a3 100644
--- a/pkg/supportbundle/rbac.go
+++ b/pkg/supportbundle/rbac.go
@@ -34,34 +34,75 @@ func checkRemoteCollectorRBAC(ctx context.Context, clientConfig *rest.Config, ti
 
 	var forbidden []error
 
-	spec := authorizationv1.SelfSubjectAccessReviewSpec{
-		ResourceAttributes: &authorizationv1.ResourceAttributes{
-			Namespace:   namespace,
-			Verb:        "create,delete",
-			Group:       "",
-			Version:     "",
-			Resource:    "pods,configmap",
-			Subresource: "",
-			Name:        "",
+	resourceAttributesList := []authorizationv1.ResourceAttributes{
+		{
+			Namespace: namespace,
+			Verb:      "get",
+			Resource:  "pods",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "create",
+			Resource:  "pods",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "delete",
+			Resource:  "pods",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "get",
+			Resource:  "pods/log",
+		},
+		{
+			Verb:     "list",
+			Resource: "nodes",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "get",
+			Resource:  "configmaps",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "create",
+			Resource:  "configmaps",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "delete",
+			Resource:  "configmaps",
+		},
+		{
+			Namespace: namespace,
+			Verb:      "get",
+			Resource:  "serviceaccounts",
 		},
-		NonResourceAttributes: nil,
 	}
 
-	sar := &authorizationv1.SelfSubjectAccessReview{
-		Spec: spec,
-	}
-	resp, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, sar, metav1.CreateOptions{})
-	if err != nil {
-		return errors.Wrap(err, "failed to run subject review")
-	}
+	for _, resourceAttributes := range resourceAttributesList {
+		spec := authorizationv1.SelfSubjectAccessReviewSpec{
+			ResourceAttributes: &resourceAttributes,
+		}
+
+		sar := &authorizationv1.SelfSubjectAccessReview{
+			Spec: spec,
+		}
+
+		resp, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, sar, metav1.CreateOptions{})
+		if err != nil {
+			return errors.Wrap(err, "failed to run subject review")
+		}
 
-	if !resp.Status.Allowed {
-		forbidden = append(forbidden, collect.RBACError{
-			DisplayName: title,
-			Namespace:   spec.ResourceAttributes.Namespace,
-			Resource:    spec.ResourceAttributes.Resource,
-			Verb:        spec.ResourceAttributes.Verb,
-		})
+		if !resp.Status.Allowed {
+			forbidden = append(forbidden, collect.RBACError{
+				DisplayName: title,
+				Namespace:   spec.ResourceAttributes.Namespace,
+				Resource:    spec.ResourceAttributes.Resource,
+				Verb:        spec.ResourceAttributes.Verb,
+			})
+		}
 	}
 
 	if len(forbidden) > 0 {
diff --git a/pkg/supportbundle/supportbundle.go b/pkg/supportbundle/supportbundle.go
index d85742c45..31f28deed 100644
--- a/pkg/supportbundle/supportbundle.go
+++ b/pkg/supportbundle/supportbundle.go
@@ -3,7 +3,6 @@ package supportbundle
 import (
 	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"net/http"
 	"os"
@@ -118,26 +117,6 @@ func CollectSupportBundleFromSpec(
 		root.End()
 	}()
 
-	// only create a node list if we are running host collectors in a pod
-	if opts.RunHostCollectorsInPod {
-		clientset, err := kubernetes.NewForConfig(opts.KubernetesRestConfig)
-		if err != nil {
-			return nil, errors.Wrap(err, "failed to create kubernetes clientset to run host collectors in pod")
-		}
-		nodeList, err := getNodeList(clientset, opts)
-		if err != nil {
-			return nil, errors.Wrap(err, "failed to get remote node list")
-		}
-		nodeListBytes, err := json.MarshalIndent(nodeList, "", "  ")
-		if err != nil {
-			return nil, errors.Wrap(err, "failed to marshal remote node list")
-		}
-		err = result.SaveResult(bundlePath, constants.NODE_LIST_FILE, bytes.NewBuffer(nodeListBytes))
-		if err != nil {
-			return nil, errors.Wrap(err, "failed to write remote node list")
-		}
-	}
-
 	// Cache error returned by collectors and return it at the end of the function
 	// so as to have a chance to run analyzers and archive the support bundle after.
 	// If both host and in cluster collectors fail, the errors will be wrapped