diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 50997cf9..88fe07e4 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -8,6 +8,7 @@ package container import ( "bytes" + "cmp" "context" "errors" "fmt" @@ -1178,7 +1179,10 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai sanitizedBinds = append(sanitizedBinds, bind) continue } - if matcher.isValid(parsed.Source, mount.Type(parsed.Type)) { + if matcher.isValid(parsed.Source, mount.Type(parsed.Type), parsed.ReadOnly) { + if matcher.needsNoCopy(parsed.Source, mount.Type(parsed.Type)) && (parsed.Volume == nil || !parsed.Volume.NoCopy) { + bind += ",nocopy" + } sanitizedBinds = append(sanitizedBinds, bind) } else { logger.Warnf("[%s] is not a valid volume, will be ignored", parsed.Source) @@ -1194,7 +1198,12 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai // sanitize mounts sanitizedMounts := make([]mount.Mount, 0, len(hostConfig.Mounts)) for _, mt := range hostConfig.Mounts { - if matcher.isValid(mt.Source, mt.Type) { + readOnly := mt.ReadOnly && (mt.BindOptions == nil || !mt.BindOptions.ReadOnlyNonRecursive) // bind-recursive=writable leaves submounts writable + if matcher.isValid(mt.Source, mt.Type, readOnly) { + if matcher.needsNoCopy(mt.Source, mt.Type) { + mt.VolumeOptions = cmp.Or(mt.VolumeOptions, &mount.VolumeOptions{}) + mt.VolumeOptions.NoCopy = true + } sanitizedMounts = append(sanitizedMounts, mt) } else { logger.Warnf("[%s] is not a valid volume, will be ignored", mt.Source) @@ -1240,26 +1249,37 @@ func overlayVolumes(dst, src *container.HostConfig) { } type validVolumeMatcher struct { - allowAll bool - named []string - host []string + allowAll bool + allowAllReadOnly bool + named []validVolumeRule + host []validVolumeRule +} + +type validVolumeRule struct { + pattern string + readOnly bool } func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolumeMatcher { logger := common.Logger(ctx) ret := validVolumeMatcher{ - named: make([]string, 0, len(validVolumes)), - host: make([]string, 0, len(validVolumes)), + named: make([]validVolumeRule, 0, len(validVolumes)), + host: make([]validVolumeRule, 0, len(validVolumes)), } for _, v := range validVolumes { + v, readOnly := strings.CutSuffix(v, ":ro") if v == "**" { - ret.allowAll = true + if readOnly { + ret.allowAllReadOnly = true + } else { + ret.allowAll = true + } continue } if !isHostVolumePattern(v) { if doublestar.ValidatePattern(v) { - ret.named = append(ret.named, v) + ret.named = append(ret.named, validVolumeRule{pattern: v, readOnly: readOnly}) } else { logger.Errorf("invalid volume pattern %s", v) } @@ -1271,7 +1291,7 @@ func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolu continue } if doublestar.ValidatePathPattern(normalized) { - ret.host = append(ret.host, normalized) + ret.host = append(ret.host, validVolumeRule{pattern: normalized, readOnly: readOnly}) } else { logger.Errorf("invalid volume pattern %s", normalized) } @@ -1280,33 +1300,34 @@ func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolu return ret } -func (m validVolumeMatcher) isValid(source string, sourceType mount.Type) bool { - if m.allowAll { +func (m validVolumeMatcher) isValid(source string, sourceType mount.Type, readOnly bool) bool { + if m.allowAll || (m.allowAllReadOnly && readOnly) { return true } if sourceType == mount.TypeTmpfs && source == "" { // backed by memory, exposes nothing from the host, same as --tmpfs return true } + rules, match := m.named, doublestar.MatchUnvalidated if isHostVolumeSource(source, sourceType) { normalized, err := normalizeHostVolumePath(source) if err != nil { return false } - for _, pattern := range m.host { - if doublestar.PathMatchUnvalidated(pattern, normalized) { - return true - } - } - return false + rules, match, source = m.host, doublestar.PathMatchUnvalidated, normalized } - for _, pattern := range m.named { - if doublestar.MatchUnvalidated(pattern, source) { + for _, rule := range rules { + if (readOnly || !rule.readOnly) && match(rule.pattern, source) { return true } } return false } +// needsNoCopy reports a volume only `:ro` rules allow, as dockerd fills an empty volume from the image even when mounted read-only. +func (m validVolumeMatcher) needsNoCopy(source string, sourceType mount.Type) bool { + return sourceType == mount.TypeVolume && !m.isValid(source, sourceType, false) +} + func isHostVolumePattern(pattern string) bool { return filepath.IsAbs(pattern) || strings.HasPrefix(pattern, "."+string(filepath.Separator)) || diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 249141d5..a2c59c51 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -519,10 +519,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 + expectedBinds []string + mounts []mount.Mount + expectedMounts []mount.Mount }{ { desc: "match all volumes", @@ -578,6 +580,27 @@ func TestCheckVolumes(t *testing.T) { "/opt/app/cache:/cache", }, }, + { + desc: "read-only rules allow only read-only mounts and copy nothing into volumes", + validVolumes: []string{"/etc/ssl:ro", "data:ro", "shared"}, + binds: []string{"/etc/ssl:/ssl:ro", "/etc/ssl:/ssl", "data:/data:ro", "data:/data", "shared:/shared:ro"}, + expectedBinds: []string{"/etc/ssl:/ssl:ro", "data:/data:ro,nocopy", "shared:/shared:ro"}, + mounts: []mount.Mount{ + {Type: mount.TypeBind, Source: "/etc/ssl", Target: "/ssl", ReadOnly: true}, + {Type: mount.TypeBind, Source: "/etc/ssl", Target: "/ssl", ReadOnly: true, BindOptions: &mount.BindOptions{ReadOnlyNonRecursive: true}}, + {Type: mount.TypeVolume, Source: "data", Target: "/data", ReadOnly: true}, + }, + expectedMounts: []mount.Mount{ + {Type: mount.TypeBind, Source: "/etc/ssl", Target: "/ssl", ReadOnly: true}, + {Type: mount.TypeVolume, Source: "data", Target: "/data", ReadOnly: true, VolumeOptions: &mount.VolumeOptions{NoCopy: true}}, + }, + }, + { + desc: "read-only wildcard keeps writable rules writable", + validVolumes: []string{"**:ro", "shared"}, + binds: []string{"/etc/ssl:/ssl:ro", "data:/data", "shared:/shared"}, + expectedBinds: []string{"/etc/ssl:/ssl:ro", "shared:/shared"}, + }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { @@ -588,9 +611,10 @@ 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}) assert.Equal(t, tc.expectedBinds, hostConf.Binds) - assert.Len(t, hook.AllEntries(), len(tc.binds)-len(tc.expectedBinds)) // every drop is warned about + assert.ElementsMatch(t, tc.expectedMounts, hostConf.Mounts) + assert.Len(t, hook.AllEntries(), len(tc.binds)+len(tc.mounts)-len(tc.expectedBinds)-len(tc.expectedMounts)) // every drop is warned about }) } } @@ -771,7 +795,7 @@ func TestSanitizeConfigKeepsTmpfsMounts(t *testing.T) { hostConfig, _ := mergeOptions(t, "", "--mount type=tmpfs,dst=/var/lib/mysql", false) require.Len(t, hostConfig.Mounts, 1) - for _, validVolumes := range [][]string{nil, {"data"}} { + for _, validVolumes := range [][]string{nil, {"data"}, {"**:ro"}} { logger, hook := test.NewNullLogger() _, sanitized := (&containerReference{input: &NewContainerInput{ValidVolumes: validVolumes}}).sanitizeConfig(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{Mounts: []mount.Mount{ hostConfig.Mounts[0], diff --git a/internal/pkg/config/config.example.yaml b/internal/pkg/config/config.example.yaml index fb33dc2c..f18d6ecd 100644 --- a/internal/pkg/config/config.example.yaml +++ b/internal/pkg/config/config.example.yaml @@ -239,10 +239,12 @@ container: #workdir_parent: # Volumes and bind mounts containers may mount, as globs (https://github.com/bmatcuk/doublestar#patterns). # `*` matches within a path segment, `**` across segments. Empty allows none. tmpfs mounts are always allowed. - # For example, to allow the `data` volume and all json files in `/src`: + # A `:ro` suffix allows only read-only mounts, unless an entry without it matches too. + # For example, to allow the `data` volume and all json files in `/src`, and `/etc/ssl/certs` only read-only: # valid_volumes: # - data # - /src/*.json + # - /etc/ssl/certs:ro # To allow any volume: # valid_volumes: # - '**'