Skip to content

Catch invalid root user kickstart options early [HMS-9378] #1879

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
supakeen merged 6 commits into from
Oct 9, 2025
Merged

Catch invalid root user kickstart options early [HMS-9378] #1879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
37343ed
osbuild: add tests for the various kickstart constructors
achilleas-k Sep 17, 2025
f3482f3
osbuild: validate kickstart path option
achilleas-k Sep 17, 2025
3e7e28c
manifest: test AnacondaISOTree variants with kickstart users
achilleas-k Sep 17, 2025
adb7ff2
manifest: fix error message for tar payload stage creation failure
achilleas-k Sep 22, 2025
eede93b
customizations/kickstart: test the kickstart options constructor
achilleas-k Sep 17, 2025
4f76acf
customizations/kickstart: validate root user options
achilleas-k Sep 17, 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
36 changes: 36 additions & 0 deletions pkg/customizations/kickstart/kickstart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package kickstart

import (
"fmt"
"strings"

"github.com/osbuild/blueprint/pkg/blueprint"
"github.com/osbuild/images/pkg/customizations/users"
Expand Down Expand Up @@ -89,5 +90,40 @@ func (options Options) Validate() error {
return fmt.Errorf("kickstart users and/or groups are not compatible with user-supplied kickstart content")
}
}

// This check repeats the same checks that are made in the kickstart stage

@mvo5 mvo5 Sep 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quick drive-by: can move the checks into a shared helper somehow maybe? It worries me that we have two places now that (almost) do the same thing, it will most likely cause drift if/when we add more checks to one place.

Or we could look at the root of the problem and make serialize() return an error (probably a much bigger PR though)

@achilleas-k achilleas-k Sep 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Quick drive-by: can move the checks into a shared helper somehow maybe? It worries me that we have two places now that (almost) do the same thing, it will most likely cause drift if/when we add more checks to one place.

Yeah I mentioned as much in the PR description, but I couldn't think of an obvious way to do it nicely since they're different types.

Or we could look at the root of the problem and make serialize() return an error (probably a much bigger PR though)

Yes, we should get on this.

// constructor. Repeating it here catches it earlier in the pipeline
// generation, before serialization starts.
for _, user := range options.Users {
if user.Name == "root" {
// return an error if any other field is set (except SSH)
unsupportedOptionsSet := make([]string, 0, 7)
if user.ExpireDate != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "expiredate")
}
if user.ForcePasswordReset != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "force_password_reset")
}
if user.GID != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "gid")
}
if user.Groups != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "groups")
}
if user.Home != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "home")
}
if user.Shell != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "shell")
}
if user.UID != nil {
unsupportedOptionsSet = append(unsupportedOptionsSet, "uid")
}
if len(unsupportedOptionsSet) > 0 {
return fmt.Errorf("unsupported kickstart options for user \"root\": %s", strings.Join(unsupportedOptionsSet, ", "))
}
}
}

return nil
}
159 changes: 159 additions & 0 deletions pkg/customizations/kickstart/kickstart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
package kickstart_test

import (
"testing"

"github.com/osbuild/blueprint/pkg/blueprint"
"github.com/osbuild/images/internal/common"
"github.com/osbuild/images/pkg/customizations/kickstart"
"github.com/osbuild/images/pkg/customizations/users"
"github.com/stretchr/testify/assert"
)

