Feature/openid connect #3093

dylanturn · 2021-04-10T18:16:17Z

Adds OIDC support without needing sidecars/proxies.

Resolves: #2636

jawnsy

Hey there,

Thanks so much for your contribution! This looks really great, but I'm not one of the primary maintainers of code-server, so I'll leave the review to others 😄

jawnsy · 2021-04-10T18:23:56Z

src/node/http.ts

@@ -69,6 +71,29 @@ export const authenticated = (req: express.Request): boolean => {
          ? safeCompare(req.cookies.key, req.args["hashed-password"])
          : req.args.password && safeCompare(req.cookies.key, hash(req.args.password)))
      )
+    case AuthType.Openid:
+      console.log(req.oidc.user)


It looks like we might want to use console.debug and add some other text here? It's definitely helpful to have debug output as figuring out OIDC integration problems can be... a challenge 😅 but we'd some other text that we can search for, so we know where the logs are coming from.

maybe something like:

console.debug("authenticated using OpenID Connect as", req.oidc.user)

other debug info like the returned OIDC claims seems useful too? they're secret credentials, but since code-server is meant for a single user, maybe logging those is OK?

Yes, very sorry, I'll include this change along with the updates to yarn lockfile.

No apologies needed! Thanks so much for your contribution! ❤️

code-asher · 2021-04-12T16:38:08Z

Thanks for the contribution! I think we'll need to discuss whether we want to support more authentication methods. The main reason we've been against it is that we felt it wasn't worth the maintenance effort when you could just use a purpose-built proxy.

For example, I personally wish we had recommended an external reverse proxy instead of building one into code-server so I'm reluctant to do a similar thing here.

But I'm definitely curious to everyone's thoughts and this seems to have been requested quite a few times so the desire certainly seems to be there. So maybe we put it to a vote or something.

@jsjoeio @oxy @jawnsy

oxy · 2021-04-12T19:48:55Z

Hi there! I'm not really against the PR and I think its a great addition! My only concern is a lack of documentation around how to set up OpenID authentication, what the secrets are, how it would work...

I've never touched OpenID before, and in my quick Googling it didn't sound really easy to me either - so I'd really appreciate if you could also write some documentation on how to set it up 😅

dylanturn · 2021-04-12T21:28:56Z

Hi there! I'm not really against the PR and I think its a great addition! My only concern is a lack of documentation around how to set up OpenID authentication, what the secrets are, how it would work...

I've never touched OpenID before, and in my quick Googling it didn't sound really easy to me either - so I'd really appreciate if you could also write some documentation on how to set it up 😅

Funny you should mention that, I'm busy writing documentation that explains how to setup an OpenID application on Auth0, Okta, and Keycloak. I've also been considering a small code update that would make the openid-group-claim argument optional, simplifying the OpenID application configuration process for those who don't require group/role based access.

jsjoeio · 2021-04-13T17:05:32Z

First, I just want to say thanks for the contribution! We really appreciate your input and and want to encourage more people to contribute to code-server.

Echoing @code-asher, biggest concern with something like this is:

What precedent does it establish for folks who want other types of authentication methods in code-server?
What would the maintenance for something like this look like for us a maintainers?

I wish we had some type of plugin ecosystem so that people could build on top of code-server, which would be a nice alternative.

I think we'll need to discuss this as a group before we decide what to do. In the meantime, thank you for being patient!

dylanturn · 2021-04-14T15:21:25Z

I'm not really sure what my part in this conversation is meant to be, but for what it's worth here are my thoughts on the two concerns you raised.

What precedent does it establish for folks who want other types of authentication methods in code-server?

Would adding LDAP/SAML/Kerberos authentication be problematic provided the implementations were simple and leveraged well known and trusted libraries?

What would the maintenance for something like this look like for us a maintainers?

Given that the implementation uses Auth0s express-openid-connect library and only consists of around 100 lines of actual logic, I would imagine this would be maintained like any of the other functionality that leverages 3rd party libraries.

Judging by the contents of typings/pluginapi.d.ts and typings/ipc.d.ts it looks like there has been work done to create some kind of plugin system. That said, from what I can tell the implemented plugins wouldn't be capable of authenticating users, which makes sense since allowing plugins to authenticate users could prove to be a security risk.

