sdkman / sdkman-hooks

An API responsible for serving up pre- and post- hooks
Other
5 stars 23 forks source link

Existing Installation Detection Issues #53

Open rgajason opened 2 years ago

rgajason commented 2 years ago

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 commented 2 years ago

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