Add backend for Hashicorp Vault (alternative approach) #669
Conversation
candlerb
force-pushed
the
candlerb/oidc
branch
from
ed53109
to
12f3c24
Compare
Codecov Report
@@ Coverage Diff @@
## master #669 +/- ##
==========================================
+ Coverage 76.95% 77.15% +0.19%
==========================================
Files 317 321 +4
Lines 9647 9739 +92
Branches 1034 1042 +8
==========================================
+ Hits 7424 7514 +90
- Misses 2071 2072 +1
- Partials 152 153 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for trying to contribute @candlerb
I'm personally against this big list of changes that affect everything a lot of providers. I think we should go one step smaller and deal with individual problems as they arise. What is the issue for Vault here? What are you trying to solve?
The project is in maintenance mode and I feel like changes like this one are dangerous if there isn't a clear benefit to them. I won't merge for now and I would ask other maintainers not to merge as without discussing this first.
|
The specific problem is that you can't configure PSA to connect to an arbitrary standards-compliant OpenID Connect provider: you have to write Python code to subclass There are a few things such a generic backend needs to do:
The most flexible place to do (2) was in However I take your point about not touching working code. I could create a new subclass of |
|
Now I remember the other problem. In You can spoof it by turning it into a property (which is what I did in #668 to minimise other changes): This works, but it seems pretty ugly to me. It would probably be better just to have a fresh The nearest existing backend I found with configurable endpoint URL is the Keycloak backend, which gets around this problem because it doesn't subclass from |
|
I have personally had the same issue. I was able to work around that by using a function that generates those classes and writes them to If you are just writing an OpenID Connect compatible software that only needs OIDC, I would probably recommend to not use this library, because it's not very well maintained. There are probably other libraries that only do OpenID Connect and are therefore more correct when it comes to the standard. In general I'm just worried about touching the OIDC pieces here, because they are already not that well maintained and almost nobody around here understands OIDC on a good level. I certainly do not, because it's just hard to understand and I'm kind of maintaining that part :-/. |
It's the other way round: I wanted to take an application which already has python-social-auth integration (Netbox), and connect it to an OIDC provider. Netbox does offer an alternative way of doing this, via remote authentication with HTTP headers, so I can instead use an Apache reverse proxy with mod_auth_openidc. I just thought that in fixing this use case for Netbox, it would be nice to make it available to other consumers of python-social-auth. |
|
I don't see anything breaking in this (though my OpenID Connect experience is very limited). I think having generic OpenID Connect provider is good for reasons described by @candlerb - we use python-social-auth at Weblate and users do not have the choice for the authentication library (I agree that there might be better choices out there these days), but they want to have the choice in the authentication service. Therefore, I'd go and merge this. It would be great, if @candlerb could prepare matching docs pull request in https://github.com/python-social-auth/social-docs/ |
|
Since I feel very much responsible for OpenID Connect stuff, please do not merge (otherwise I might have to revert). I will try to review some things again next week. My feeling still is that this is a bad approach (and probably breaks backwards compatibility), but I do not have the time to explain right now. |
|
I've just gone again through this patch and I don't see anything breaking compatibility there. I think approach in having generic OpenID Connect backend is desired. For example IdentityServer could be integrated this way also. |
|
I don't see how this is generic. If I wanted a generic OpenID Connect backend, I would want multiple ones. This is pretty common actually. The current approach is already generic, you just have to instantiate a class. I still have to review the code better, though. |
|
Yes, it's limited to a single OpenID Connect provider, but this is really what I'm looking for. Our users typically have internal authentication service and want to use that. Being able to specify that only in configuration without implementing a subclass for every service would be great. |
I feel like the difference of inheriting from a class and setting a few attributes on it versus setting a few variables in a global namespace is minimal. Both of these ways can be documented, will be around the same line count and with inheritance it is even possible to use multiple OIDC backends if that is ever wanted. I kind of understand your approach with managing variables better now. Is there a way you could provide me with a way where in the OIDC standard you were looking up the basic auth stuff? I feel like the big problem is that we currently do not support that and the OIDC standard is really hard to get right... |
Sure. When you want to exchange a code for an ID token (at the Token endpoint), OIDC provides two ways of the client authenticating itself: via HTTP Basic Authentication, or via client_id and client_secret fields in the POST request body. This is described in section 3.1.3.2 and section 9 of the OIDC Core 1.0 specification. This in turn refers to section 2.3.1 of the OAuth 2.0 specification, RFC 6749. Here it is made explicit that the authorization server must implement HTTP Basic Auth, and may also implement the POST body fields (but the latter is not recommended). By supporting only basic auth but not POST, Vault is compliant to the standard. However, prior to this patch, PSA's oauth.py (and hence also open_id_connect.py) implements only the POST option. There are some subclasses of BaseOAuth2 which override some methods to do basic auth - see for example RedditOAuth2. |
There was a problem hiding this comment.
Sorry for the delay again. I'm pretty busy and this is not easy to understand.
I feel like I understand the OpenID Connect part now, thanks for linking the relevant documents.
I feel like since @nijel wants the settings changes, this is also fine (although I personally prefer the subclassing stuff). Is there documentation needed @nijel ?
|
Yes, documentation for new backends is definitely needed in https://github.com/python-social-auth/social-docs/ |
|
I made some docs a while ago here: python-social-auth/social-docs@master...candlerb:candlerb/oidc I didn't make a PR as it was unclear whether these oidc changes were accepted, either in current form or differently. |
* Modify OpenIdConnectAuth so it can be directly instantiated (i.e. as SOCIAL_AUTH_OIDC_xxx) for use with a generic OpenID provider * Update BaseOAuth2 so that it can perform HTTP Basic Auth for completing the auth process, configured from OIDC discovery * Fix tests for okta, which previously were not calling oidc_config() from OktaMixin
|
To me this looks good now, thanks a lot. I think @nijel should approve all the documentation stuff, I have no idea about that. |
|
I've made the PR for the docs here: python-social-auth/social-docs#109 |
|
Merged, thanks for your contribution! |
It leads to crash with Django. This reverts a tiny portion of python-social-auth#669. Fixes python-social-auth#690
…ectAuth.oidc_endpoint()` exists, refs python-social-auth/social-core#669.
Proposed changes
This is a more generic solution to PR #668 - for consideration.
In this PR, the OpenIdConnectAuth class is enhanced so that it can be instantiated directly, as a "generic" OIDC connector.
To enable this, it also makes the OAuth2 authentication mechanism configurable (i.e. form post or HTTP Basic), and uses OIDC discovery to select this automatically.
Vault then becomes an empty subclass of OpenIdConnectAuth, since it's an out-of-the-box OIDC provider.
Types of changes
Please check the type of change your PR introduces:
I believe this shouldn't break existing code, and all tests pass, but it would be wise to check the other OpenIdConnectAuth subclasses just in case.
Checklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.
Other information
This PR also includes a fix for okta test which surfaced as part of this change. I believe this should only fix the test, not the actual behavior of the okta backend.