… client

…onsistent

…im-requirement OIDC group claim optional

Documentation/openid setup guides

dylanturn · 2021-04-15T20:37:54Z

For what it's worth, I just merged in the following changes to this branch:

Removed the OIDC group claims requirement to allow for simpler OpenID-Connect configurations.
Added documentation that walks through the process of configuring OpenID-Connect authentication endpoints in Okta, Auth0, and Keycloak.

oxy

Thanks a lot for your contribution! Only nit is the logger.debug/console.debug difference; sorry about misleading you earlier 😅

We've had this feature requested from us several times; I'll test this myself and merge this soon - I figure that since its a relatively isolated change (~150 lines of code), and a library made by a relatively trusted org (Auth0), it wouldn't cause a large maintenance burden.

src/node/http.ts

replace console.debug with logger.debug

updated the logger format to match what was actually requested

dylanturn · 2021-04-22T17:04:23Z

Thanks a lot for your contribution! Only nit is the logger.debug/console.debug difference; sorry about misleading you earlier 😅

We've had this feature requested from us several times; I'll test this myself and merge this soon - I figure that since its a relatively isolated change (~150 lines of code), and a library made by a relatively trusted org (Auth0), it wouldn't cause a large maintenance burden.

Is there anything you need from me before this can be merged?

greyscaled

Hey, 👏 thanks so much for a rad contribution. I took a look at just the documentation and have various very minor nitpicks so that it aligns with our style guide at Coder.

Check it out here if you'd like to: https://github.com/cdr/styleguide

Namely, we are trying to ensure the following styles:

code-server is always referred to in lowercase
headings are sentence case
UI elements use bold, but not italics

👏

docs/openid-auth.md

Co-authored-by: G r e y <[email protected]>

dylanturn · 2021-04-23T16:12:14Z

Hey, 👏 thanks so much for a rad contribution. I took a look at just the documentation and have various very minor nitpicks so that it aligns with our style guide at Coder.

Check it out here if you'd like to: https://github.com/cdr/styleguide

Namely, we are trying to ensure the following styles:

code-server is always referred to in lowercase

headings are sentence case

UI elements use bold, but not italics

👏

Thank you for making the changes easy to commit :-). I've applied your suggestions, let me know if there's anything else that needs to be updated.

Thank you :-)

greyscaled · 2021-04-23T16:16:55Z

Hey, clap thanks so much for a rad contribution. I took a look at just the documentation and have various very minor nitpicks so that it aligns with our style guide at Coder.
Check it out here if you'd like to: https://github.com/cdr/styleguide
Namely, we are trying to ensure the following styles:

code-server is always referred to in lowercase

headings are sentence case

UI elements use bold, but not italics

clap

Thank you for making the changes easy to commit :-). I've applied your suggestions, let me know if there's anything else that needs to be updated.

Thank you :-)

AFAICT, things are good to go - thanks for taking so much care with this contribution!!!

I'm deferring any remaining ✔️ to the core code-server crew

code-asher · 2021-04-28T16:03:52Z

Hey @dylanturn thank you for all the work you've done here!

Since my first comment we've been internally continuing the discussion on whether to accept PRs that introduce alternative authentication methods and we ultimately decided we would stick with the existing policy.

Part of the reason was because we want to limit core features to ones that directly impact the development experience of code-server itself (I feel that authentication is more an infrastructure problem) and the other part was that we didn't feel prepared to own and test the feature especially as we'd need to test manually.

I'm sorry we didn't reach this decision sooner before you put all this work into addressing the comments on your PR! 😭

But all is not lost. I think it makes sense to adapt this code to a plugin. I think we can get there most of the way with the existing API but we will probably need to make some changes to do it properly.

I'm not sure exactly what the API will need to look like so we're very much open to ideas. The current API is not considered stable so we can change it as much as we like before finally making it public. Once that's completed we can re-open and adapt the code.

Dylan Turnbull and others added 3 commits April 9, 2021 16:53

added openid auth support

d189ffc

merged in changes from main

5f59e99

scrubbed some small changes that I didn't mean to include

e1abd86

dylanturn requested a review from a team as a code owner April 10, 2021 18:16

jawnsy reviewed Apr 10, 2021

View reviewed changes

