fix: [sc-118372] /var/lib/embedded-cluster is not exec by other non-root user #1729

nvanthao · 2025-01-23T02:54:55Z

Story details: https://app.shortcut.com/replicated/story/118372

Problem

When creating directories using os.MkdirAll, the final permissions are affected by the system's umask. This behavior is undesirable because it can lead to permission issues when other processes attempt to access or modify files in our data directory (/var/lib/embedded-cluster). Specifically, directories may end up with restrictive permissions that prevent necessary access by other services or processes.

Solution

To ensure consistent and correct directory permissions, this PR sets the umask to 022 before beginning execution of the install, restore, or join commands.

Demo

Before: https://asciinema.org/a/6BIISglNrpFdvONP03d683q15

ls -lah /var/lib/embedded-cluster/                                                                                  
total 24K                                                                                                                                            
drwx------  6 root root 4.0K Jan 22 23:54 .                                                                                                          
drwxr-xr-x 39 root root 4.0K Jan 22 23:54 ..                                                                                                         
drwx------  2 root root 4.0K Jan 22 23:54 bin                                                                                                        
drwx------  3 root root 4.0K Jan 22 23:54 k0s                                                                                                        
drwx------  2 root root 4.0K Jan 22 23:54 support                                                                                                    
drwx------  2 root root 4.0K Jan 22 23:54 tmp

After: https://asciinema.org/a/GZCkeoOtPuW0uOPhCDP6EuXdc

ls -lah /var/lib/embedded-cluster/
total 36K
drwxr-xr-x 6 root root 4.0K Jan 23 02:46 .
drwxr-xr-x 1 root root 4.0K Jan 23 02:20 ..
drwxr-xr-x 2 root root 4.0K Jan 23 02:46 bin
drwxr-xr-x 3 root root 4.0K Jan 23 02:46 k0s
drwx------ 2 root root 4.0K Jan 23 02:46 support
drwxr-xr-x 2 root root 4.0K Jan 23 02:46 tmp

github-actions · 2025-01-23T03:10:28Z

This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID.

Online Installer:

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci/appver-dev-15d4856" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Airgap Installer (may take a few minutes before the airgap bundle is built):

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci-airgap/appver-dev-15d4856?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

pkg/helpers/fs.go

sgalsaleh

There's many other places in the code that use os.MkdirAll that I think we should change as well

pkg/helpers/fs.go

nvanthao · 2025-01-23T23:41:09Z

I've added more dir but only for dirs under EmbeddedClusterHomeDirectory as it could be use by other users created by k0s.

banjoh · 2025-02-04T13:46:37Z

I think we should modify these as well since they are code paths in the install workflow

embedded-cluster/pkg/runtimeconfig/defaults.go

Line 36 in 381a48e