func TestNewKickstart(t *testing.T) {
type testCase struct {
customizations *blueprint.Customizations

expOptions *kickstart.Options
expErr string
}

testCases := map[string]testCase{
"empty": {
expOptions: &kickstart.Options{
Users: []users.User{},
Groups: []users.Group{},
},
},

"users+groups": {
// we don't need to extensively test the whole blueprint.User ->
// users.User and blueprint.Group -> users.Group conversion here,
// so just one test case is enough
customizations: &blueprint.Customizations{
User: []blueprint.UserCustomization{
{
Name: "alice",
Key: common.ToPtr("ssh-ed25519 iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii"),
Home: common.ToPtr("/var/home/alice"),
},
{
Name: "bob",
Key: common.ToPtr("ssh-ed25519 eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"),
Home: common.ToPtr("/var/home/bob"),
},
},
Group: []blueprint.GroupCustomization{
{
Name: "data",
GID: common.ToPtr(2010),
},
{
Name: "datarw",
GID: common.ToPtr(2020),
},
},
},

expOptions: &kickstart.Options{
Users: []users.User{
{
Name: "alice",
Key: common.ToPtr("ssh-ed25519 iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii"),
Home: common.ToPtr("/var/home/alice"),
},
{
Name: "bob",
Key: common.ToPtr("ssh-ed25519 eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"),
Home: common.ToPtr("/var/home/bob"),
},
},
Groups: []users.Group{
{
Name: "data",
GID: common.ToPtr(2010),
},
{
Name: "datarw",
GID: common.ToPtr(2020),
},
},
},
},

"installer-customizations-kickstart-contents": {
customizations: &blueprint.Customizations{
Installer: &blueprint.InstallerCustomization{
Kickstart: &blueprint.Kickstart{
Contents: "echo 'Hello!!'",
},
},
},

expOptions: &kickstart.Options{
Users: []users.User{},
Groups: []users.Group{},
UserFile: &kickstart.File{
Contents: "echo 'Hello!!'",
},
},
},

"installer-customizations-unattended": {
customizations: &blueprint.Customizations{
Installer: &blueprint.InstallerCustomization{
Unattended: true,
},
},

expOptions: &kickstart.Options{
Users: []users.User{},
Groups: []users.Group{},
Unattended: true,
},
},

"error/installer-customizations-kickstart+unattended": {
customizations: &blueprint.Customizations{
Installer: &blueprint.InstallerCustomization{
Kickstart: &blueprint.Kickstart{
Contents: "echo 'Hello!!'",
},
Unattended: true,
},
},

expErr: "installer.unattended is not supported when adding custom kickstart contents",
},

"error/unsupported-root-options": {
customizations: &blueprint.Customizations{
User: []blueprint.UserCustomization{
{
Name: "root",
Home: common.ToPtr("/megaroot"),
Shell: common.ToPtr("/bin/zsh"),
Groups: []string{"cups"},
},
},
},

expErr: "unsupported kickstart options for user \"root\": groups, home, shell",
},
}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
assert := assert.New(t)

options, err := kickstart.New(tc.customizations)
if tc.expErr != "" {
assert.EqualError(err, tc.expErr)
return
}

assert.NoError(err)
assert.Equal(tc.expOptions, options)
})
}
}
2 changes: 1 addition & 1 deletion pkg/manifest/anaconda_installer_iso_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ func (p *AnacondaInstallerISOTree) serialize() (osbuild.Pipeline, error) {
case p.OSPipeline != nil:
tarPayloadStages, err := p.tarPayloadStages()
if err != nil {
return osbuild.Pipeline{}, fmt.Errorf("cannot create ostree container stages: %w", err)
return osbuild.Pipeline{}, fmt.Errorf("cannot create tar payload stages: %w", err)
}
pipeline.AddStages(tarPayloadStages...)
default:
Expand Down
99 changes: 97 additions & 2 deletions pkg/manifest/anaconda_installer_iso_tree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/osbuild/images/pkg/arch"
"github.com/osbuild/images/pkg/container"
"github.com/osbuild/images/pkg/customizations/kickstart"
"github.com/osbuild/images/pkg/customizations/users"
"github.com/osbuild/images/pkg/datasizes"
"github.com/osbuild/images/pkg/disk"
"github.com/osbuild/images/pkg/manifest"
Expand Down Expand Up @@ -447,7 +448,7 @@ func TestAnacondaISOTreeSerializeWithOS(t *testing.T) {
}
pipeline.ISOBoot = manifest.SyslinuxISOBoot
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{})
assert.EqualError(t, err, "cannot create ostree container stages: cannot create kickstart stages: kickstart unattended options are not compatible with user-supplied kickstart content")
assert.EqualError(t, err, "cannot create tar payload stages: cannot create kickstart stages: kickstart unattended options are not compatible with user-supplied kickstart content")
})

t.Run("unhappy/user-kickstart-with-sudo-bits", func(t *testing.T) {
Expand All @@ -463,7 +464,7 @@ func TestAnacondaISOTreeSerializeWithOS(t *testing.T) {
}
pipeline.ISOBoot = manifest.SyslinuxISOBoot
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{})
assert.EqualError(t, err, "cannot create ostree container stages: cannot create kickstart stages: kickstart sudo nopasswd drop-in file creation is not compatible with user-supplied kickstart content")
assert.EqualError(t, err, "cannot create tar payload stages: cannot create kickstart stages: kickstart sudo nopasswd drop-in file creation is not compatible with user-supplied kickstart content")
})

