Don't override shareProcessNamespace unless annotation is explicitly set

By having a default of `false`, we would automatically set `shareProcessNamespace` to `false` when injecting the agent pod, regardless of what the main manifest had set. Instead, we should only set `shareProcessNamespace` if the annotation is explicitly set, and otherwise let the default from the main manifest stand. Fixes #436 (thanks @justinas-b for reporting it). I tested this by: * Before this patch: * Create a pod with `shareProcessNamespace: true` with injected agent * Verifying that the containers cannot see each other with `ps -ax` * After this patch: * Create a pod with `shareProcessNamespace: true` with injected agent and no annotation * Verifying that the containers *can* see each other with `ps -ax` * Create a pod with `shareProcessNamespace: true` with injected agent and annotation set to `'false'` * Verifying that the containers *cannot* see each other with `ps -ax`
hashicorp · Mar 17, 2023 · 0573487 · 0573487
1 parent d705584
commit 0573487
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,9 @@ Changes:
   * golang.org/x/term v0.3.0 => v0.5.0
   * golang.org/x/text v0.5.0 => v0.7.0
 
+Bugs:
+* Don't override `shareProcessNamespace` if an annotation is not present [GH-445](https://github.com/hashicorp/vault-k8s/pull/445)
+
 ## 1.2.0 (February 6, 2023)
 
 Changes:

