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.

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

$KREW_ROOT/store/<plugin>/* has wrong permissions, 0700 instead of 0755 #840

Open
NicolasGoeddel opened this issue Sep 11, 2023 · 18 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@NicolasGoeddel
Copy link

Hi there,

I tried to install krew using a different KREW_ROOT, exactly like documented here: https://krew.sigs.k8s.io/docs/user-guide/advanced-configuration/

I basically ran this command from a sudo user:

sudo KREW_ROOT=/usr/local/krew ./krew-linux_amd64 install krew

After that the binary was not accessible from a normal user. This is because the symlink pointed to a file that was only accessible from root:

ls -l /usr/local/krew/bin/
lrwxrwxrwx 1 root root   38 Sep 11 18:42 kubectl-krew -> /usr/local/krew/store/krew/v0.4.4/krew
ls -l /usr/local/krew/store/krew
total 12
drwx------ 2 root root 4096 Sep 11 18:42 v0.4.4/

After a sudo chmod 750 /usr/local/krew/store/krew/v0.4.4 it worked fine.

Unfortunately this happens with every plugin that is installed this way. The subdirectories in /usr/local/krew/store/*/v* always have the wrong permissions.

For example I installed resource-capacity from the sudo user like this:

sudo PATH="/usr/local/krew/bin:$PATH" KREW_ROOT=/usr/local/krew kubectl krew install resource-capacity

Then the new directory had the wrong permissions again:

