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

Support HTTP BasicAuth for authentication if $AUTH_USER is set #7143

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
**
!release-packages
!ci
!release-standalone
!startup.sh
!install_locales.sh
51 changes: 51 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
ARG SERVER_VERSION=20241223_895aa2908981
FROM registry.goldenhelix.com/gh/server:${SERVER_VERSION}

USER root

# Install various locales to support users from different regions
COPY ./install_locales.sh /tmp/install_locales.sh
RUN bash /tmp/install_locales.sh

# Copy the release-standalone directory
COPY ./release-standalone /opt/code-server
COPY ./startup.sh /opt/code-server/startup.sh

# Remove the existing node binary and create symlink to the system node
RUN rm -f /opt/code-server/lib/node && \
ln -s /opt/node/bin/node /opt/code-server/lib/node && \
chmod +x /opt/code-server/startup.sh

# Set the environment variables
ARG LANG='en_US.UTF-8'
ARG LANGUAGE='en_US:en'
ARG LC_ALL='en_US.UTF-8'
ARG START_XFCE4=1
ARG TZ='Etc/UTC'
ENV HOME=/home/ghuser \
SHELL=/bin/bash \
USERNAME=ghuser \
LANG=$LANG \
LANGUAGE=$LANGUAGE \
LC_ALL=$LC_ALL \
TZ=$TZ \
AUTH_USER=ghuser \
CODE_SERVER_SESSION_SOCKET=/home/ghuser/.config/code-server/code-server-ipc.sock \
PASSWORD=ghuserpassword \
PORT=8080

### Ports and user
EXPOSE $PORT
WORKDIR $HOME
USER 1000

RUN mkdir -p $HOME/.config/code-server && \
echo "bind-addr: 0.0.0.0:8080" > $HOME/.config/code-server/config.yaml && \
echo "auth: password" >> $HOME/.config/code-server/config.yaml && \
echo "password: \${PASSWORD}" >> $HOME/.config/code-server/config.yaml && \
echo "cert: true" >> $HOME/.config/code-server/config.yaml


RUN mkdir -p $HOME/Workspace/Documents

ENTRYPOINT ["/opt/code-server/startup.sh"]
45 changes: 45 additions & 0 deletions build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

This Docker image seems unrelated to the auth changes, could we remove it from this PR? Happy to read about it in another PR/issue, but at first glance I am not sure it is something we should be maintaining here.

Copy link
Author

Choose a reason for hiding this comment

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

Yea, I didn't meant to include this.


# Build a Golden Helix docker image for code-server with changes to support App Streaming on VSWarehouse

# Follow the directions under [CONTRIBUTING.md](docs/CONTRIBUTING.md) to build the image

# git submodule update --init
# quilt push -a
# npm install
# npm run build
# VERSION=4.96.2 npm run build:vscode
# npm run release

# VERSION=4.96.2 npm run package
# cd release
# npm install --omit=dev
# cd ..
# npm run release:standalone

export VERSION=4.96.2

export SERVER_VERSION=20241223_895aa2908981

# Ensure we're in the correct directory
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
cd "$SCRIPT_DIR"

# Temporarily move the bundled node out of the way
if [ -f "release-standalone/lib/node" ]; then
mv release-standalone/lib/node ./node
fi

echo "PWD: $PWD"

docker build --no-cache \
--build-arg SERVER_VERSION=${SERVER_VERSION} \
-t registry.goldenhelix.com/gh/code-server:${VERSION} .

# Move the bundled node back
if [ -f "./node" ]; then
mv ./node release-standalone/lib/node
fi

# Run like
# docker run -it -p 8081:8080 -e PASSWORD=your_secure_password123 -e PORT=8080 registry.goldenhelix.com/gh/code-server:20241223_895aa2908981 /home/ghuser/Workspace/
15 changes: 15 additions & 0 deletions extensions.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
github.copilot
Copy link
Member

Choose a reason for hiding this comment

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

Could we omit this file? Seems unrelated to code-server itself.

Copy link
Author

Choose a reason for hiding this comment

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

yes, unintended

github.copilot-chat
hashicorp.terraform
ms-python.debugpy
ms-python.python
ms-python.vscode-pylance
ms-toolsai.jupyter
ms-toolsai.jupyter-keymap
ms-toolsai.jupyter-renderers
ms-toolsai.vscode-jupyter-cell-tags
ms-toolsai.vscode-jupyter-slideshow
ms-vscode-remote.remote-containers
stkb.rewrap
streetsidesoftware.code-spell-checker
redhat.vscode-yaml
12 changes: 12 additions & 0 deletions install_locales.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Also seems unrelated to this particular PR.

