Kerberos Auth Implementation Resolves #698 #1027
Conversation
Auth implementation, based upon LDAP I do not currently have access to a KDC so this is adapted from 3 year old code. I also have never tested gssapi with Active Directory / Azure AD I highly encourage testing and comments for both MIT KDC and Active Directory. Also I'm not expert at poetry so please review the pyproject.toml additions carefully. A Note about Kerberos I have a lot of experience working with kerberos and deploying applications into the enterprise. The login method defined here was arrived at after 5 million complaints and help requests from various teams of administrators that were NOT familiar with kerberos but just wanted to authenticate. The logic within largely protects the user from themselves and the major mis-configuration pitfalls. Kerberos will not warn you if your keytab doesn't exist, or your principal or service name is invalid or illogical. Kerberos also will attempt a whole suite of authentication methods. If you give kerberos a keytab that doesn't work, you'll get an error saying something like "your password is invalid" - why ? Because kerberos tries every possible auth mechanism and misleads the user about what the real problem is. So with that in mind, please don't assume any lines of codes are wasted in the login method - GOOD LUCK!
There was a problem hiding this comment.
@bendemott thank you so much for submitting this! I've only done a very cursory look so far, but it looks really good. I love the attention to edge cases, the thorough code comments, and general thoughtfulness put into it.
I think this one will take some time to get into the project but I'm very much looking forward to it. I expect it will be in a 2.x release at some point, even just based on available maintainer time and when I expect we'll do a 2.0.0 release. I hope that's ok and look forward to working with you on it.
I commented on the LDAP subclassing inline, but for me the major things I want to look at are adding tests. When I have some time, I'll look into some options for adding a KDC to integration tests; in the meantime hopefully some unit tests are less onerous.
btw we also have a Discord server for contributors if you'd like to join (not at all a requirement) linked at the bottom of this discussion.
Thanks again!
| pass | ||
|
|
||
|
|
||
| class Kerberos(LdapAuth): # TODO for discussion do you want ldap subclassed or to be verbose and a bit redundant ? |
There was a problem hiding this comment.
# TODO for discussion do you want ldap subclassed or to be verbose and a bit redundant ?
I think we want to be verbose/redundant.
We don't know if/how the LDAP API will change, and would like these to be independent.
If we were going to look to DRY up things like this, I think we'd make some sort of abstract class one level higher, and have it be the parent for both of these (and anything else it applies to) but for right now I think we ought to duplicate.
| self, name, policies=None, mount_point=DEFAULT_MOUNT_POINT | ||
| ): | ||
| """ | ||
| Create or update LDAP group policies. |
There was a problem hiding this comment.
Just putting one comment here but there are several references to LDAP that will need to be updated.
| :return: The response of the create_or_update_group request. | ||
| :rtype: requests.Response | ||
| """ | ||
| return super().create_or_update_group(name, policies, mount_point) |
There was a problem hiding this comment.
Though we will probably want to avoid subclassing anyway, please consistently use named arguments instead of positional for calling. There is a chance we will look to make many arguments named-only in the future.
| log.debug(f'translate "service" from "{service_name}" to "{service}" - host-based-service is assumed') | ||
| service_names.append(gssapi.Name(service, gssapi.NameType.hostbased_service)) | ||
| elif '/' in service: | ||
| # if "/" appears by itself convert it to a "@" - which is required by gssapi |
There was a problem hiding this comment.
I'm putting this comment in a sort of random place even though it applies elsewhere too, but I wanted to say I love the comments in the code for this type of thing, which I think will go a long way in keeping this code maintainable in the long term.
The only thing I might ask is whenever possible, include a link to where requirements like this are defined or can otherwise be looked up in more detail.
| @@ -38,6 +38,9 @@ python = "^3.6.2" | |||
| pyhcl = "^0.4.4" | |||
| requests = "^2.27.1" | |||
|
|
|||
| # Note on linux krb5 libs are required: `sudo apt install libkrb5-dev` or `yum install krb5-devel` | |||
| requests_gssapi = { version = "^1.2.0", optional = true } # kerberos support, will include `gssapi` | |||
There was a problem hiding this comment.
should this be both here and in the extras? if it's here won't it always install and not just with hvac[kerberos]?
briantist
added
enhancement
| @@ -38,6 +38,9 @@ python = "^3.6.2" | |||
| pyhcl = "^0.4.4" | |||
| requests = "^2.27.1" | |||
|
|
|||
| # Note on linux krb5 libs are required: `sudo apt install libkrb5-dev` or `yum install krb5-devel` | |||
| requests_gssapi = { version = "^1.2.0", optional = true } # kerberos support, will include `gssapi` | |||
There was a problem hiding this comment.
requests_gssapi requires users to install kfw which is a pain
kerberos and winkerberos do not have that requirement and would be preferable
There was a problem hiding this comment.
I think you mean kfw is a requirement of gssapi on windows.
I'll have to take a look, not sure if the other api's can be used / they are somewhat incomplete.
There was a problem hiding this comment.
Correct. it's a dependency of gssapi (here).
I realized after posting the comment.
There is a conda requests-gssapi package which seems to provide the same functionality but uses native O/S krb libs
I have to say the python ecosystem is a real mess around such a crucial piece of functionality
Auth implementation, based upon LDAP
I do not currently have access to a KDC so this is adapted from 3 year old code.
I also have never tested gssapi with Active Directory / Azure AD I highly encourage testing and comments for both MIT KDC and Active Directory.
Also I'm not expert at poetry so please review the pyproject.toml additions carefully.
A Note about Kerberos
I have a lot of experience working with kerberos and deploying applications into the enterprise. The login method defined here was arrived at after 5 million complaints and help requests from various teams of administrators that were NOT familiar with kerberos but just wanted to authenticate. The logic within largely protects the user from themselves and the major mis-configuration pitfalls.
Kerberos will not warn you if your keytab doesn't exist, or your principal or service name is invalid or illogical. Kerberos also will attempt a whole suite of authentication methods.
If you give kerberos a keytab that doesn't work, you'll get an error saying something like "your password is invalid" - why ?
Because kerberos tries every possible auth mechanism and misleads the user about what the real problem is.
So with that in mind, please don't assume any lines of codes are wasted in the login method - GOOD LUCK!