-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Added keycloak authentication #3486
Conversation
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.
This implements keycloak auth purely on the server so this is not safe at all over unencrypted connections.
All the client does is to send the password unencrypted. That's not how keycloak is meant to be used.
I could merge this module as keycloak_password
or something - similar to the kerberos_password
we already have.
But not the client changes.
A proper keycloak
module would do the request from the client and send the token to the server only.
I think there is a misunderstanding, this does not implement the If you want me to request the token in the client, and then validate it in the server, that's possible, but I'm not sure what this changes security-wise. We would actually send a lot of info about the user, instead of just the authorization code... |
There definitely is:
It means that with your authentication module, the password is not sent over the network - often unencrypted. |
What is sent through that is the authorization code, this isn't a password. I will improve this file to use another variable name instead. |
Refactoring for PR
I'm not sure what the handler file was for. I seem to have needed it at some point, but in the end, it isn't used at all so I removed it. I hope these changes address your concerns. |
Please don't be put off by the number of comments below, it's all fairly minor at this point. A few more questions about testing:
|
self.redirect_uri = kwargs.pop("redirect_uri", KEYCLOAK_REDIRECT_URI) | ||
self.scope = kwargs.pop("scope", KEYCLOAK_SCOPE) | ||
self.grant_type = kwargs.pop("grant_type", KEYCLOAK_GRANT_TYPE) | ||
kwargs["prompt"] = kwargs.pop("prompt", "keycloak") |
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 wouldn't hurt to add a comment here, like "use keycloak
as default prompt".
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.
minor: keycloak token
as default prompt would be clearer IMO.
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.
When we add more grant_types we could have
- keycloak password
- keycloak authorization code
- others
But never token. We could also implement them in different files if too different / complex.
We can add what you prefer here but in this case it will never be shown, or I don't see how. And it will never ask for a token.
Bump! |
Hey, the work is almost done but I'm currently on holidays. Will finish it up next week! |
About testing this I'm not sure. I'm not running my own keycloak instance. Maybe we will do so in the future. |
You've "resolved" the review without actually addressing any of the numerous comments I made. |
I'm going to push them soon, it was just easier to understand what I addressed already that way. I have a question. I would like to implement the logout functionality. Do you have any pointers or examples on how to do that? |
Good point. This wasn't needed until now. |
It's not urgent, just nice to have! Thanks! |
I added the changes we discussed about. In order to test you can call it that way:
The |
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.
python3 keycloak_auth.py '{"code":"authorization_code"}'
The authorization_code must be requested from the keycloak auth endpoint. I hope it's suitable.
It is fine.
A couple of questions:
- if a valid response is just a JSON string, wouldn't a keycloak auth client be trivial to implement?
- do you have sample responses we could add to a unit test? (which is missing BTW)
There are just a few nitpicks more that I just found in the PR.
If you don't want to take care of them, just let me know and I'll tweak things after merging this PR.
self.redirect_uri = kwargs.pop("redirect_uri", KEYCLOAK_REDIRECT_URI) | ||
self.scope = kwargs.pop("scope", KEYCLOAK_SCOPE) | ||
self.grant_type = kwargs.pop("grant_type", KEYCLOAK_GRANT_TYPE) | ||
kwargs["prompt"] = kwargs.pop("prompt", "keycloak") |
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.
minor: keycloak token
as default prompt would be clearer IMO.
How the client works, if you look at the diagram I sent above, is indeed very simple. We redirect to the This is exactly what I implemented in the client PR. Could you please review it while I take care of the last details here? Regarding the unit tests, there is only one value that we can give, which is just a random string like above which is going to fail the auth, unless you have access to a keycloak server and can get a valid authorization code during testing. |
Is the JSON string always just one As for the unit tests, even testing that authentication fails will at least exercise some of the code and increase code coverage.
Ideally we would mock a keycloak server to validate fully - but that's too much effort. |
In this case, this isn't the token that is sent unencrypted (it still uses SSL), it's the authorization code.
The client isn't prompting anything to the user. The client is redirecting to an URL to get the authorization code. I'm not sure in which conditions we would be able to see this prompt? Even if that happened, what it should prompt the user would be the authorization code, not the token. Keycloak has several
The way that this authentication module works is not the same as gss/kerberos. I hope I could clarify. |
Actually it can also have an error inside. Let me send you an example in a minute. Edit: here is an example of error we can find in the response_json. It will either have a code or an error in this form: |
Refactoring for PR
* whitespace was wrong (4 spaces) * empty lines * error message format * consistency: check for failures and shortcut out, success at the end * unused import * 'raise' format
Stumbled upon this good overview of oauth2: The complete guide to protecting your APIs with OAuth2 - part 1 |
the kwargs were being parsed but not actually used
@iDmple please take a look at the obvious fixes from the unit test I have added - since I don't have a keycloak server to test against. |
@iDmple can you please try b0834ef or later? |
@totaam We will test this ASAP and report back. |
Fixes #3334
I added keycloak authentication as discussed. This is needed for it:
These specific versions are due to this issue.
Regarding how the XPRA auth is working, I'm unsure how to display the error messages in the client when the authentication fails in the function
check
, but it displays fine when it fails in__init__
.Please check this PR together with the associated PR in xpra-html5. Let me know if anything needs to be improved.