Authenticate native and pam_password with iRODS 4.3+ auth framework #685

d-w-moore · 2025-01-28T20:18:22Z

No description provided.

MartinFlores751

Looks good. Have a good number of questions since I'm not that familiar with more complex Python, and a few on code style.

MartinFlores751 · 2025-01-28T21:06:18Z

irods/__init__.py

-    default_irods_authentication_file = os.path.join(
-        os.path.dirname(env_filename), ".irodsA"
+    default_irods_authentication_file = (
+        #os.path.join(os.path.dirname(env_filename), ".irodsA")# <-- Issue #XXX)


Don't forget to remove commented code.

Will remove !

MartinFlores751 · 2025-01-28T21:13:32Z

irods/auth/__init__.py

+            # So that the connection object doesn't hold on to password data too long:
+            conn.auth_storage = weakref.ref(store)
+        return store


Does store live long enough in the call site for its proper usage before deletion?

Not sure the desired goal/requirements, but is there a way to make sure that store gets cleaned up at a specific point?

If I understand the weakref docs, do we check that conn.auth_storage() is None?

Does store live long enough in the call site for its proper usage before deletion?

Yeah, it's kind of left up to the caller, but assuming the caller immediately assigns an appropriately scoped variable to store a hard reference to the return value, Python won't let the weakref expire. I've tested it manually.

MartinFlores751 · 2025-01-28T22:27:03Z

irods/auth/native.py

+    ## short-cut back to the 4.2 logic:
+    #conn._login_native()


Don't forget to remove dead code.

Don't forget to remove dead code.

will do!

MartinFlores751 · 2025-01-28T22:29:56Z

irods/auth/native.py

+    # Default request value (None) assumes the conn.account has been initialized with the normal authentication
+    # params such as user, zone, host, port, password, etc., because in normal PRC operation this has all been
+    # seen to by the iRODSSession constructor.
+    if req is None:
+        req = {'user_name': conn.account.proxy_user,
+               'zone_name': conn.account.proxy_zone}


Is it possible that conn.account could not be initialized? What would happen then?

Based on your comment, it sounds like any errors with conn would show up earlier in iRODSSession, but just asking to be sure.

Is it possible that conn.account could not be initialized? What would happen then?

It's as unlikely as it gets. In normal PRC use, properly constructed account objects are given to connections in their constructors, and must be of the right type (iRODSAccount) or things would quickly fall apart right there in the Connection class's inner workings.
in "normal" PRC use, iRODSAccount objects are never constructed except for the purpose of building an iRODSSession, and connections are never instantiated in a vacuum. In fact, when sessions are cloned, the new copy is absent any subordinate connections.
If any of these assumptions were subverted , things very plainly wouldn't go well. But that said, no-one to my knowledge has filed an issue requiring those assumptions should be changed. It isn't the most iron-clad situation, admittedly, as it requires developers to be all the more on their guard. I had to create a connect parameter in the Connection constructor, in fact, just to be able to write the " demo script " versions of these modules. Since those demos are now gone, I of course plan to remove the connect parameter as well.

MartinFlores751 · 2025-01-28T22:46:10Z

irods/auth/native.py

+        depot = AuthStorage.get_temp_pw_storage(self.conn)
+        if depot:
+            # The following is how pam_password communicates a server-generated password.
+            password = depot.retrieve_pw()


If I understand the code correctly, there are no strong references to the auth_storage member in conn. This means that depot not being None relies on when gc is done. gc may run between storage creation and retrieval.

Is this desired? Or is there another way to have the auth_storage be short lived?

depot being none means that no AuthStorage object was set, which currently should only be the case when authenticate_native is called from the Connection logic without going first through authenticate_pam_password. Otherwise, the duration of the _ variable (the AuthStorage instance) will still be alive at this point.

Or at least that's how I planned it.
I realize some of these things need to be commented for clarity, just haven't made it that far yet ;)

MartinFlores751 · 2025-01-29T18:46:44Z

irods/auth/pam_password.py

+FORCE_PASSWORD_PROMPT = "force_password_prompt"
+AUTH_PASSWORD_KEY = "a_pw"


Is it better to have these variables outside pam_password_ClientAuthState, or to move them inside?

Not familiar with Python scoping practices.

We could move them inside if they're meant to be scoped for appropriate usage only from that location. I don't really know yet what is appropriate. In C++ irods core code , AUTH_PASSWORD_KEY is located in namespace irods and the FORCE_PASSWORD_PROMPT in irods::experimental::auth. I compromised by putting both in the irods.auth.pam_password module namespace from sheer laziness and desire to get this impl' done quickly, but since you mention it, I don't know for sure these would not be used by pam_interactive (not yet implemented), so I am actually wondering if they should be moved to namespace irods.auth and then be imported here.

MartinFlores751 · 2025-01-29T18:47:56Z

irods/auth/pam_password.py

+        # TODO: Remove. This is only for debug & testing; check_ssl=False lets us send
+        # password-related information in the clear (i.e. when SSL/TLS isn't active).
+        self.check_ssl =  check_ssl


Don't forget TODO. Remove if done.

MartinFlores751 · 2025-01-29T18:50:54Z

irods/auth/pam_password.py

+    # Client define
+    AUTH_CLIENT_AUTH_REQUEST = "pam_password_auth_client_request"
+    # Server define
+    AUTH_AGENT_AUTH_REQUEST = "auth_agent_auth_request"


Should these be put at the top of the class, before __init__? It might make it more clear that these are class variables instead of part of a method.

MartinFlores751 · 2025-01-29T18:52:15Z

irods/auth/pam_password.py


-#   # in the future we might need cross-plugin calls:
-#   native_login(conn)  # see below for import
+    perform_native_auth = pam_password_auth_client_perform_native_auth


Similar question, should this be at the top of the class before __init__?

In this case it needs the previous reference to be defined. (I.e. this time it's a symbol, not quoted.) A simpler example:

class This_Will_Work: def x(self): return 1 alias = x

But if the e assignment is placed before x, Python's scanning won't find the x definition until too late.

Good point! That's a mistake on my part. You can resolve this.

MartinFlores751 · 2025-01-29T18:55:16Z

irods/connection.py

@@ -754,3 +756,5 @@ def temp_password(self):
        # Convert and return answer
        msg = response.get_main_message(GetTempPasswordOut)
        return obf.create_temp_password(msg.stringToHashWith, self.account.password)
+
+import irods.auth


Is there a reason to move this import to the bottom of the script? I'm assuming we want something there to be executed/evaluated later, but asking to be sure.

I believe there's a cyclic import dependency somewhere above it in the module. I didn't think very hard about it or try to track it down, I admit. Placing this irods.auth import with the others in the file header doesn't work, but placing it at the end does.

Hmm... you've got me wondering. It seems now I'm no longer able to reproduce the cyclic import dependency.

I'll place it back with the others at the head of the file. It should fail in testing if it truly creates a cyclic dependency.

…new auth-fwk)

d-w-moore · 2025-01-31T08:21:26Z

irods/connection.py

+            ses = self.pool.session_ref()
+            if ses:
+                ses.resolve_auth_options(scheme, conn = self)
+


Note to self.
Remember to pass these options to the auth framework as extras.

FifthPotato

Looks pretty good to me! Just a few minor things.

FifthPotato · 2025-01-30T17:55:45Z

irods/__init__.py

-        os.path.dirname(env_filename), ".irodsA"
+    default_irods_authentication_file = (
+        #os.path.join(os.path.dirname(env_filename), ".irodsA")# <-- Issue #XXX)


With this change, I don't actually see a reference to env_filename in this function (other than the guard clause at the start). Is that intentional?

FifthPotato · 2025-01-30T18:11:27Z

irods/auth/__init__.py

+
+    def __init__(self, connection, scheme):
+        self.conn = connection
+        self.loggedIn = 0


Since this is set directly to 1 below and used only in a boolean context, is there value in making this integral instead of boolean?

d-w-moore added 2 commits January 28, 2025 15:17

authenticate with iRODS 4.3+ auth framework

35ac255

remove redundant scripts

0b430d8

d-w-moore marked this pull request as draft January 28, 2025 20:18

korydraughn requested review from MartinFlores751 and FifthPotato January 28, 2025 20:24

d-w-moore self-assigned this Jan 29, 2025

MartinFlores751 reviewed Jan 29, 2025

View reviewed changes

d-w-moore added 4 commits January 31, 2025 03:18

new write_pam_irodsA_file function

36e1e08

experimental - auth options/write_pam_irodsA related

6e7e1cc

auth_options per connection (in support of writing PAM .irodsA using …

41b4227

…new auth-fwk)

review-related and experimental changes

3b0345c

d-w-moore commented Jan 31, 2025

View reviewed changes

FifthPotato reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authenticate native and pam_password with iRODS 4.3+ auth framework #685

Authenticate native and pam_password with iRODS 4.3+ auth framework #685

d-w-moore commented Jan 28, 2025

MartinFlores751 left a comment

MartinFlores751 Jan 28, 2025

d-w-moore Jan 30, 2025

MartinFlores751 Jan 28, 2025

MartinFlores751 Jan 28, 2025

d-w-moore Jan 30, 2025

MartinFlores751 Jan 28, 2025

d-w-moore Jan 30, 2025

MartinFlores751 Jan 28, 2025

MartinFlores751 Jan 28, 2025

d-w-moore Jan 30, 2025

MartinFlores751 Jan 28, 2025

d-w-moore Jan 30, 2025 •

edited

Loading

MartinFlores751 Jan 29, 2025

d-w-moore Jan 30, 2025 •

edited

Loading

MartinFlores751 Jan 29, 2025

d-w-moore Jan 30, 2025

MartinFlores751 Jan 29, 2025

MartinFlores751 Jan 29, 2025

d-w-moore Jan 30, 2025 •

edited

Loading

MartinFlores751 Jan 30, 2025

MartinFlores751 Jan 29, 2025

d-w-moore Jan 30, 2025

d-w-moore Jan 30, 2025 •

edited

Loading

d-w-moore Jan 30, 2025

d-w-moore Jan 31, 2025

FifthPotato left a comment

FifthPotato Jan 30, 2025

FifthPotato Jan 30, 2025

		FORCE_PASSWORD_PROMPT = "force_password_prompt"
		AUTH_PASSWORD_KEY = "a_pw"

Authenticate native and pam_password with iRODS 4.3+ auth framework #685

Are you sure you want to change the base?

Authenticate native and pam_password with iRODS 4.3+ auth framework #685

Conversation

d-w-moore commented Jan 28, 2025

MartinFlores751 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-w-moore Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-w-moore Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-w-moore Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-w-moore Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FifthPotato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-w-moore Jan 30, 2025 •

edited

Loading

d-w-moore Jan 30, 2025 •

edited

Loading

d-w-moore Jan 30, 2025 •

edited

Loading

d-w-moore Jan 30, 2025 •

edited

Loading