t.Run("plain+squashfs-rootfs", func(t *testing.T) {
Expand All @@ -488,6 +489,38 @@ func TestAnacondaISOTreeSerializeWithOS(t *testing.T) {
append(variantStages, []string{"org.osbuild.kickstart", "org.osbuild.isolinux", "org.osbuild.squashfs"}...)),
dumpStages(sp.Stages))
})

t.Run("happy/kickstart-with-users", func(t *testing.T) {
pipeline := newTestAnacondaISOTree()
pipeline.OSPipeline = osPayload
pipeline.Kickstart = &kickstart.Options{
Path: testKsPath,
Users: []users.User{
{
Name: "me",
Shell: common.ToPtr("/bin/true"),
},
},
}
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{})
assert.NoError(t, err)
})

t.Run("unhappy/kickstart-with-root-options", func(t *testing.T) {
pipeline := newTestAnacondaISOTree()
pipeline.OSPipeline = osPayload
pipeline.Kickstart = &kickstart.Options{
Path: testKsPath,
Users: []users.User{
{
Name: "root",
Shell: common.ToPtr("/bin/true"),
},
},
}
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{})
assert.EqualError(t, err, "cannot create tar payload stages: failed to create kickstart stage options: org.osbuild.kickstart: unsupported options for user \"root\": shell")
})
}

func TestAnacondaISOTreeSerializeWithOSTree(t *testing.T) {
Expand Down Expand Up @@ -624,6 +657,38 @@ func TestAnacondaISOTreeSerializeWithOSTree(t *testing.T) {
append(variantStages, []string{"org.osbuild.isolinux", "org.osbuild.squashfs"}...)),
dumpStages(sp.Stages))
})

t.Run("happy/kickstart-with-users", func(t *testing.T) {
pipeline := newTestAnacondaISOTree()
pipeline.Kickstart = &kickstart.Options{
Path: testKsPath,
Users: []users.User{
{
Name: "me",
Shell: common.ToPtr("/bin/true"),
},
},
OSTree: &kickstart.OSTree{},
}
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{Commits: []ostree.CommitSpec{ostreeCommit}})
assert.NoError(t, err)
})

t.Run("unhappy/kickstart-with-root-options", func(t *testing.T) {
pipeline := newTestAnacondaISOTree()
pipeline.Kickstart = &kickstart.Options{
Path: testKsPath,
Users: []users.User{
{
Name: "root",
Shell: common.ToPtr("/bin/true"),
},
},
OSTree: &kickstart.OSTree{},
}
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{Commits: []ostree.CommitSpec{ostreeCommit}})
assert.EqualError(t, err, "cannot create ostree commit stages: failed to create kickstart stage options: org.osbuild.kickstart: unsupported options for user \"root\": shell")
})
}

func makeFakeContainerPayload() container.Spec {
Expand Down Expand Up @@ -743,6 +808,36 @@ func TestAnacondaISOTreeSerializeWithContainer(t *testing.T) {
append(variantStages, []string{"org.osbuild.isolinux", "org.osbuild.squashfs"}...)),
dumpStages(sp.Stages))
})

t.Run("happy/kickstart-with-users", func(t *testing.T) {
pipeline := newTestAnacondaISOTree()
pipeline.Kickstart = &kickstart.Options{
Path: testKsPath,
Users: []users.User{
{
Name: "me",
Shell: common.ToPtr("/bin/true"),
},
},
}
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{Containers: []container.Spec{containerPayload}})
assert.NoError(t, err)
})

t.Run("unhappy/kickstart-with-root-options", func(t *testing.T) {
pipeline := newTestAnacondaISOTree()
pipeline.Kickstart = &kickstart.Options{
Path: testKsPath,
Users: []users.User{
{
Name: "root",
Shell: common.ToPtr("/bin/true"),
},
},
}
_, err := manifest.SerializeWith(pipeline, manifest.Inputs{Containers: []container.Spec{containerPayload}})
assert.EqualError(t, err, "cannot create ostree container stages: cannot generate bootc installer kickstart stages: failed to create kickstart stage options: org.osbuild.kickstart: unsupported options for user \"root\": shell")
})
}

func TestMakeKickstartSudoersPost(t *testing.T) {
Expand Down
Loading
Loading