Support HTTP BasicAuth for authentication if $AUTH_USER is set #7143
Conversation
| @@ -0,0 +1,45 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yea, I didn't meant to include this.
| @@ -0,0 +1,15 @@ | |||
| github.copilot | |||
There was a problem hiding this comment.
Could we omit this file? Seems unrelated to code-server itself.
| @@ -0,0 +1,12 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Also seems unrelated to this particular PR.
| @@ -0,0 +1,44 @@ | |||
| #!/bin/bash | |||
| @@ -86,6 +86,7 @@ | |||
| "safe-buffer": "^5.2.1", | |||
| "safe-compare": "^1.1.4", | |||
| "semver": "^7.5.4", | |||
| "tslib": "^2.8.1", | |||
There was a problem hiding this comment.
What was this added for? We have no direct dependency on tslib, do we?
There was a problem hiding this comment.
I will remove it, but I had a packaging issue without it.
| } 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") |
There was a problem hiding this comment.
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")
}
There was a problem hiding this comment.
Ok, I'll incorporate this change
| @@ -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"') | |||
There was a problem hiding this comment.
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"}`
There was a problem hiding this comment.
This is the only place a fresh browser load hits and thus needs this header to prompt the user. I'll update the realm
| @@ -0,0 +1,27 @@ | |||
| #!/bin/bash | |||
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const base64Credentials = authHeader.split(' ')[1]; | ||
| const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8'); | ||
| const [username, password] = credentials.split(':'); | ||
| return username === authUser && password === authPassword; |
There was a problem hiding this comment.
We should use safeCompare instead of === probably.
There was a problem hiding this comment.
Ok, I'll switch the password comparison to safeCompare
Fixes #7142