-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Feature/openid connect #3093
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
Feature/openid connect #3093
Conversation
@jawnsy
jawnsy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
src/node/http.ts
Outdated
@jawnsy
jawnsy
Apr 10, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, very sorry, I'll include this change along with the updates to yarn lockfile.
@jawnsy
jawnsy
Apr 10, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No apologies needed! Thanks so much for your contribution! ❤️
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.
oxy
commented
Apr 12, 2021
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 😅
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.
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!
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.
...im-requirement OIDC group claim optional
Documentation/openid setup guides
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
oxy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
replace console.debug with logger.debug
updated the logger format to match what was actually requested
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
👏
Co-authored-by: G r e y <vapurrmaid@pm.me>
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 :-)
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
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.
Adds OIDC support without needing sidecars/proxies.
Resolves: #2636