dylanturn added 2 commits April 10, 2021 14:02

Updated the yarn lockfile. Updated the OIDC user console log with debug.

906e7b6

ran yarn fmt, and made sure yarn lint, _audit, test:unit would pass.

12c813f

dylanturn added 11 commits April 15, 2021 10:46

made the oidc group claim optional

0440796

Added documentation that explains how to configure and OpenID-Connect…

4fcfab2

… client

added markdown to reduce the image sizes

a11d9a9

resized the images

1532cb5

updated the doc images

2a0c448

updated the doc images

6add98b

updated the doc images

3ac81d9

Updated the documentation with a couple extra steps after testing it out

aabfded

updated the screenshot to make sure the code-server endpoint url is c…

c3c305c

…onsistent

Merge pull request #4 from turnbros/feature/openid-connect-remove-cla…

153e89b

…im-requirement OIDC group claim optional

Merge pull request #5 from turnbros/documentation/openid-setup-guides

d5ffe24

Documentation/openid setup guides

oxy suggested changes Apr 16, 2021

View reviewed changes

src/node/http.ts Outdated Show resolved Hide resolved

src/node/http.ts Outdated Show resolved Hide resolved

src/node/http.ts Outdated Show resolved Hide resolved

src/node/http.ts Outdated Show resolved Hide resolved

dylanturn added 3 commits April 16, 2021 14:23

replace console.debug with logger.debug

9e2b4f3

updated the logger.debug and ran yarn fmt

af31604

Merge pull request #6 from turnbros/fix/openid-connect-logging

793817b

replace console.debug with logger.debug

jsjoeio requested a review from oxy April 16, 2021 20:27

updated the logger format to match what was actually requested

d503138

dylanturn added 3 commits April 16, 2021 16:51

Merge pull request #7 from turnbros/fix/openid-connect-logging

f2f731a

updated the logger format to match what was actually requested

Merge branch 'main' into feature/openid-connect

becd7da

bumped the jose version to appease the audit gods

ed29b32

greyscaled reviewed Apr 23, 2021

View reviewed changes

dylanturn and others added 2 commits April 23, 2021 10:59

Apply suggestions from code review

cf1ad0d

Co-authored-by: G r e y <[email protected]>

update the documentation to better conform to the style guide

ab718f1

code-asher closed this Apr 28, 2021

jsjoeio mentioned this pull request Aug 30, 2021

Customizable authentication #4068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/openid connect #3093

Feature/openid connect #3093

dylanturn commented Apr 10, 2021

jawnsy left a comment

jawnsy Apr 10, 2021

dylanturn Apr 10, 2021

jawnsy Apr 10, 2021

code-asher commented Apr 12, 2021

oxy commented Apr 12, 2021

dylanturn commented Apr 12, 2021

jsjoeio commented Apr 13, 2021

dylanturn commented Apr 14, 2021 •

edited

Loading

dylanturn commented Apr 15, 2021

oxy left a comment

dylanturn commented Apr 22, 2021

greyscaled left a comment

dylanturn commented Apr 23, 2021

greyscaled commented Apr 23, 2021 •

edited

Loading

code-asher commented Apr 28, 2021

Feature/openid connect #3093

Feature/openid connect #3093

Conversation

dylanturn commented Apr 10, 2021

jawnsy left a comment

Choose a reason for hiding this comment

jawnsy Apr 10, 2021

Choose a reason for hiding this comment

dylanturn Apr 10, 2021

Choose a reason for hiding this comment

jawnsy Apr 10, 2021

Choose a reason for hiding this comment

code-asher commented Apr 12, 2021

oxy commented Apr 12, 2021

dylanturn commented Apr 12, 2021

jsjoeio commented Apr 13, 2021

dylanturn commented Apr 14, 2021 • edited Loading

dylanturn commented Apr 15, 2021

oxy left a comment

Choose a reason for hiding this comment

dylanturn commented Apr 22, 2021

greyscaled left a comment

Choose a reason for hiding this comment

dylanturn commented Apr 23, 2021

greyscaled commented Apr 23, 2021 • edited Loading

code-asher commented Apr 28, 2021

dylanturn commented Apr 14, 2021 •

edited

Loading

greyscaled commented Apr 23, 2021 •

edited

Loading