Copy link
Author

Choose a reason for hiding this comment

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

yes, unintended

# *GH* selected major languages for space and time
LOCALES="de_DE fr_FR it_IT es_ES en_GB nl_NL sv_SE pl_PL fr_BE nl_BE de_AT da_DK fi_FI el_GR en_IE pt_PT cs_CZ hu_HU ro_RO bg_BG ja_JP zh_CN ko_KR hi_IN ru_RU"

echo "Installing languages"
apt-get update
apt-get install -y \
locales locales-all
for LOCALE in ${LOCALES}; do
echo "Generating Locale for ${LOCALE}"
localedef -i ${LOCALE} -f UTF-8 ${LOCALE}.UTF-8
done
44 changes: 44 additions & 0 deletions install_system_extensions.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Author

Choose a reason for hiding this comment

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

yes, unintended


EXTENSIONS_FILE="./extensions.txt"
CODE_SERVER="./release-standalone/bin/code-server"
EXTENSIONS_DIR="~/.local/share/code-server/extensions"
TARGET_DIR="./release-standalone/lib/vscode/extensions"

# Check if code-server exists
if [ ! -f "$CODE_SERVER" ]; then
echo "Error: code-server not found at $CODE_SERVER"
exit 1
fi

# Create target directory if it doesn't exist
mkdir -p "$TARGET_DIR"

