cmd/containerboot: gracefully degrade if missing patch permissions in k8s.

Fixes #6629.

Signed-off-by: David Anderson <danderson@tailscale.com>

cmd/containerboot/kube.go

@@ -21,54 +21,82 @@
 	"path/filepath"
 	"strings"
 	"time"
+
+	"tailscale.com/util/multierr"
 )
 
-// checkSecretPermissions checks that the current pod has permission to read,
-// write and patch secretName in the cluster.
-func checkSecretPermissions(ctx context.Context, secretName string) error {
-	for _, verb := range []string{"get", "update", "patch"} {
-		sar := map[string]any{
-			"apiVersion": "authorization.k8s.io/v1",
-			"kind":       "SelfSubjectAccessReview",
-			"spec": map[string]any{
-				"resourceAttributes": map[string]any{
-					"namespace": kubeNamespace,
-					"verb":      verb,
-					"resource":  "secrets",
-					"name":      secretName,
-				},
-			},
-		}
-		bs, err := json.Marshal(sar)
+// checkSecretPermissions checks the secret access permissions of the current
+// pod. It returns an error if the basic permissions tailscale needs are
+// missing, and reports whether the patch permission is additionally present.
+//
+// Errors encountered during the access checking process are logged, but ignored
+// so that the pod tries to fail alive if the permissions exist and there's just
+// something wrong with SelfSubjectAccessReviews. There shouldn't be, pods
+// should always be able to use SSARs to assess their own permissions, but since
+// we didn't use to check permissions this way we'll be cautious in case some
+// old version of k8s deviates from the current behavior.
+func checkSecretPermissions(ctx context.Context, secretName string) (canPatch bool, err error) {
+	var errs []error
+	for _, verb := range []string{"get", "update"} {
+		ok, err := checkPermission(ctx, verb, secretName)
 		if err != nil {
-			return err
-		}
-		req, err := http.NewRequest("POST", "/apis/authorization.k8s.io/v1/selfsubjectaccessreviews", bytes.NewReader(bs))
-		if err != nil {
-			return err
-		}
-		resp, err := doKubeRequest(ctx, req)
-		if err != nil {
-			return err
-		}
-		defer resp.Body.Close()
-		bs, err = io.ReadAll(resp.Body)
-		if err != nil {
-			return err
-		}
-		var res struct {
-			Status struct {
-				Allowed bool `json:"allowed"`
-			} `json:"status"`
-		}
-		if err := json.Unmarshal(bs, &res); err != nil {
-			return err
-		}
-		if !res.Status.Allowed {
-			return fmt.Errorf("missing permission: cannot %s secret %q", verb, secretName)
+			log.Printf("error checking %s permission on secret %s: %v", verb, secretName, err)
+		} else if !ok {
+			errs = append(errs, fmt.Errorf("missing %s permission on secret %q", verb, secretName))
 		}
 	}
-	return nil
+	if len(errs) > 0 {
+		return false, multierr.New(errs...)
+	}
+	ok, err := checkPermission(ctx, "patch", secretName)
+	if err != nil {
+		log.Printf("error checking patch permission on secret %s: %v", secretName, err)
+		return false, nil
+	}
+	return ok, nil
+}
+
+// checkPermission reports whether the current pod has permission to use the
+// given verb (e.g. get, update, patch) on secretName.
+func checkPermission(ctx context.Context, verb, secretName string) (bool, error) {
+	sar := map[string]any{
+		"apiVersion": "authorization.k8s.io/v1",
+		"kind":       "SelfSubjectAccessReview",
+		"spec": map[string]any{
+			"resourceAttributes": map[string]any{
+				"namespace": kubeNamespace,
+				"verb":      verb,
+				"resource":  "secrets",
+				"name":      secretName,
+			},
+		},
+	}
+	bs, err := json.Marshal(sar)
+	if err != nil {
+		return false, err
+	}
+	req, err := http.NewRequest("POST", "/apis/authorization.k8s.io/v1/selfsubjectaccessreviews", bytes.NewReader(bs))
+	if err != nil {
+		return false, err
+	}
+	resp, err := doKubeRequest(ctx, req)
+	if err != nil {
+		return false, err
+	}
+	defer resp.Body.Close()
+	bs, err = io.ReadAll(resp.Body)
+	if err != nil {
+		return false, err
+	}
+	var res struct {
+		Status struct {
+			Allowed bool `json:"allowed"`
+		} `json:"status"`
+	}
+	if err := json.Unmarshal(bs, &res); err != nil {
+		return false, err
+	}
+	return res.Status.Allowed, nil
 }
 
 // findKeyInKubeSecret inspects the kube secret secretName for a data

cmd/containerboot/main.go

@@ -117,15 +117,28 @@ func main() {
 	defer cancel()
 
 	if cfg.InKubernetes && cfg.KubeSecret != "" {
-		if err := checkSecretPermissions(ctx, cfg.KubeSecret); err != nil {
+		canPatch, err := checkSecretPermissions(ctx, cfg.KubeSecret)
+		if err != nil {
 			log.Fatalf("Some Kubernetes permissions are missing, please check your RBAC configuration: %v", err)
 		}
+		cfg.KubernetesCanPatch = canPatch
+
 		if cfg.AuthKey == "" {
 			key, err := findKeyInKubeSecret(ctx, cfg.KubeSecret)
 			if err != nil {
 				log.Fatalf("Getting authkey from kube secret: %v", err)
 			}
 			if key != "" {
+				// This behavior of pulling authkeys from kube secrets was added
+				// at the same time as the patch permission, so we can enforce
+				// that we must be able to patch out the authkey after
+				// authenticating if you want to use this feature. This avoids
+				// us having to deal with the case where we might leave behind
+				// an unnecessary reusable authkey in a secret, like a rake in
+				// the grass.
+				if !cfg.KubernetesCanPatch {
+					log.Fatalf("authkey found in TS_KUBE_SECRET, but the pod doesn't have patch permissions on the secret to manage the authkey.")
+				}
 				log.Print("Using authkey found in kube secret")
 				cfg.AuthKey = key
 			} else {
@@ -149,7 +162,7 @@ func main() {
 			log.Fatalf("installing proxy rules: %v", err)
 		}
 	}
-	if cfg.InKubernetes && cfg.KubeSecret != "" {
+	if cfg.InKubernetes && cfg.KubernetesCanPatch && cfg.KubeSecret != "" {
 		if err := storeDeviceID(ctx, cfg.KubeSecret, string(st.Self.ID)); err != nil {
 			log.Fatalf("storing device ID in kube secret: %v", err)
 		}
@@ -446,21 +459,22 @@ func installIPTablesRule(ctx context.Context, dstStr string, tsIPs []netip.Addr)
 
 // settings is all the configuration for containerboot.
 type settings struct {
-	AuthKey         string
-	Routes          string
-	ProxyTo         string
-	DaemonExtraArgs string
-	ExtraArgs       string
-	InKubernetes    bool
-	UserspaceMode   bool
-	StateDir        string
-	AcceptDNS       bool
-	KubeSecret      string
-	SOCKSProxyAddr  string
-	HTTPProxyAddr   string
-	Socket          string
-	AuthOnce        bool
-	Root            string
+	AuthKey            string
+	Routes             string
+	ProxyTo            string
+	DaemonExtraArgs    string
+	ExtraArgs          string
+	InKubernetes       bool
+	UserspaceMode      bool
+	StateDir           string
+	AcceptDNS          bool
+	KubeSecret         string
+	SOCKSProxyAddr     string
+	HTTPProxyAddr      string
+	Socket             string
+	AuthOnce           bool
+	Root               string
+	KubernetesCanPatch bool
 }
 
 // defaultEnv returns the value of the given envvar name, or defVal if

cmd/containerboot/main_test.go

@@ -114,10 +114,11 @@ type phase struct {
 		WantFiles map[string]string
 	}
 	tests := []struct {
-		Name       string
-		Env        map[string]string
-		KubeSecret map[string]string
-		Phases     []phase
+		Name          string
+		Env           map[string]string
+		KubeSecret    map[string]string
+		KubeDenyPatch bool
+		Phases        []phase
 	}{
 		{
 			// Out of the box default: runs in userspace mode, ephemeral storage, interactive login.
@@ -370,6 +371,35 @@ type phase struct {
 				},
 			},
 		},
+		{
+			Name: "kube_storage_no_patch",
+			Env: map[string]string{
+				"KUBERNETES_SERVICE_HOST":       kube.Host,
+				"KUBERNETES_SERVICE_PORT_HTTPS": kube.Port,
+				"TS_AUTH_KEY":                   "tskey-key",
+			},
+			KubeSecret:    map[string]string{},
+			KubeDenyPatch: true,
+			Phases: []phase{
+				{
+					WantCmds: []string{
+						"/usr/bin/tailscaled --socket=/tmp/tailscaled.sock --state=kube:tailscale --statedir=/tmp --tun=userspace-networking",
+						"/usr/bin/tailscale --socket=/tmp/tailscaled.sock up --accept-dns=false --authkey=tskey-key",
+					},
+					WantKubeSecret: map[string]string{},
+				},
+				{
+					Status: ipnstate.Status{
+						BackendState: "Running",
+						TailscaleIPs: tsIPs,
+						Self: &ipnstate.PeerStatus{
+							ID: tailcfg.StableNodeID("myID"),
+						},
+					},
+					WantKubeSecret: map[string]string{},
+				},
+			},
+		},
 		{
 			// Same as previous, but deletes the authkey from the kube secret.
 			Name: "kube_storage_auth_once",
@@ -492,6 +522,7 @@ type phase struct {
 			for k, v := range test.KubeSecret {
 				kube.SetSecret(k, v)
 			}
+			kube.SetPatching(!test.KubeDenyPatch)
 
 			cmd := exec.Command(boot)
 			cmd.Env = []string{
@@ -722,7 +753,8 @@ type kubeServer struct {
 	srv *httptest.Server
 
 	sync.Mutex
-	secret map[string]string
+	secret   map[string]string
+	canPatch bool
 }
 
 func (k *kubeServer) Secret() map[string]string {
@@ -741,6 +773,12 @@ func (k *kubeServer) SetSecret(key, val string) {
 	k.secret[key] = val
 }
 
+func (k *kubeServer) SetPatching(canPatch bool) {
+	k.Lock()
+	defer k.Unlock()
+	k.canPatch = canPatch
+}
+
 func (k *kubeServer) Reset() {
 	k.Lock()
 	defer k.Unlock()
@@ -784,16 +822,39 @@ func (k *kubeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	if r.Header.Get("Authorization") != "Bearer bearer_token" {
 		panic("client didn't provide bearer token in request")
 	}
-	if r.URL.Path == "/apis/authorization.k8s.io/v1/selfsubjectaccessreviews" {
-		// Just say yes to all SARs, we don't enforce RBAC.
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `{"status":{"allowed":true}}`)
-		return
-	}
-	if r.URL.Path != "/api/v1/namespaces/default/secrets/tailscale" {
+	switch r.URL.Path {
+	case "/api/v1/namespaces/default/secrets/tailscale":
+		k.serveSecret(w, r)
+	case "/apis/authorization.k8s.io/v1/selfsubjectaccessreviews":
+		k.serveSSAR(w, r)
+	default:
 		panic(fmt.Sprintf("unhandled fake kube api path %q", r.URL.Path))
 	}
+}
 
+func (k *kubeServer) serveSSAR(w http.ResponseWriter, r *http.Request) {
+	var req struct {
+		Spec struct {
+			ResourceAttributes struct {
+				Verb string `json:"verb"`
+			} `json:"resourceAttributes"`
+		} `json:"spec"`
+	}
+	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+		panic(fmt.Sprintf("decoding SSAR request: %v", err))
+	}
+	ok := true
+	if req.Spec.ResourceAttributes.Verb == "patch" {
+		k.Lock()
+		defer k.Unlock()
+		ok = k.canPatch
+	}
+	// Just say yes to all SARs, we don't enforce RBAC.
+	w.Header().Set("Content-Type", "application/json")
+	fmt.Fprintf(w, `{"status":{"allowed":%v}}`, ok)
+}
+
+func (k *kubeServer) serveSecret(w http.ResponseWriter, r *http.Request) {
 	bs, err := io.ReadAll(r.Body)
 	if err != nil {
 		http.Error(w, fmt.Sprintf("reading request body: %v", err), http.StatusInternalServerError)
@@ -819,6 +880,11 @@ func (k *kubeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			panic("encode failed")
 		}
 	case "PATCH":
+		k.Lock()
+		defer k.Unlock()
+		if !k.canPatch {
+			panic("containerboot tried to patch despite not being allowed")
+		}
 		switch r.Header.Get("Content-Type") {
 		case "application/json-patch+json":
 			req := []struct {
@@ -828,8 +894,6 @@ func (k *kubeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			if err := json.Unmarshal(bs, &req); err != nil {
 				panic(fmt.Sprintf("json decode failed: %v. Body:\n\n%s", err, string(bs)))
 			}
-			k.Lock()
-			defer k.Unlock()
 			for _, op := range req {
 				if op.Op != "remove" {
 					panic(fmt.Sprintf("unsupported json-patch op %q", op.Op))
@@ -846,8 +910,6 @@ func (k *kubeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			if err := json.Unmarshal(bs, &req); err != nil {
 				panic(fmt.Sprintf("json decode failed: %v. Body:\n\n%s", err, string(bs)))
 			}
-			k.Lock()
-			defer k.Unlock()
 			for key, val := range req.Data {
 				k.secret[key] = val
 			}