Skip to content

fix(misconf): .Config.User always takes precedence over USER in .History #9050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 19, 2025
Merged

fix(misconf): .Config.User always takes precedence over USER in .History #9050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
650d5b9
Update dockerfile.go
lyoung-confluent Jun 18, 2025
d7cad56
Update dockerfile_test.go
lyoung-confluent Jun 18, 2025
b9b43e9
Update dockerfile.go
lyoung-confluent Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions pkg/fanal/analyzer/imgconf/dockerfile/dockerfile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ func (a *historyAnalyzer) Analyze(ctx context.Context, input analyzer.ConfigAnal

func imageConfigToDockerfile(cfg *v1.ConfigFile) []byte {
dockerfile := new(bytes.Buffer)
var userFound bool
baseLayerIndex := image.GuessBaseImageIndex(cfg.History)
for i := baseLayerIndex + 1; i < len(cfg.History); i++ {
h := cfg.History[i]
Expand All @@ -101,7 +100,6 @@ func imageConfigToDockerfile(cfg *v1.ConfigFile) []byte {
case strings.HasPrefix(h.CreatedBy, "USER"):
// USER instruction
createdBy = h.CreatedBy
userFound = true
case strings.HasPrefix(h.CreatedBy, "HEALTHCHECK"):
// HEALTHCHECK instruction
createdBy = buildHealthcheckInstruction(cfg.Config.Healthcheck)
Expand All @@ -119,7 +117,8 @@ func imageConfigToDockerfile(cfg *v1.ConfigFile) []byte {
dockerfile.WriteString(strings.TrimSpace(createdBy) + "\n")
}

if !userFound && cfg.Config.User != "" {
// The user can be changed from the config file or with the `--user` flag (for docker CLI), so we need to add this user to avoid incorrect user detection
if cfg.Config.User != "" {
Copy link
Contributor

@DmitriyLewen DmitriyLewen Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried that we'll add another user to the resulting dockerfile.
e.g. for this example result Dockerfile will be:

...
USER builder
USER root
...
USER builder

@nikpivkin Is this okay for misconfig scan?
Will we get some unexpected result for one of the rules?

Copy link
Contributor Author

@lyoung-confluent lyoung-confluent Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already valid to have multiple USER lines in a Dockerfile, for example this is commonly used to "sudo" and run a command during a build, ex:

FROM some-image-that-uses-non-root-already

USER root
RUN apt install foobar
USER nonroot

Copy link
Contributor

@DmitriyLewen DmitriyLewen Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are right.
i confused myself 😄
I thought about this case again, we inserted the user on the last line, and the dockerfile supports multiple "USER" lines

Copy link
Contributor

@nikpivkin nikpivkin Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this okay for misconfig scan?

Yeah, that's okay.

user := fmt.Sprintf("USER %s", cfg.Config.User)
dockerfile.WriteString(user)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/fanal/analyzer/imgconf/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ func Test_historyAnalyzer_Analyze(t *testing.T) {
CreatedBy: "RUN /bin/sh -c ls -hl /foo # buildkit",
EmptyLayer: false,
},
{
CreatedBy: "USER root", // .Config.User takes precedence over this line
EmptyLayer: true,
},
{
CreatedBy: `HEALTHCHECK &{["CMD-SHELL" "curl -sS 127.0.0.1 || exit 1"] "10s" "3s" "0s" '\x00'}`,
EmptyLayer: true,
Expand Down