func EmbeddedClusterLogsSubDir() string {
embedded-cluster/pkg/airgap/materialize.go

Line 63 in 381a48e

if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil {

@emosbaugh / @sgalsaleh is there any harm in applying this change to all os.MkdirAll in the codebase?

sgalsaleh · 2025-02-04T16:01:47Z

@banjoh no, that should be fine I think.

banjoh

I ran an install using a binary built from this PR but the install failed. Here is my install attempt

root@evans-vm1:/home/evans# umask 0077
root@evans-vm1:/home/evans# ./embedded-cluster install
? Set the Admin Console password (minimum 6 characters): ******
? Confirm the Admin Console password: ******
✔  Host files materialized!
✔  Host preflights succeeded!
✔  Waiting for embedded-cluster node to be ready
unable to wait for node: unable to get status: exit status 1: Error: status: can't get "status" via "/run/k0s/status.sock": Get "http://localhost/status": dial unix /run/k0s/status.sock: connect: connection refused

I picked a number of paths where the permissions were not right such as

drwx------ root /var/lib/embedded-cluster/
All files in /var/lib/embedded-cluster/k0s/bin have -r-xr-x--- root. They do not have global execute permission

Since the install failed very early on, there might have been some other paths not set right later on.

Changes needed

I addition to the changes here, I think we should consider setting 0022 umask before the install/join starts using unix.Umask(0022). This will be a good safeguard to ensure we do not miss created files as well. This PR just addresses directories.
We need to test joining a new node.
We need cover install and joining in e2e tests where the umask used is 0077. I choose this umask one since its what recommendations such as STIG/CIS document.

sgalsaleh · 2025-02-05T17:31:13Z

@banjoh did you resolve conflicts first before you built the binary?

banjoh · 2025-02-05T17:43:10Z

@banjoh did you resolve conflicts first before you built the binary?

Hmm. No. Good catch. I'll do that then run another test.

banjoh · 2025-02-05T18:17:37Z

            umask := 0022
            oldUmask := unix.Umask(umask)
            if oldUmask != umask {
                logrus.Debugf("seting umask to %04o from %04o", umask, oldUmask)
            }

@nvanthao I added this change to func RootCmd(ctx context.Context, name string) *cobra.Command to test working how the install works. It succeeded with your last commit (before the merge done above - git checkout 1d31f87d1a35d7415fcfe913315fc91e8b23d7fa)

laverya · 2025-02-05T22:45:42Z

    install_test.go:1071: set umask stdout: 'umask 0077' >> /etc/profile
    install_test.go:1072: stderr: 
    install_test.go:1078: check umask stdout: 0022
    install_test.go:1079: stderr:

evidently my test did not work

tests/dryrun/install_test.go

sgalsaleh · 2025-02-12T18:53:46Z

cmd/installer/cli/root.go

@@ -68,6 +69,9 @@ func RootCmd(ctx context.Context, name string) *cobra.Command {
 				metrics.DisableMetrics()
 			}

+			// set the umask to 022 so that we can create files/directories with 755 permissions
+			_ = syscall.Umask(0o022)


i assume having this in root isn't sufficient, can you please add a comment to the other ones explaining why?

also, are there no other commands that generate dirs and files other than install, join, and restore? what about materialize? isn't it used by LAM? should we set umask for materialize and LAM cli commands as well? operator cli?

I'll add it to the various 'main' functions, and try to come up with a better comment for it

emosbaugh · 2025-02-12T22:23:37Z

cmd/installer/cli/install.go

@@ -171,6 +172,9 @@ func preRunInstall(cmd *cobra.Command, flags *InstallCmdFlags) error {
 		return fmt.Errorf("install command must be run as root")
 	}

+	// set the umask to 022 so that we can create files/directories with 755 permissions
+	_ = syscall.Umask(0o022)


can you at the very least logrus.Debug this error?

it doesn't return an error, it returns the previous umask value

emosbaugh · 2025-02-12T22:23:48Z

cmd/installer/cli/join.go

@@ -96,6 +97,9 @@ func preRunJoin(flags *JoinCmdFlags) error {

 	flags.isAirgap = flags.airgapBundle != ""

+	// set the umask to 022 so that we can create files/directories with 755 permissions
+	_ = syscall.Umask(0o022)


logrus.Debug the error

emosbaugh · 2025-02-12T22:24:07Z

cmd/installer/cli/root.go

@@ -68,6 +69,9 @@ func RootCmd(ctx context.Context, name string) *cobra.Command {
 				metrics.DisableMetrics()
 			}

+			// set the umask to 022 so that we can create files/directories with 755 permissions
+			_ = syscall.Umask(0o022)


logrus.Debug the error

The return value is not an error. Its the previous umask.

apply chmod after mkdir for directories in --data-dir

eebfecf

nvanthao added type::bug bug::normal labels Jan 23, 2025

sgalsaleh reviewed Jan 23, 2025

View reviewed changes

pkg/helpers/fs.go Outdated Show resolved Hide resolved

sgalsaleh reviewed Jan 23, 2025

View reviewed changes

emosbaugh reviewed Jan 23, 2025

View reviewed changes

pkg/helpers/fs.go Outdated Show resolved Hide resolved

nvanthao added 2 commits January 24, 2025 10:19

address code reviews

c5e7097

add more dir

d115dba

nvanthao requested review from sgalsaleh and emosbaugh January 23, 2025 23:41

update os.MkdirAll in all places

1d31f87

banjoh requested changes Feb 5, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into gerard/sc-118372/chmod

841e54a

laverya added 3 commits February 5, 2025 15:24

potentially fix airgap upgrade job

67f728a

attempt setting umask 0077 in CI

1bb7a9f

use bash to run umask

2ddbf46

remove e2e test in favor of future dryrun test

c9fe8c3

banjoh force-pushed the gerard/sc-118372/chmod branch from d4d863b to c9fe8c3 Compare February 6, 2025 12:32

laverya added 4 commits February 6, 2025 10:48

set restrictive umask before running a dryrun test

13ebcf3

f

6563885

test that this does not pass without new code

59bfc2d

properly set umask

98efb3c

laverya added 13 commits February 6, 2025 11:11

return to using new code

a06c81b

octal prints, no new code

80af595

reenable new code

646db33

check all folders even after failure

67ba616

update test to match behavior

e5d2a27

Merge remote-tracking branch 'origin/main' into gerard/sc-118372/chmod

acec990

set umask in root command

fbc4e04

return to os.MkdirAll

79db89b

printf debugging

048b806

test without setting umask to 077 first

c5b087f

check if the setting actually stuck

1d7ba6a

call umask in each prerun function

83b1132

remove debug logs

ce907b8

banjoh reviewed Feb 12, 2025

View reviewed changes

tests/dryrun/install_test.go Show resolved Hide resolved

check that the kubectl-preflight binary has appropriate permissions

fc169e7

banjoh previously approved these changes Feb 12, 2025

View reviewed changes

sgalsaleh reviewed Feb 12, 2025

View reviewed changes

emosbaugh requested changes Feb 12, 2025

View reviewed changes

set umask in main function, improve comments

15d4856

laverya dismissed banjoh’s stale review via 15d4856 February 17, 2025 17:00

emosbaugh approved these changes Feb 17, 2025

View reviewed changes

laverya enabled auto-merge (squash) February 18, 2025 16:26

laverya merged commit 0ac84ec into main Feb 18, 2025
65 checks passed

laverya deleted the gerard/sc-118372/chmod branch February 18, 2025 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [sc-118372] /var/lib/embedded-cluster is not exec by other non-root user #1729

fix: [sc-118372] /var/lib/embedded-cluster is not exec by other non-root user #1729

nvanthao commented Jan 23, 2025 •

edited by laverya

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

sgalsaleh left a comment

nvanthao commented Jan 23, 2025

banjoh commented Feb 4, 2025

sgalsaleh commented Feb 4, 2025

banjoh left a comment •

edited

Loading

sgalsaleh commented Feb 5, 2025

banjoh commented Feb 5, 2025

banjoh commented Feb 5, 2025

laverya commented Feb 5, 2025

sgalsaleh Feb 12, 2025

laverya Feb 12, 2025

emosbaugh Feb 12, 2025

laverya Feb 12, 2025

emosbaugh Feb 12, 2025

emosbaugh Feb 12, 2025

banjoh Feb 14, 2025

fix: [sc-118372] /var/lib/embedded-cluster is not exec by other non-root user #1729

fix: [sc-118372] /var/lib/embedded-cluster is not exec by other non-root user #1729

Conversation

nvanthao commented Jan 23, 2025 • edited by laverya Loading

Problem

Solution

Demo

github-actions bot commented Jan 23, 2025 • edited Loading

sgalsaleh left a comment

Choose a reason for hiding this comment

nvanthao commented Jan 23, 2025

banjoh commented Feb 4, 2025

sgalsaleh commented Feb 4, 2025

banjoh left a comment • edited Loading

Choose a reason for hiding this comment

Changes needed

sgalsaleh commented Feb 5, 2025

banjoh commented Feb 5, 2025

banjoh commented Feb 5, 2025

laverya commented Feb 5, 2025

sgalsaleh Feb 12, 2025

Choose a reason for hiding this comment

laverya Feb 12, 2025

Choose a reason for hiding this comment

emosbaugh Feb 12, 2025

Choose a reason for hiding this comment

laverya Feb 12, 2025

Choose a reason for hiding this comment

emosbaugh Feb 12, 2025

Choose a reason for hiding this comment

emosbaugh Feb 12, 2025

Choose a reason for hiding this comment

banjoh Feb 14, 2025

Choose a reason for hiding this comment

nvanthao commented Jan 23, 2025 •

edited by laverya

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

banjoh left a comment •

edited

Loading