Support HTTP BasicAuth for authentication if $AUTH_USER is set by gaberudy · Pull Request #7143 · coder/code-server

gaberudy · 2024-12-29T04:49:53Z

code-asher

This is neat, thank you! I know we are not generally adding auth methods, but this seems simple enough to maintain.

code-asher · 2025-01-03T02:45:45Z

build.sh

@@ -0,0 +1,45 @@
+#!/bin/bash


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.

Yea, I didn't meant to include this.

code-asher · 2025-01-03T02:46:11Z

extensions.txt

@@ -0,0 +1,15 @@
+github.copilot


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

yes, unintended

code-asher · 2025-01-03T02:46:33Z

install_locales.sh

@@ -0,0 +1,12 @@
+#!/bin/bash


Also seems unrelated to this particular PR.

yes, unintended

code-asher · 2025-01-03T02:46:41Z

install_system_extensions.sh

@@ -0,0 +1,44 @@
+#!/bin/bash


yes, unintended

code-asher · 2025-01-03T02:47:14Z

package.json

    "safe-buffer": "^5.2.1",
    "safe-compare": "^1.1.4",
    "semver": "^7.5.4",
+    "tslib": "^2.8.1",


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

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

code-asher · 2025-01-03T03:13:01Z

src/node/main.ts

+  } else if (args.auth === AuthType.HttpBasic) {
+    logger.info("  - HTTP basic authentication is enabled")
+    logger.info("    - Using user from $AUTH_USER")
+    logger.info("    - Using password from $PASSWORD")


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") }

Ok, I'll incorporate this change

code-asher · 2025-01-03T03:30:40Z

src/node/routes/vscode.ts

  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"')


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"}`

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

code-asher · 2025-01-03T03:30:57Z

startup.sh

@@ -0,0 +1,27 @@
+#!/bin/bash


Unrelated to the PR?

code-asher · 2025-01-03T03:47:11Z

src/node/http.ts

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


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.

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.

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).

code-asher · 2025-01-03T03:50:53Z

src/node/http.ts

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


We should use safeCompare instead of === probably.

Ok, I'll switch the password comparison to safeCompare

code-asher

Whoops, did not mean to approve quite yet. 😄

gaberudy · 2025-01-20T00:00:22Z

Superceeded by #7173

Support HTTP BasicAuth for authentication if $AUTH_USER is set

d911eac

gaberudy requested a review from a team as a code owner December 29, 2024 04:49

Build scripts for custom Golden Helix build of code-server

cee7031

code-asher approved these changes Jan 3, 2025

View reviewed changes

code-asher requested changes Jan 3, 2025

View reviewed changes

Add code as symlink

6906308

gaberudy mentioned this pull request Jan 19, 2025

Support HTTP BasicAuth for authentication with auth-user argument, password or hashedPassword #7173

Closed

gaberudy closed this Jan 20, 2025

Conversation

gaberudy commented Dec 29, 2024

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaberudy Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

gaberudy commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaberudy Jan 3, 2025 •

edited

Loading