irods / python-irodsclient

A Python API for iRODS
Other
63 stars 73 forks source link

Document how to store passwords in `.irodsA` file #596

Closed alanking closed 2 weeks ago

alanking commented 2 months ago

It seems this is possible: https://github.com/irods/python-irodsclient/blob/067e63d1075d97d240f45a427495673aac5aaa07/irods/connection.py#L532-L536

However, this is not documented anywhere, and it's not clear (at least to me) whether this option is available outside of PAM authentication (this code is under _pam_login) or how to use it. Let's add documentation around how this works and how to use it.

d-w-moore commented 2 months ago

I think it's already documented here in the list item that describes the setting legacy_auth.pam.store_password_to_environment. Do we need more detail in that section, or perhaps (more likely) elsewhere?

alanking commented 2 months ago

Oh, I see. I was not looking for that.

This step is only performed while auto-renewing PAM authenticated sessions.

If this is true, that means that iRODS native authentication cannot use this feature. Is that true?

I feel like having a dedicated how-to section for this would be helpful. Or perhaps a mention of this option in the section about creating a connection (https://github.com/irods/python-irodsclient?tab=readme-ov-file#establishing-a-secure-connection) or one of the sections following that. Does that seem like a good idea? Maybe what we have is enough...

korydraughn commented 2 months ago

A new section makes sense given it wasn't obvious where to look.

That could be either a FAQ, Troubleshooting, or How-To section.

d-w-moore commented 2 months ago

This step is only performed while auto-renewing PAM authenticated sessions.

If this is true, that means that iRODS native authentication cannot use this feature. Is that true?

Native authentication doesn't execute this particular section of code , since it's located within _login_pam(), but know that PRC does - like iinit - store the encoded native encoded password data in the environment's auth file, .irodsA, in particular when a user changes their native password. I remember implementing that one, too, some time ago. But when it's the iRODS native password that's changing, no configuration setting is needed to approve the overwriting of the .irodsA. Which seems appropriate, because native auth is the most direct and straightforward form of authentication in iRODS.

What would be more accurate to say regarding PAM is that PAM authentication in PRC currently needs explicit approval to overwrite .irodsA with its encoded password token, and that approval is granted by setting legacy_auth.pam.store_password_to_environment to True. I think my mindset was that I wanted the designer of an application that leverages PRC to be explicit about (thus provably conscious of) the decision of making PAM the active authentication method, rather than just let PRC possibly accidentally overwrite a current copy of .irodsA that might contain a user's native login data. So, ... accountability, in other words. I may have been misguided or inconsistent with other clients in taking that approach, however. After all iinit just goes ahead and overwrites it with whatever the current auth scheme dictates, right?

This all arose from trying to give the PRC some iinit-like behavior with respect to reading & writing PAM password data in .irodsA, which it never could do until a very recent release (v2.0.0). And implementing things the way I did seems to have made sense as opposed to PRC shelling out to iinit when for example a PAM password was found to have expired.

The consequence I guess is that iinit-type behavior is more arbitrarily distributed throughout the PRC in its present form than it should be, ideally.

Background: Yes, it's true PRC has long had the capability of authenticating through PAM, but until that recent release it wasn't using stored .irodsA data or persisting such data back into .irodsA for the next PAM session when the current password was found to have expired. Rather, PRC was actually authenticating each new session by generating the login tokens anew, via calling the PAM api instead of respecting what iinit might have stored in .irodsA. Examine the _login_pam method in v1.1.9, and you'll see that _login_native is not called preemptively to query the environment-stored password before deciding whether to have the entire PAM-api exchange with the server.

Whereas the iCommands' methodology was always to read out the PAM token that existed already in .irodsA . So this just brings PRC up to where the iCommands had been.

d-w-moore commented 1 month ago

There's now a commit in pr #620 documenting how users may employ PRC in place of iinit to store pam passwords.

It isn't clear to me which route to go, in furnishing the iinit-like capability to create an .irodsA in PRC for native logins as well. Is that something we want / need ?

If so, how do we prefer to proceed?

korydraughn commented 1 month ago

Providing a free function feels the most correct and enables flexibility.

I don't think the PRC should automatically write any files due to its design. That feels like a decision the user/dev should make. It's also not clear when the PRC would write that file.

korydraughn commented 1 month ago

Adding this as a discussion item.