# Read extensions file line by line
while IFS= read -r extension || [ -n "$extension" ]; do
# Skip empty lines and comments
if [[ -z "$extension" || "$extension" =~ ^# ]]; then
continue
fi

echo "Installing extension: $extension"

# Install the extension
$CODE_SERVER --install-extension "$extension"

if [ $? -ne 0 ]; then
echo "Warning: Failed to install $extension"
continue
fi

echo "Copying extension files to standalone directory"
# Use cp -R with expanded source path
if ! eval "cp -R $EXTENSIONS_DIR/${extension}* $TARGET_DIR/"; then
echo "Warning: Failed to copy $extension to standalone directory"
fi

echo "Completed processing $extension"
echo "----------------------------------------"
done < "$EXTENSIONS_FILE"

echo "All extensions processed"
8 changes: 5 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
"safe-buffer": "^5.2.1",
"safe-compare": "^1.1.4",
"semver": "^7.5.4",
"tslib": "^2.8.1",
Copy link
Member

Choose a reason for hiding this comment

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

What was this added for? We have no direct dependency on tslib, do we?

Copy link
Author

Choose a reason for hiding this comment

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

I will remove it, but I had a packaging issue without it.

"ws": "^8.14.2",
"xdg-basedir": "^4.0.0"
},
Expand Down
10 changes: 10 additions & 0 deletions src/node/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export enum Feature {

export enum AuthType {
Password = "password",
HttpBasic = "http-basic",
None = "none",
}

Expand Down Expand Up @@ -65,6 +66,7 @@ export interface UserProvidedCodeArgs {
export interface UserProvidedArgs extends UserProvidedCodeArgs {
config?: string
auth?: AuthType
"auth-user"?: string
password?: string
"hashed-password"?: string
cert?: OptionalString
Expand Down Expand Up @@ -137,6 +139,10 @@ export type Options<T> = {

export const options: Options<Required<UserProvidedArgs>> = {
auth: { type: AuthType, description: "The type of authentication to use." },
"auth-user": {
type: "string",
description: "The username for http-basic authentication."
},
password: {
type: "string",
description: "The password for password authentication (can only be passed in via $PASSWORD or the config file).",
Expand Down Expand Up @@ -569,6 +575,10 @@ export async function setDefaults(cliArgs: UserProvidedArgs, configArgs?: Config
if (process.env.PASSWORD) {
args.password = process.env.PASSWORD
}
if (process.env.AUTH_USER) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be OK to prepend this with CODE_SERVER_? So CODE_SERVER_AUTH_USER. I wish we had done this with all our env vars to prevent conflicts, but better late than never.

Also, I think typically we prefer in order: command-line, env var, then config file. So this should be:

Suggested change
if (process.env.AUTH_USER) {
if (process.env.AUTH_USER && !cliArgs["auth-user"]) {

(We are not doing this for the password because we disallow setting the password on the command line.)

args["auth"] = AuthType.HttpBasic
Copy link
Member

Choose a reason for hiding this comment

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

I know we are generally not planning on adding any other auth methods, but I do worry about automatically setting --auth=http-basic with --auth-user or AUTH_USER set in case in the future we do want to have some other auth method that needs a username, and then we are unable to use --auth-user for that because it is tied to basic auth and undoing that would be a breaking change. Would it be terrible to make it so you have to explicitly set --auth=http-basic?

It may then also make sense to error if --auth-user is set but --auth is not a valid type (so anything not http-basic for now) and also if --auth=http-basic is set but --auth-user is not set.

args["auth-user"] = process.env.AUTH_USER
}

if (process.env.CS_DISABLE_FILE_DOWNLOADS?.match(/^(1|true)$/)) {
args["disable-file-downloads"] = true
Expand Down
22 changes: 22 additions & 0 deletions src/node/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,25 @@ export const ensureAuthenticated = async (
}
}

/**
* Validate basic auth credentials.
*/
const validateBasicAuth = (authHeader: string | undefined, authUser: string | undefined, authPassword: string | undefined): boolean => {
if (!authHeader?.startsWith('Basic ')) {
return false;
}

try {
const base64Credentials = authHeader.split(' ')[1];
const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8');
const [username, password] = credentials.split(':');
return username === authUser && password === authPassword;
Copy link
Member

Choose a reason for hiding this comment

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

We should use safeCompare instead of === probably.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll switch the password comparison to safeCompare

} catch (error) {
logger.error('Error validating basic auth:' + error);
return false;
}
};

/**
* Return true if authenticated via cookies.
*/
Expand All @@ -132,6 +151,9 @@ export const authenticated = async (req: express.Request): Promise<boolean> => {

return await isCookieValid(isCookieValidArgs)
}
case AuthType.HttpBasic: {
return validateBasicAuth(req.headers.authorization, req.args["auth-user"], req.args.password);
Copy link
Member

Choose a reason for hiding this comment

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

What if someone sets HASHED_PASSWORD but not PASSWORD and then sets auth=basic-auth? I think it would use the config password instead (a default one is always generated), which might be unexpected. We could use handlePasswordValidation to support both HASHED_PASSWORD and PASSWORD, or we could error when HASHED_PASSWORD is set and say it can only be used with PASSWORD.

Copy link
Author

@gaberudy gaberudy Jan 3, 2025

Choose a reason for hiding this comment

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

I don't think we can check HASHED_PASSWORD in this context. The password has to come through standard basic auth format. I think the use case for basic auth is to specify a user and password.

Copy link
Member

Choose a reason for hiding this comment

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

The hashing is just an internal code-server thing, if you want to give code-server the password but not give it the plain text password. The hash is not used by the browser or anything like that, if that makes sense.

Whether we are comparing that hashed password with a password submitted from a login form or a password from the basic auth flow, it is all the same to code-server. Basically, the only difference is argon2.verify(hashedPassword, passwordFromBasicAuth) versus safeCompare(plainTextPassword, passwordFromBasicAuth).

}
default: {
throw new Error(`Unsupported auth type ${req.args.auth}`)
}
Expand Down
4 changes: 4 additions & 0 deletions src/node/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ export const runCodeServer = async (
} else {
logger.info(` - Using password from ${args.config}`)
}
} else if (args.auth === AuthType.HttpBasic) {
logger.info(" - HTTP basic authentication is enabled")
logger.info(" - Using user from $AUTH_USER")
Copy link
Member

Choose a reason for hiding this comment

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

This might be set from --auth-user, not the env var. We could maybe just print the user name if it is not sensitive, but otherwise we could do a similar thing to the password and add a usingEnvAuthUser.

logger.info(" - Using password from $PASSWORD")
Copy link
Member

Choose a reason for hiding this comment

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

The password might be from the config, so we would need to check args.usingEnvPassword (just like the block above). We can maybe combine these blocks. Something like:

  if (args.auth === AuthType.Password || args.auth === AuthType.HttpBasic) {
    logger.info("  - Authentication is enabled")
    if (args.usingEnvPassword) {
      logger.info("    - Using password from $PASSWORD")
    } else if (args.usingEnvHashedPassword) {
      logger.info("    - Using password from $HASHED_PASSWORD")
    } else {
      logger.info(`    - Using password from ${args.config}`)
    }
    if (args.auth === AuthType.HttpBasic) {
      logger.info("    - With user ${args["auth-user"]}") // or add an args.usingEnvAuthUser and switch on it as appropriate
    }
  } else {
    logger.info("  - Authentication is disabled")
  }

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll incorporate this change

} else {
logger.info(" - Authentication is disabled")
}
Expand Down
5 changes: 5 additions & 0 deletions src/node/routes/domainProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { HttpCode, HttpError } from "../../common/http"
import { getHost, ensureProxyEnabled, authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
import { proxy } from "../proxy"
import { Router as WsRouter } from "../wsRouter"
import { AuthType } from "../cli"

export const router = Router()

Expand Down Expand Up @@ -78,6 +79,10 @@ router.all(/.*/, async (req, res, next) => {
if (/\/login\/?/.test(req.path)) {
return next()
}
// If auth is HttpBasic, return a 401.
if (req.args.auth === AuthType.HttpBasic) {
throw new HttpError("Unauthorized", HttpCode.Unauthorized)
}
// Redirect all other pages to the login.
const to = self(req)
return redirect(req, res, "login", {
Expand Down
3 changes: 2 additions & 1 deletion src/node/routes/pathProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as pluginapi from "../../../typings/pluginapi"
import { HttpCode, HttpError } from "../../common/http"
import { ensureProxyEnabled, authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
import { proxy as _proxy } from "../proxy"
import { AuthType } from "../cli"

const getProxyTarget = (
req: Request,
Expand All @@ -28,7 +29,7 @@ export async function proxy(

if (!(await authenticated(req))) {
// If visiting the root (/:port only) redirect to the login page.
if (!req.params.path || req.params.path === "/") {
if ((!req.params.path || req.params.path === "/") && req.args.auth !== AuthType.HttpBasic) {
const to = self(req)
return redirect(req, res, "login", {
to: to !== "/" ? to : undefined,
Expand Down
8 changes: 7 additions & 1 deletion src/node/routes/vscode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import * as net from "net"
import * as path from "path"
import { WebsocketRequest } from "../../../typings/pluginapi"
import { logError } from "../../common/util"
import { CodeArgs, toCodeArgs } from "../cli"
import { AuthType, CodeArgs, toCodeArgs } from "../cli"
import { isDevMode, vsRootPath } from "../constants"
import { authenticated, ensureAuthenticated, ensureOrigin, redirect, replaceTemplates, self } from "../http"
import { SocketProxyProvider } from "../socket"
import { isFile } from "../util"
import { Router as WsRouter } from "../wsRouter"
import { HttpCode, HttpError } from "../../common/http"

export const router = express.Router()

Expand Down Expand Up @@ -118,6 +119,11 @@ router.get("/", ensureVSCodeLoaded, async (req, res, next) => {
const FOLDER_OR_WORKSPACE_WAS_CLOSED = req.query.ew

if (!isAuthenticated) {
// If auth is HttpBasic, return a 401.
if (req.args.auth === AuthType.HttpBasic) {
res.setHeader('WWW-Authenticate', 'Basic realm="Access to the site"')
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set this header on the other places where we throw unauthorized for basic auth? And in the ensureAuthenticated middleware as well.

Also, could be cool to put the name in the realm, maybe:

`Basic realm=${req.args["app-name"] || "code-server"}`

Copy link
Author

Choose a reason for hiding this comment

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

This is the only place a fresh browser load hits and thus needs this header to prompt the user. I'll update the realm

throw new HttpError("Unauthorized", HttpCode.Unauthorized)
};
const to = self(req)
return redirect(req, res, "login", {
to: to !== "/" ? to : undefined,
Expand Down
27 changes: 27 additions & 0 deletions startup.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the PR?

Copy link
Author

Choose a reason for hiding this comment

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

Yes


# Try to set up the private (per user) User Data folder
set +e
mkdir -p $HOME/Workspace/Documents/$USERNAME/code-server
export XDG_DATA_HOME=$HOME/Workspace/Documents/$USERNAME
set -e

echo 'export PS1="$USERNAME:\w\$ "' >> $HOME/.bashrc

# Set the default project folder
DEFAULT_PROJECT_FOLDER="$HOME/Workspace/"

# Use the provided PROJECT_FOLDER or default to DEFAULT_PROJECT_FOLDER
STARTING_FOLDER="${PROJECT_FOLDER:-$DEFAULT_PROJECT_FOLDER}"

# Your script logic here
echo "Starting in folder: $STARTING_FOLDER"

/opt/code-server/bin/code-server \
--disable-telemetry \
--disable-update-check \
--disable-workspace-trust \
--locale=$LANG \
--welcome-text="Welcome to your Golden Helix VSCode environment" \
--ignore-last-opened \
$STARTING_FOLDER
Loading