sudo ls -l /usr/local/krew/store/resource-capacity
total 4
drwx------ 2 root root 4096 Sep 11 18:54 v0.7.4
@NicolasGoeddel NicolasGoeddel changed the title When using customized installation directory /store/krew/v1.2.3 has wrong permissions When using customized installation directory /store/*/v* has wrong permissions Sep 11, 2023
@ahmetb
Copy link
Member

ahmetb commented Sep 11, 2023

Feel free to dig into the code to figure out why this might be happening. However, I'm not seeing (in a setup with default paths) that .krew/store directories have drwx------.

ls -l $HOME/.krew/store
total 0
drwxr-xr-x@ 3 abalkan  abalkan  96 Jun  7 15:47 access-matrix
drwxr-xr-x@ 3 abalkan  abalkan  96 Aug 16 22:19 ca-cert
drwxr-xr-x@ 3 abalkan  abalkan  96 Jul 14 12:03 deprecations
drwxr-xr-x@ 3 abalkan  abalkan  96 May 23 22:35 edit-status

This might have something to do with sudo command or something else in the picture masking the provided chown flags.

/kind support

@NicolasGoeddel
Copy link
Author

@ahmetb Thanks for your answer.

My Go skills are not that good anymore. I coded something way back in 2011 at university. Can you maybe point me to the right source file that creates these paths? I will also do more tests with that. Maybe directly as root user and not with sudo. Maybe the real and effective UIDs have something to do with that? I don't know, I am just guessing.

@NicolasGoeddel
Copy link
Author

I created a complete script so it can be tested very easily. It downloads the installer, installs krew, shows the content of ${KREW_ROOT}/store/krew and deletes everything again.

#!/bin/bash

KREW_ROOT=/usr/local/krew
KREW_URL="https://github.com/kubernetes-sigs/krew/releases/latest/download/krew-linux_amd64.tar.gz"
KREW_INSTALL_BIN_FILENAME="krew-linux_amd64"


# END OF CONFIG
set -e

if (( "$(id -u)" != 0 )); then
    echo "Please run as root." >&2
    exit 1
fi

TMP_DIR="$(mktemp -d)"
KREW_INSTALL_BIN="${TMP_DIR}/${KREW_INSTALL_BIN_FILENAME}"

# Download
curl -sL "${KREW_URL}" | tar -C "${TMP_DIR}" --gzip --file=- --extract "./${KREW_INSTALL_BIN_FILENAME}" 

export PATH="${KREW_ROOT}/bin:$PATH"
export KREW_ROOT

${KREW_INSTALL_BIN} install krew

ls -la "${KREW_ROOT}/store/krew/"

rm -rf "${TMP_DIR}"
rm -rf "${KREW_ROOT}"

The output on my machine is:

Adding "default" plugin index from https://github.com/kubernetes-sigs/krew-index.git.
Updated the local copy of plugin index.
Installing plugin: krew
Installed plugin: krew
\
 | Use this plugin:
 | 	kubectl krew
 | Documentation:
 | 	https://krew.sigs.k8s.io/
 | Caveats:
 | \
 |  | krew is now installed! To start using kubectl plugins, you need to add
 |  | krew's installation directory to your PATH:
 |  | 
 |  |   * macOS/Linux:
 |  |     - Add the following to your ~/.bashrc or ~/.zshrc:
 |  |         export PATH="${KREW_ROOT:-$HOME/.krew}/bin:$PATH"
 |  |     - Restart your shell.
 |  | 
 |  |   * Windows: Add %USERPROFILE%\.krew\bin to your PATH environment variable
 |  | 
 |  | To list krew commands and to get help, run:
 |  |   $ kubectl krew
 |  | For a full list of available plugins, run:
 |  |   $ kubectl krew search
 |  | 
 |  | You can find documentation at
 |  |   https://krew.sigs.k8s.io/docs/user-guide/quickstart/.
 | /
/
total 12
drwxr-xr-x 3 root root 4096 Sep 13 13:59 .
drwxr-xr-x 3 root root 4096 Sep 13 13:59 ..
drwx------ 2 root root 4096 Sep 13 13:59 v0.4.4

As you can see the directory v0.4.4 has the wrong permissions. It should have at least drwxr-xr-x in my opinion.

@NicolasGoeddel
Copy link
Author

By replacing the line

${KREW_INSTALL_BIN} install krew

with

strace -ff --output ${KREW_INSTALL_BIN_FILENAME} ${KREW_INSTALL_BIN} install krew

I found out that at some point during the installation process a folder created in /tmp simply gets renamed to /usr/local/krew/store/krew/v0.4.4 without setting the permissions again:

mkdirat(AT_FDCWD, "/tmp/krew-temp-move420776093", 0700) = 0
newfstatat(AT_FDCWD, "/tmp/krew-downloads3998705052/krew-linux_amd64", {st_mode=S_IFREG|0755, st_size=12384307, ...}, 0) = 0
newfstatat(AT_FDCWD, "/tmp/krew-temp-move420776093", {st_mode=S_IFDIR|0700, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/tmp/krew-temp-move420776093/krew", 0xc000120858, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/tmp/krew-temp-move420776093/krew", 0xc000120928, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
renameat(AT_FDCWD, "/tmp/krew-downloads3998705052/krew-linux_amd64", AT_FDCWD, "/tmp/krew-temp-move420776093/krew") = 0
newfstatat(AT_FDCWD, "/tmp/krew-downloads3998705052/LICENSE", {st_mode=S_IFREG|0644, st_size=11358, ...}, 0) = 0
newfstatat(AT_FDCWD, "/tmp/krew-temp-move420776093", {st_mode=S_IFDIR|0700, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/tmp/krew-temp-move420776093/LICENSE", 0xc000120d38, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/tmp/krew-temp-move420776093/LICENSE", 0xc000121078, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
renameat(AT_FDCWD, "/tmp/krew-downloads3998705052/LICENSE", AT_FDCWD, "/tmp/krew-temp-move420776093/LICENSE") = 0
newfstatat(AT_FDCWD, "/usr/local/krew/store/krew/v0.4.4", 0xc0001213b8, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/usr/local/krew/store/krew/v0.4.4", 0xc000121898, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
renameat(AT_FDCWD, "/tmp/krew-temp-move420776093", AT_FDCWD, "/usr/local/krew/store/krew/v0.4.4") = 0
unlinkat(AT_FDCWD, "/tmp/krew-temp-move420776093", 0) = -1 ENOENT (No such file or directory)
unlinkat(AT_FDCWD, "/tmp/krew-temp-move420776093", AT_REMOVEDIR) = -1 ENOENT (No such file or directory)

And as you can see in the very first line of that snippet, the temporary folder that later is renamed was created with the permissions 0700. So I don't think it's only an issue on my side. It's clearly a problem of the installation itself.

@NicolasGoeddel
Copy link
Author

NicolasGoeddel commented Sep 13, 2023

I guess the line in question is this one:

        tmp, err := os.MkdirTemp("", "krew-temp-move")

Renaming/copying then happens here:

	err = os.Rename(from, to)
	// Fallback for invalid cross-device link (errno:18).
	if isCrossDeviceRenameErr(err) {
		klog.V(2).Infof("Cross-device link error while copying, fallback to manual copy")
		return errors.Wrap(copyTree(from, to), "failed to copy directory tree as a fallback")
	}

So I think at this point it would be good to also call a os.chmod(to, 0o755) right after.

@NicolasGoeddel
Copy link
Author

The background is that a default mktemp CLI command usually sets 0700 for directories and 0600 for files. And it looks like golang does the same.

@ahmetb
Copy link
Member

ahmetb commented Sep 13, 2023

🤔 but why do dirs in ls -l $HOME/.krew/store have 0o755 right now?

@NicolasGoeddel
Copy link
Author

On my system it has not. I just installed it without setting KREW_ROOT before.

ngoeddel@stretched-b1:~/bin$ ll ~/.krew/store/krew/
total 12
drwxr-xr-x 3 ngoeddel ngoeddel 4096 Sep 13 16:38 ./
drwxr-xr-x 3 ngoeddel ngoeddel 4096 Sep 13 16:38 ../
drwx------ 2 ngoeddel ngoeddel 4096 Sep 13 16:38 v0.4.4/

I can only think of differences in the underlying operating system. Maybe there are settings for creating temporary folders somewhere?

At least I found this statement:

For GLIBC, versions 2.0.6 and earlier, the file is created with permissions 0666; for GLIBC, versions 2.0.7 and later, the file is created with permissions 0600. On NetBSD, the file is created with permissions 0600. This creates a security risk in that an attacker will have write access to the file immediately after creation. Consequently, programs need a private version of the mkstemp() function in which this issue is known to be fixed.

@NicolasGoeddel NicolasGoeddel changed the title When using customized installation directory /store/*/v* has wrong permissions $KREW_ROOT/store/<plugin>/* has wrong permissions, 0700 instead of 0755 Sep 13, 2023
@NicolasGoeddel
Copy link
Author

NicolasGoeddel commented Sep 13, 2023

Btw this is my system:

Distributor ID:	Ubuntu
Description:	Ubuntu 20.04.4 LTS
Release:	20.04
Codename:	focal

@ahmetb
Copy link
Member

ahmetb commented Sep 13, 2023

Ah I see, the dirs directly under store/<plugin> are ok but the dirs like store/<plugin>/<ver> are not. Yeah I think this is a bug.
/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 13, 2023
ahmetb added a commit to ahmetb/krew that referenced this issue Sep 13, 2023
Because we're doing `mktemp` (which gives 0700) and then moving it to
`$KREW_ROOT/store/PLUGIN/VERSION`, we need to make sure that the
directory is 0755 like the other dirs in `store`.

Fixes kubernetes-sigs#840.
ahmetb added a commit to ahmetb/krew that referenced this issue Sep 13, 2023
Because we're doing `mktemp` (which gives 0700) and then moving it to
`$KREW_ROOT/store/PLUGIN/VERSION`, we need to make sure that the
directory is 0755 like the other dirs in `store`.

Fixes kubernetes-sigs#840.
ahmetb added a commit to ahmetb/krew that referenced this issue Sep 13, 2023
Because we're doing `mktemp` (which gives 0700) and then moving it to
`$KREW_ROOT/store/PLUGIN/VERSION`, we need to make sure that the
directory is 0755 like the other dirs in `store`.

Fixes kubernetes-sigs#840.
@NicolasGoeddel
Copy link
Author

Do you know when this will be solved and included in the next release?

@NicolasGoeddel
Copy link
Author

Do you know it now?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2024
@NicolasGoeddel
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 27, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 26, 2024
@ngoeddel-openi
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 26, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 24, 2024
@ngoeddel-openi
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

5 participants