Add pam_interactive authentication flow #752
Conversation
irods/auth/pam_interactive.py
Outdated
| ensure_ssl = request.pop(ENSURE_SSL_IS_ACTIVE, None) | ||
| if ensure_ssl is not None: | ||
| self.check_ssl = ensure_ssl | ||
|
|
||
| if self.check_ssl: | ||
| if not isinstance(self.conn.socket, ssl.SSLSocket): | ||
| msg = "pam_interactive auth scheme requires secure communications (TLS/SSL) with the server." | ||
| raise RuntimeError(msg) |
There was a problem hiding this comment.
The pam_interactive plugin has an "insecure mode" which allows for communications with the server without TLS. The server will enforce the use of TLS in the agent auth request step, so this check is not necessary unless this client will require TLS regardless. This would differ from the behavior of the C++ client plugin.
That being said, I might be missing something with regards to the ENSURE_SSL_IS_ACTIVE configuration. @d-w-moore may know more.
There was a problem hiding this comment.
That's right, the C++ client plugin does not perform this check. I included it to remain consistent with the pam_password implementation, which has a client-side check. If this is not the preferred approach, I can remove it.
There was a problem hiding this comment.
Right, TLS is indeed required for pam_password authentication (at least in iRODS 5+). The client-side check was implemented in PRC because the server-side check is "too late" (if you're curious about what I mean by that, see: irods/irods#8360 (comment)). pam_password also does not have an insecure mode feature (yet: irods/irods#8117).
The ENSURE_SSL_IS_ACTIVE configuration appears to be specific to pam_password, so honoring it here would expand its scope.
I think this is just a decision we will have to make for this client. The options are...
- Enforce TLS usage for pam_interactive authentication in PRC.
- TLS enforcement is deferred to the connected server.
Perhaps we can discuss this and write back here once we've reached consensus.
There was a problem hiding this comment.
After discussion, we've decided on 2. Let's remove these checks.
irods/auth/pam_interactive.py
Outdated
| if retr_path and retr_path.startswith('/'): | ||
| key = retr_path[1:] | ||
| pstate = req.get("pstate", {}) | ||
| if key in pstate: | ||
| req["resp"] = pstate[key] | ||
| return True |
There was a problem hiding this comment.
The variable name key is referring to what could be a whole "path" to an item in a sub-object (i.e. it's supposed to be treated as a JSON pointer). Will that work with the pstate dict?
For reference: https://github.com/irods/irods_auth_plugin_pam_interactive/blob/9092c0dfbd1151f8ff53214d6da30ea64b72eac2/plugin/src/main.cpp#L228
There was a problem hiding this comment.
Thanks for catching that. I overlooked the need to handle nested JSON pointers. Would the same apply to _get_default_value and _patch_state since they also process similar paths? If so, would using the jsonpointer and jsonpatch libraries be best?
There was a problem hiding this comment.
Would the same apply to
_get_default_valueand_patch_statesince they also process similar paths?
Yes, I think so.
Hmm... I am not excited about the prospect of bringing in more dependencies, but it may be better to do so than trying to implement this ourselves (at least for the sake of time). Try it out and see if things work as you expect. We can discuss this, as well.
There was a problem hiding this comment.
After some discussion, we've agreed that introducing more dependencies is fine - we just need to review the license for any dependencies to make sure they are okay to bring in. We also need to be cautious of the new dependencies bringing in a large number of secondary dependencies.
|
|
||
| import getpass | ||
| import sys | ||
| import logging |
There was a problem hiding this comment.
Could alphabetize. Not critical
There was a problem hiding this comment.
Thanks for undertaking this, @khansameer25 . This effort looks really good, and with some tests and extra explanatory pointers/comments (for people unfamiliar as yet to pam_interactive as a strategy) could be perfect.
| resp["pstate"] = resp.get("pstate", {}) | ||
| resp["pdirty"] = resp.get("pdirty", False) |
There was a problem hiding this comment.
For my sake being yet unfamiliar with but still a potential maintainer of the pam_interactive strategy and client plugin, some of the sections of code could be adorned with explanatory comments esp. where they involve server interactions and the associated key-strings for the exchange, as these seem to be external and not self-explanatory within this body of code. This could mean using web links to other explanations, to save on repetition.
| return True | ||
|
|
||
| # If no value found in pstate, set resp to empty string | ||
| req["resp"] = "" |
There was a problem hiding this comment.
What, if anything, does the server do when it finds the response is empty? Just trying to understand the overall process here. Again, this could be a possible subject to add to the already existing comment.
| if is_password: | ||
| user_input = getpass.getpass(display_prompt) | ||
| else: | ||
| sys.stdout.write(display_prompt) |
There was a problem hiding this comment.
Usually stderr is chosen for outputting prompts, rather than stdout
|
|
||
| # Pass through and failure states | ||
|
|
||
| def next(self, request): |
There was a problem hiding this comment.
Again, as a beginner who hasn't worked with the pam_interactive plugin before, I'd be interested in a comment maintaining that part played in the overall function of server/client interaction by such methods. Plus, I'm a little confused at trying to parse the existing comment, "pass through and failure states."
|
|
||
| return resp | ||
|
|
||
| def running(self, request): |
There was a problem hiding this comment.
I'm assuming we arrive at such states as "running" and "waiting[_pw]" as a direct result of the server altering NEXT_OPERATION field. Please correct if wrong, or further adorn with comments to help the user/maintainer.
| @@ -115,3 +115,35 @@ def write_pam_credentials_to_secrets_file(password, overwrite=True, ttl="", **kw | |||
| raise RuntimeError(f"Password token was not passed from server.") | |||
| auth_file = s.pool.account.derived_auth_file | |||
| _write_encoded_auth_value(auth_file, to_encode[0], overwrite) | |||
|
|
|||
| def write_pam_interactive_irodsA_file(overwrite=True, ttl="", **kw): | |||
There was a problem hiding this comment.
Nice. Let's be sure to test this routine works as expected, too. Feel free to ping me about the writing of the tests as well.
Client-side implementation for the pam_interactive authentication flow. This is a direct port of the C++ Pam Interactive Auth Plugin
Unlike native or pam_password, pam_interactive supports a multi-step flow where the server controls the prompts (e.g., password, OTP, PIN)
The write_pam_interactive_irodsA_file function performs a one-time interactive login using pam_interactive and generates a .irodsA file for authentication in future sessions through the native scheme. It follows the pattern of existing free functions for the other auth flows, reusing their logic but omitting a password parameter, as prompts and inputs are handled interactively by the server.
Performed basic manual testing on an iRODS 5.0.1 server (without SSL/TLS), including successful login with correct credentials and failed login with incorrect credentials using simple single-password setups. More complex multi-step flows remain untested.