diff --git a/agent-inject/agent/agent.go b/agent-inject/agent/agent.go
@@ -22,7 +22,6 @@ const (
 	DefaultAgentRunAsUser                   = 100
 	DefaultAgentRunAsGroup                  = 1000
 	DefaultAgentRunAsSameUser               = false
-	DefaultAgentShareProcessNamespace       = false
 	DefaultAgentAllowPrivilegeEscalation    = false
 	DefaultAgentDropCapabilities            = "ALL"
 	DefaultAgentSetSecurityContext          = true
@@ -142,7 +141,7 @@ type Agent struct {
 	RunAsSameID bool
 
 	// ShareProcessNamespace sets the shareProcessNamespace value on the pod spec.
-	ShareProcessNamespace bool
+	ShareProcessNamespace *bool
 
 	// SetSecurityContext controls whether the injected containers have a
 	// SecurityContext set.
@@ -445,10 +444,14 @@ func New(pod *corev1.Pod) (*Agent, error) {
 		return agent, err
 	}
 
-	agent.ShareProcessNamespace, err = agent.setShareProcessNamespace(pod)
+	setShareProcessNamespace, ok, err := agent.setShareProcessNamespace(pod)
 	if err != nil {
 		return agent, err
 	}
+	if ok {
+		agent.ShareProcessNamespace = new(bool)
+		*agent.ShareProcessNamespace = setShareProcessNamespace
+	}
 
 	agent.SetSecurityContext, err = agent.setSecurityContext()
 	if err != nil {
@@ -655,7 +658,9 @@ func (a *Agent) Patch() ([]byte, error) {
 		}
 
 		// Add shareProcessNamespace
-		patches = append(patches, updateShareProcessNamespace(a.ShareProcessNamespace)...)
+		if a.ShareProcessNamespace != nil {
+			patches = append(patches, updateShareProcessNamespace(*a.ShareProcessNamespace)...)
+		}
 	}
 
 	// Sidecar Container

diff --git a/agent-inject/agent/annotations.go b/agent-inject/agent/annotations.go
@@ -349,7 +349,6 @@ func Init(pod *corev1.Pod, cfg AgentConfig) error {
 	var runAsUserIsSet bool
 	var runAsSameUserIsSet bool
 	var runAsGroupIsSet bool
-	var shareProcessNamespaceIsSet bool
 
 	if pod == nil {
 		return errors.New("pod is empty")
@@ -469,10 +468,6 @@ func Init(pod *corev1.Pod, cfg AgentConfig) error {
 		pod.ObjectMeta.Annotations[AnnotationAgentRunAsSameUser] = strconv.FormatBool(cfg.SameID)
 	}
 
-	if _, shareProcessNamespaceIsSet = pod.ObjectMeta.Annotations[AnnotationAgentShareProcessNamespace]; !shareProcessNamespaceIsSet {
-		pod.ObjectMeta.Annotations[AnnotationAgentShareProcessNamespace] = strconv.FormatBool(cfg.ShareProcessNamespace)
-	}
-
 	if _, runAsGroupIsSet = pod.ObjectMeta.Annotations[AnnotationAgentRunAsGroup]; !runAsGroupIsSet {
 		if cfg.GroupID == "" {
 			cfg.GroupID = strconv.Itoa(DefaultAgentRunAsGroup)
@@ -754,26 +749,27 @@ func (a *Agent) runAsSameID(pod *corev1.Pod) (bool, error) {
 	return runAsSameID, nil
 }
 
-func (a *Agent) setShareProcessNamespace(pod *corev1.Pod) (bool, error) {
+// returns value, ok, error
+func (a *Agent) setShareProcessNamespace(pod *corev1.Pod) (bool, bool, error) {
 	annotation := AnnotationAgentShareProcessNamespace
 	raw, ok := a.Annotations[annotation]
 	if !ok {
-		return DefaultAgentShareProcessNamespace, nil
+		return false, false, nil
 	}
 	shareProcessNamespace, err := strconv.ParseBool(raw)
 	if err != nil {
-		return DefaultAgentShareProcessNamespace, fmt.Errorf(
+		return false, true, fmt.Errorf(
 			"invalid value %v for annotation %q, err=%w", raw, annotation, err)
 	}
 	if pod.Spec.ShareProcessNamespace != nil {
 		if !*pod.Spec.ShareProcessNamespace && shareProcessNamespace {
-			return DefaultAgentShareProcessNamespace,
+			return false, true,
 				errors.New("shareProcessNamespace explicitly disabled on the pod, " +
 					"refusing to enable it")
 		}
 	}
 
-	return shareProcessNamespace, nil
+	return shareProcessNamespace, true, nil
 }
 
 func (a *Agent) setSecurityContext() (bool, error) {

diff --git a/agent-inject/agent/annotations_test.go b/agent-inject/agent/annotations_test.go
@@ -97,12 +97,17 @@ func TestInitDefaults(t *testing.T) {
 		{annotationKey: AnnotationAgentImage, annotationValue: DefaultVaultImage},
 		{annotationKey: AnnotationAgentRunAsUser, annotationValue: strconv.Itoa(DefaultAgentRunAsUser)},
 		{annotationKey: AnnotationAgentRunAsGroup, annotationValue: strconv.Itoa(DefaultAgentRunAsGroup)},
-		{annotationKey: AnnotationAgentShareProcessNamespace, annotationValue: strconv.FormatBool(DefaultAgentShareProcessNamespace)},
+		{annotationKey: AnnotationAgentShareProcessNamespace, annotationValue: ""},
 	}
 
 	for _, tt := range tests {
 		raw, ok := pod.Annotations[tt.annotationKey]
-		if !ok {
+		if tt.annotationValue == "" && !ok {
+			// okay, we expected it not to be set
+			continue
+		} else if tt.annotationValue == "" && ok {
+			t.Errorf("Default annotation value incorrect, wanted unset, got %s", raw)
+		} else if !ok {
 			t.Errorf("Default annotation %s not set, it should be.", tt.annotationKey)
 		}
 
@@ -988,7 +993,6 @@ func TestInjectContainers(t *testing.T) {
 				internal.AddOp("/spec/containers/1/volumeMounts/-", nil),
 				internal.AddOp("/spec/containers/2/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(AnnotationAgentStatus), nil),
 			},
@@ -1003,7 +1007,6 @@ func TestInjectContainers(t *testing.T) {
 				internal.AddOp("/spec/volumes", nil),
 				internal.AddOp("/spec/containers/1/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(AnnotationAgentStatus), nil),
 			},
@@ -1019,7 +1022,6 @@ func TestInjectContainers(t *testing.T) {
 				internal.AddOp("/spec/containers/1/volumeMounts/-", nil),
 				internal.AddOp("/spec/containers/2/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(AnnotationAgentStatus), nil),
 			},

diff --git a/agent-inject/handler_test.go b/agent-inject/handler_test.go
@@ -160,7 +160,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},
@@ -192,7 +191,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/initContainers", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/1/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},
@@ -222,7 +220,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},
@@ -253,7 +250,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},
@@ -284,7 +280,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},
@@ -343,7 +338,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},
 		},
@@ -372,7 +366,6 @@ func TestHandlerHandle(t *testing.T) {
 				internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
 				internal.AddOp("/spec/initContainers/-", nil),
 				internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
-				internal.AddOp("/spec/shareProcessNamespace", nil),
 				internal.AddOp("/spec/containers/-", nil),
 				internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
 			},