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

Existing Installation Detection Issues #53

Open
rgajason opened this issue Mar 17, 2022 · 1 comment · May be fixed by #54
Open

Existing Installation Detection Issues #53

rgajason opened this issue Mar 17, 2022 · 1 comment · May be fixed by #54
Labels

Comments

@rgajason
Copy link

rgajason commented Mar 17, 2022

Bug report
Issue 1:

Attempting to install SDKMAN to an existing - but empty - directory fails with:

You already have SDKMAN installed.
SDKMAN was found at:

   /path/to/sdkman

Issue 2:

The installation script will also fail if there is a file matching the value of $SDKMAN_DIR:

mkdir: cannot create directory ‘/path/to/sdkman’: Not a directory

(the "Not a directory" error is repeated multiple times in a couple different formats as the installer attempts to keep going)

To reproduce
Issue 1:

mkdir -p /path/to/sdkman
export SDKMAN_DIR=/path/to/sdkman
curl -s "https://get.sdkman.io" | bash

Issue 2:

touch /path/to/sdkman
export SDKMAN_DIR=/path/to/sdkman
curl -s "https://get.sdkman.io" | bash

System info
Linux/Bash - version independent
SDKMAN 5.14.1

These issues are due to the way the installer checks for a pre-existing installation:

echo "Looking for a previous installation of SDKMAN..."
if [ -d "$SDKMAN_DIR" ]; then
...
	echo " You already have SDKMAN installed."
...
	exit 0
fi

The presence of the directory alone doesn't mean that SDKMAN is installed, and -d won't catch if a file is in the way.

Why does this matter?

In short, issue 1 prevents allowing a non-privileged user to install in a privileged location. This is common in Docker images where the user's home directory is overridden at runtime with a volume (to allow for persistence of some artifacts between executions, such as Jenkins jobs). For example, snippet of a Dockerfile for an image used by Jenkins to run containerized agents:

USER root
ENV SDKMAN_DIR=/opt/sdkman
RUN mkdir -p "$SDKMAN_DIR" \
    && chown jenkins: "$SDKMAN_DIR"

USER jenkins
RUN curl -sSL -o- "https://get.sdkman.io" | bash \
    && source "${SDKMAN_DIR}/bin/sdkman-init.sh" \
    && sdk version

The above will fail because /opt/sdkman already exists. The non-privleged "jenkins" user doesn't have the ability to create /opt/sdkman and we don't want to use the default location in $HOME because, when the Docker image is ran, $HOME is overridden with a Docker volume (that doesn't include SDKMAN).

One workaround for issue 1 would be to create a parent directory where the non-privileged user can create sub-directories:

USER root
ENV SDKMAN_DIR=/opt/jenkins/sdkman
RUN mkdir -p /opt/jenkins \
    && chown jenkins: /opt/jenkins

USER jenkins
RUN curl -sSL -o- "https://get.sdkman.io" | bash \
    && source "${SDKMAN_DIR}/bin/sdkman-init.sh" \
    && sdk version

Another workaround for issue 1 would be to run the install as root, but piping scripts from the Internet to a root shell without any sort of SHA/GPG verification is not ideal...

My request is that the installation script is updated with a more robust check to address both issues. This could come in many forms, but a simple version could be to just check whether the SDKMAN_DIR is empty and not a file:

# Note that "-f" could be used but won't catch some edge cases (special files)
if [ -e "$SDKMAN_DIR" ] && [ ! -d "$SDKMAN_DIR" ]; then
    echo "Cannot install to requested location - file of same name exists"
    exit 1
fi
echo "Looking for a previous installation of SDKMAN..."
if [ -d "$SDKMAN_DIR" ] && [ ! -z $(ls -A "$SDKMAN_DIR") ]; then

Additionally, I suggest that the installer resolve paths to their real components (remove symlinks, "..", etc) to avoid potential issues those items can introduce by replacing:

if [ -z "$SDKMAN_DIR" ]; then
    SDKMAN_DIR="$HOME/.sdkman"
    SDKMAN_DIR_RAW='$HOME/.sdkman'
else
    SDKMAN_DIR_RAW="$SDKMAN_DIR"
fi

...with:

if [ -z "$SDKMAN_DIR" ]; then
    SDKMAN_DIR=$(readlink -f "$HOME/.sdkman")
    SDKMAN_DIR_RAW='$HOME/.sdkman'
else
    SDKMAN_DIR_RAW="$SDKMAN_DIR"
    SDKMAN_DIR=$(readlink -f $SDKMAN_DIR)
fi

(the order in the "else" is intentional to preserve user preference post-installation)

@rgajason rgajason added the bug label Mar 17, 2022
@rgajason
Copy link
Author

rgajason commented Mar 17, 2022

It would appear that the installer is actually part of the sdkman-hooks repository. If that is correct, I'm more than happy to move this issue over there and create a PR as well. Please let me know if I'm correct in that assumption based on:

https://github.com/sdkman/sdkman-hooks/blob/master/app/views/install_stable.scala.txt

@marc0der marc0der transferred this issue from sdkman/sdkman-cli Mar 17, 2022
@rgajason rgajason linked a pull request Mar 17, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant