Enforce read-only valid volume rules

pkg/container/docker_run.go

@@ -1006,27 +1006,78 @@ func (cr *containerReference) wait() common.Executor {
 }
 
 // For Gitea
+// volumeRule is one compiled valid_volumes entry. An entry ending in `:ro`
+// allows the source but forces the mount read-only; an unmarked entry allows
+// the source and preserves the requested mode.
+type volumeRule struct {
+	pattern  glob.Glob
+	readOnly bool
+}
+
+func compileVolumeRules(ctx context.Context, validVolumes []string) []volumeRule {
+	logger := common.Logger(ctx)
+
+	rules := make([]volumeRule, 0, len(validVolumes))
+	for _, v := range validVolumes {
+		source, readOnly := strings.CutSuffix(v, ":ro")
+		if g, err := glob.Compile(source); err != nil {
+			logger.Errorf("create glob from %s error: %v", source, err)
+		} else {
+			rules = append(rules, volumeRule{pattern: g, readOnly: readOnly})
+		}
+	}
+	return rules
+}
+
+// matchVolumeRules reports whether a mount source is allowed and whether it
+// must be read-only. Rules are checked in configuration order and the last
+// matching rule wins.
+func matchVolumeRules(rules []volumeRule, source string) (allowed, readOnly bool) {
+	for _, r := range rules {
+		if r.pattern.Match(source) {
+			allowed = true
+			readOnly = r.readOnly
+		}
+	}
+	return allowed, readOnly
+}
+
+// forceBindReadOnly rewrites a parsed bind spec's mode to `ro`, keeping other
+// options such as SELinux labels and propagation modifiers.
+func forceBindReadOnly(bind, source, target string) string {
+	base := source + ":" + target
+	if bind == base {
+		return bind + ":ro"
+	}
+
+	rawOptions, hasOptions := strings.CutPrefix(bind, base+":")
+	if !hasOptions {
+		return bind + ":ro"
+	}
+	opts := strings.Split(rawOptions, ",")
+	sanitized := make([]string, 0, len(opts)+1)
+	hasMode := false
+	for _, o := range opts {
+		if o == "rw" {
+			o = "ro"
+		}
+		if o == "ro" {
+			hasMode = true
+		}
+		sanitized = append(sanitized, o)
+	}
+	if !hasMode {
+		sanitized = append(sanitized, "ro")
+	}
+	return base + ":" + strings.Join(sanitized, ",")
+}
+
 // sanitizeConfig remove the invalid configurations from `config` and `hostConfig`
 func (cr *containerReference) sanitizeConfig(ctx context.Context, config *container.Config, hostConfig *container.HostConfig) (*container.Config, *container.HostConfig) {
 	logger := common.Logger(ctx)
 
 	if len(cr.input.ValidVolumes) > 0 {
-		globs := make([]glob.Glob, 0, len(cr.input.ValidVolumes))
-		for _, v := range cr.input.ValidVolumes {
-			if g, err := glob.Compile(v); err != nil {
-				logger.Errorf("create glob from %s error: %v", v, err)
-			} else {
-				globs = append(globs, g)
-			}
-		}
-		isValid := func(v string) bool {
-			for _, g := range globs {
-				if g.Match(v) {
-					return true
-				}
-			}
-			return false
-		}
+		rules := compileVolumeRules(ctx, cr.input.ValidVolumes)
 		// sanitize binds
 		sanitizedBinds := make([]string, 0, len(hostConfig.Binds))
 		for _, bind := range hostConfig.Binds {
@@ -1040,21 +1091,29 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai
 				sanitizedBinds = append(sanitizedBinds, bind)
 				continue
 			}
-			if isValid(parsed.Source) {
-				sanitizedBinds = append(sanitizedBinds, bind)
-			} else {
+			allowed, readOnly := matchVolumeRules(rules, parsed.Source)
+			if !allowed {
 				logger.Warnf("[%s] is not a valid volume, will be ignored", parsed.Source)
+				continue
 			}
+			if readOnly && !parsed.ReadOnly {
+				bind = forceBindReadOnly(bind, parsed.Source, parsed.Target)
+			}
+			sanitizedBinds = append(sanitizedBinds, bind)
 		}
 		hostConfig.Binds = sanitizedBinds
 		// sanitize mounts
 		sanitizedMounts := make([]mount.Mount, 0, len(hostConfig.Mounts))
 		for _, mt := range hostConfig.Mounts {
-			if isValid(mt.Source) {
-				sanitizedMounts = append(sanitizedMounts, mt)
-			} else {
+			allowed, readOnly := matchVolumeRules(rules, mt.Source)
+			if !allowed {
 				logger.Warnf("[%s] is not a valid volume, will be ignored", mt.Source)
+				continue
+			}
+			if readOnly {
+				mt.ReadOnly = true
 			}
+			sanitizedMounts = append(sanitizedMounts, mt)
 		}
 		hostConfig.Mounts = sanitizedMounts
 	} else {

pkg/container/docker_run_test.go

@@ -258,10 +258,12 @@ var _ ExecutionsEnvironment = &containerReference{}
 
 func TestCheckVolumes(t *testing.T) {
 	testCases := []struct {
-		desc          string
-		validVolumes  []string
-		binds         []string
-		expectedBinds []string
+		desc           string
+		validVolumes   []string
+		binds          []string
+		mounts         []mount.Mount
+		expectedBinds  []string
+		expectedMounts []mount.Mount
 	}{
 		{
 			desc:         "match all volumes",
@@ -313,6 +315,97 @@ func TestCheckVolumes(t *testing.T) {
 				"/etc/conf.d/base.json:/config/base.json",
 			},
 		},
+		{
+			desc:          "read-only entry forces a bind read-only",
+			validVolumes:  []string{"/etc/ssl/certs/ca-certificates.crt:ro"},
+			binds:         []string{"/etc/ssl/certs/ca-certificates.crt:/etc/ssl/certs/ca-certificates.crt"},
+			expectedBinds: []string{"/etc/ssl/certs/ca-certificates.crt:/etc/ssl/certs/ca-certificates.crt:ro"},
+		},
+		{
+			desc:          "read-only entry rewrites a requested rw mode",
+			validVolumes:  []string{"/host/toolcache:ro"},
+			binds:         []string{"/host/toolcache:/opt/hostedtoolcache:rw"},
+			expectedBinds: []string{"/host/toolcache:/opt/hostedtoolcache:ro"},
+		},
+		{
+			desc:          "unmarked entry preserves the requested rw mode",
+			validVolumes:  []string{"/home/test/data"},
+			binds:         []string{"/home/test/data:/test_data:rw"},
+			expectedBinds: []string{"/home/test/data:/test_data:rw"},
+		},
+		{
+			desc:          "already read-only bind stays read-only",
+			validVolumes:  []string{"/home/test/data:ro"},
+			binds:         []string{"/home/test/data:/test_data:ro"},
+			expectedBinds: []string{"/home/test/data:/test_data:ro"},
+		},
+		{
+			desc:          "read-only entry keeps other bind modifiers",
+			validVolumes:  []string{"/home/test/data:ro"},
+			binds:         []string{"/home/test/data:/test_data:z"},
+			expectedBinds: []string{"/home/test/data:/test_data:z,ro"},
+		},
+		{
+			desc:          "read-only entry handles Windows drive paths without a mode",
+			validVolumes:  []string{"C:/host/data:ro"},
+			binds:         []string{"C:/host/data:D:/container/data"},
+			expectedBinds: []string{"C:/host/data:D:/container/data:ro"},
+		},
+		{
+			desc:          "non-matching bind is dropped when read-only rules exist",
+			validVolumes:  []string{"/home/test/data:ro"},
+			binds:         []string{"/secrets/keys:/keys"},
+			expectedBinds: []string{},
+		},
+		{
+			desc: "last matching rule wins for overlapping entries",
+			validVolumes: []string{
+				"/data/**",
+				"/data/cache:ro",
+			},
+			binds: []string{
+				"/data/cache:/cache",
+				"/data/tool:/tool",
+			},
+			expectedBinds: []string{
+				"/data/cache:/cache:ro",
+				"/data/tool:/tool",
+			},
+		},
+		{
+			desc: "internally appended exact entry stays writable",
+			validVolumes: []string{
+				"/**:ro",
+				"/var/run/docker.sock",
+			},
+			binds: []string{
+				"/var/run/docker.sock:/var/run/docker.sock",
+				"/etc/passwd:/pw",
+			},
+			expectedBinds: []string{
+				"/var/run/docker.sock:/var/run/docker.sock",
+				"/etc/passwd:/pw:ro",
+			},
+		},
+		{
+			desc:         "read-only entry forces a long-form mount read-only",
+			validVolumes: []string{"/etc/ssl/certs/ca-certificates.crt:ro"},
+			mounts: []mount.Mount{
+				{
+					Type:   mount.TypeBind,
+					Source: "/etc/ssl/certs/ca-certificates.crt",
+					Target: "/etc/ssl/certs/ca-certificates.crt",
+				},
+			},
+			expectedMounts: []mount.Mount{
+				{
+					Type:     mount.TypeBind,
+					Source:   "/etc/ssl/certs/ca-certificates.crt",
+					Target:   "/etc/ssl/certs/ca-certificates.crt",
+					ReadOnly: true,
+				},
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -323,8 +416,15 @@ func TestCheckVolumes(t *testing.T) {
 					ValidVolumes: tc.validVolumes,
 				},
 			}
-			_, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{Binds: tc.binds})
+			_, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{Binds: tc.binds, Mounts: tc.mounts})
+			if tc.expectedBinds == nil {
+				tc.expectedBinds = []string{}
+			}
+			if tc.expectedMounts == nil {
+				tc.expectedMounts = []mount.Mount{}
+			}
 			assert.Equal(t, tc.expectedBinds, hostConf.Binds)
+			assert.Equal(t, tc.expectedMounts, hostConf.Mounts)
 		})
 	}
 }