Skip to content
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

extender: better uid checks and error if it stay as root #1093

Merged
merged 1 commit into from
Jun 1, 2023
Merged

extender: better uid checks and error if it stay as root #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 14 additions & 5 deletions extender.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,15 +307,14 @@ func (e *Extender) extend(kind string, baseImage v1.Image, logger log.Logger) (v
}
workingHistory = configFile.History
buildOptions := e.extendOptions()
userID, groupID := userFrom(*configFile)
origUserID := userID
for _, dockerfile := range dockerfiles {
// create args
userID, groupID := userFrom(*configFile)
dockerfile.Args = append([]extend.Arg{
{Name: argBuildID, Value: uuid.New().String()},
{Name: argUserID, Value: userID},
{Name: argGroupID, Value: groupID},
}, dockerfile.Args...)

// apply Dockerfile
if baseImage, err = e.DockerfileApplier.Apply(
dockerfile,
Expand All @@ -325,8 +324,7 @@ func (e *Extender) extend(kind string, baseImage v1.Image, logger log.Logger) (v
); err != nil {
return nil, fmt.Errorf("applying Dockerfile to image: %w", err)
}

// update rebasable, history in config
// update rebasable, history in config, and user/group IDs
configFile, err = baseImage.ConfigFile()
if err != nil || configFile == nil {
return nil, fmt.Errorf("getting image config: %w", err)
Expand All @@ -350,6 +348,17 @@ func (e *Extender) extend(kind string, baseImage v1.Image, logger log.Logger) (v
)
}
configFile.History = workingHistory
prevUserID := userID
userID, groupID = userFrom(*configFile)
if userID == "0" {
logger.Warnf("Extension from %s changed the user ID from %d to %d; this must not be the final user ID (a following extension must reset the user).", prevUserID, userID, dockerfile.Path)
}
}
if userID == "0" {
return baseImage, fmt.Errorf("the final user ID is 0 (root); please add another extension that resets the user to non-root")
}
if userID != origUserID {
logger.Warnf("The original user ID was %s but the final extension left the user ID set to %s.", origUserID, userID)
}
if kind == buildpack.DockerfileKindBuild {
return baseImage, nil
Expand Down
47 changes: 45 additions & 2 deletions extender_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func testExtender(t *testing.T, when spec.G, it spec.S) {
// first dockerfile

firstConfig := &v1.ConfigFile{Config: v1.Config{
User: "1234:5678",
User: "0:5678",
}}
someFakeImage.ConfigFileReturnsOnCall(0, firstConfig, nil)
fakeDockerfileApplier.EXPECT().Apply(
Expand All @@ -247,7 +247,7 @@ func testExtender(t *testing.T, when spec.G, it spec.S) {
_, err := uuid.Parse(dockerfile.Args[0].Value)
h.AssertNil(t, err)
h.AssertEq(t, dockerfile.Args[1].Name, "user_id")
h.AssertEq(t, dockerfile.Args[1].Value, "1234")
h.AssertEq(t, dockerfile.Args[1].Value, "0")
h.AssertEq(t, dockerfile.Args[2].Name, "group_id")
h.AssertEq(t, dockerfile.Args[2].Value, "5678")
h.AssertEq(t, dockerfile.Args[3], expectedDockerfileA.Args[0])
Expand Down Expand Up @@ -295,6 +295,49 @@ func testExtender(t *testing.T, when spec.G, it spec.S) {
h.AssertEq(t, os.Getenv("SOME_VAR"), "some-val")
h.AssertNil(t, os.Unsetenv("SOME_VAR"))
})
it("errors if the last extension leaves the user as root", func() {
expectedDockerfileA := extend.Dockerfile{
Path: filepath.Join(generatedDir, "build", "A", "Dockerfile"),
Args: []extend.Arg{{Name: "argA", Value: "valueA"}},
}
h.Mkdir(t, filepath.Join(generatedDir, "build", "A"))
h.Mkfile(t, "some dockerfile content", filepath.Join(generatedDir, "build", "A", "Dockerfile"))
buf := new(bytes.Buffer)
data := extend.Config{Build: extend.BuildConfig{Args: expectedDockerfileA.Args}}
h.AssertNil(t, toml.NewEncoder(buf).Encode(data))
h.Mkfile(t, buf.String(), filepath.Join(generatedDir, "build", "A", "extend-config.toml"))

fakeDockerfileApplier.EXPECT().ImageFor(extender.ImageRef).Return(someFakeImage, nil)
firstConfig := &v1.ConfigFile{Config: v1.Config{
User: "0:5678",
}}
someFakeImage.ConfigFileReturns(firstConfig, nil)
someFakeImage.ManifestReturns(&v1.Manifest{Layers: []v1.Descriptor{}}, nil)

fakeDockerfileApplier.EXPECT().Apply(
gomock.Any(),
gomock.Any(),
gomock.Any(),
logger,
).DoAndReturn(
func(dockerfile extend.Dockerfile, toBaseImage v1.Image, withBuildOptions extend.Options, logger llog.Logger) (v1.Image, error) {
h.AssertEq(t, dockerfile.Path, expectedDockerfileA.Path)
h.AssertEq(t, len(dockerfile.Args), 4)
h.AssertEq(t, dockerfile.Args[0].Name, "build_id")
_, err := uuid.Parse(dockerfile.Args[0].Value)
h.AssertNil(t, err)
h.AssertEq(t, dockerfile.Args[1].Name, "user_id")
h.AssertEq(t, dockerfile.Args[1].Value, "0")
h.AssertEq(t, dockerfile.Args[2].Name, "group_id")
h.AssertEq(t, dockerfile.Args[2].Value, "5678")
h.AssertEq(t, dockerfile.Args[3], expectedDockerfileA.Args[0])

return someFakeImage, nil
})

err := extender.Extend("build", logger)
h.AssertError(t, err, "extending build image: the final user ID is 0 (root); please add another extension that resets the user to non-root")
})
})

when("run base image", func() {
Expand Down