Skip to content
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

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions irods/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ def env_filename_from_keyword_args(kwargs):
def derived_auth_filename(env_filename):
if not env_filename:
return ""
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to remove commented code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove !

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

os.path.expanduser("~/.irods/.irodsA")
)
return os.environ.get(
"IRODS_AUTHENTICATION_FILE", default_irods_authentication_file
Expand Down
1 change: 1 addition & 0 deletions irods/api_number.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,5 @@
"REPLICA_CLOSE_APN": 20004,
"TOUCH_APN": 20007,
"AUTH_PLUG_REQ_AN": 1201,
"AUTHENTICATION_APN": 110000
}
119 changes: 115 additions & 4 deletions irods/auth/__init__.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,64 @@
import logging
import weakref
from irods.api_number import api_number
from irods.message import iRODSMessage, JSON_Message
import irods.password_obfuscation as obf
import irods.session


__all__ = ["pam_password", "native"]


AUTH_PLUGIN_PACKAGE = "irods.auth"


import importlib


class AuthStorage:

@staticmethod
def get_env_password():
return irods.session.iRODSSession.get_irods_password()

@staticmethod
def set_env_password(unencoded_pw):
from ..client_init import _open_file_for_protected_contents
with _open_file_for_protected_contents(irods.session.iRODSSession.get_irods_password_file(),'w') as irodsA:
irodsA.write(obf.encode(unencoded_pw))

@staticmethod
def get_temp_pw_storage(conn):
return getattr(conn,'auth_storage',lambda:None)()

@staticmethod
def create_temp_pw_storage(conn):
"""A reference to the value returned by this call should be stored for the duration of the
authentication exchange.
"""
store = getattr(conn,'auth_storage',None)
if store is None:
store = AuthStorage(conn)
# So that the connection object doesn't hold on to password data too long:
conn.auth_storage = weakref.ref(store)
return store
Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def __init__(self, conn):
self.conn = conn
self.pw = ''

def store_pw(self,pw):
if self.conn.account.env_file:
self.set_env_password(pw)
else:
self.pw = pw

def retrieve_pw(self):
if self.conn.account.env_file:
return self.get_env_password()
return pw


def load_plugins(subset=set(), _reload=False):
if not subset:
subset = set(__all__)
Expand All @@ -18,9 +72,66 @@ def load_plugins(subset=set(), _reload=False):
return dir_


# TODO(#499): X models a class which we could define here as a base for various server or client state machines
# as appropriate for the various authentication types.
class REQUEST_IS_MISSING_KEY(Exception): pass


def throw_if_request_message_is_missing_key( request, required_keys ):
for key in required_keys:
if not key in request:
raise REQUEST_IS_MISSING_KEY(f"key = {key}")


def _auth_api_request(conn, data):
message_body = JSON_Message(data, conn.server_version)
message = iRODSMessage('RODS_API_REQ', msg=message_body,
int_info=api_number['AUTHENTICATION_APN']
)
conn.send(message)
response = conn.recv()
return response.get_json_encoded_struct()


__FLOW_COMPLETE__ = "authentication_flow_complete"
__NEXT_OPERATION__ = "next_operation"


CLIENT_GET_REQUEST_RESULT = 'client_get_request_result'
FORCE_PASSWORD_PROMPT = "force_password_prompt"


class authentication_base:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

self.scheme = scheme

def call(self, next_operation, request):
logging.info('next operation = %r', next_operation)
old_func = func = next_operation
while isinstance(func, str):
old_func, func = (func, getattr(self, func, None))
func = (func or old_func)
if not func:
raise RuntimeError("client request contains no callable 'next_operation'")
resp = func(request)
logging.info('resp = %r',resp)
return resp

def authenticate_client(self, next_operation = "auth_client_start", initial_request = {}):

to_send = initial_request.copy()
to_send["scheme"] = self.scheme

while True:
resp = self.call(next_operation, to_send)
if self.loggedIn:
break
next_operation = resp.get(__NEXT_OPERATION__)
if next_operation is None:
raise ClientAuthError("next_operation key missing; cannot determine next operation")
if next_operation in (__FLOW_COMPLETE__,""):
raise ClientAuthError(f"authentication flow stopped without success: scheme = {self.scheme}")
to_send = resp

class X:
pass
logging.info("fully authenticated")
142 changes: 136 additions & 6 deletions irods/auth/native.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,141 @@
import base64
import logging
import hashlib
import struct

from irods import MAX_PASSWORD_LENGTH

from . import (__NEXT_OPERATION__, __FLOW_COMPLETE__,
AuthStorage,
authentication_base, _auth_api_request,
throw_if_request_message_is_missing_key)


def login(conn):
conn._login_native()
authenticate_native(conn,
req = {'user_name': conn.account.proxy_user,
'zone_name': conn.account.proxy_zone} )


_scheme = 'native'


def authenticate_native( conn, req = None ):

logging.info('----------- %s (begin)', _scheme)

# 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}
Comment on lines +27 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


native_ClientAuthState(
conn,
scheme = _scheme
).authenticate_client(
# TODO - Q: should we rename 'initial_request' as 'context'?
initial_request = req
)

logging.info('----------- %s (end)', _scheme)


class native_ClientAuthState(authentication_base):

def auth_client_start(self, request):
resp = request.copy()
# user_name and zone_name keys injected by authenticate_client() method
resp[__NEXT_OPERATION__] = self.AUTH_CLIENT_AUTH_REQUEST # native_auth_client_request
return resp

# Client defines. These strings should match instance method names within the class namespace.
AUTH_AGENT_START = 'native_auth_agent_start'
AUTH_CLIENT_AUTH_REQUEST = 'native_auth_client_request'
AUTH_ESTABLISH_CONTEXT = 'native_auth_establish_context'
AUTH_CLIENT_AUTH_RESPONSE = 'native_auth_client_response'

# Server defines.
AUTH_AGENT_AUTH_REQUEST = "auth_agent_auth_request"
AUTH_AGENT_AUTH_RESPONSE = "auth_agent_auth_response"

def native_auth_client_request(self, request):
server_req = request.copy()
server_req[__NEXT_OPERATION__] = self.AUTH_AGENT_AUTH_REQUEST

resp = _auth_api_request(self.conn, server_req)

resp[__NEXT_OPERATION__] = self.AUTH_ESTABLISH_CONTEXT
return resp

def native_auth_establish_context(self, request):
throw_if_request_message_is_missing_key(request,
["user_name", "zone_name", "request_result"])
request = request.copy()

password = ''
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()
Comment on lines +78 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@d-w-moore d-w-moore Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)


if not password:
password = self.conn.account.password or ''

challenge = request["request_result"].encode('utf-8')
self.conn._client_signature = "".join("{:02x}".format(c) for c in challenge[:16])

padded_pwd = struct.pack(
"%ds" % MAX_PASSWORD_LENGTH, password.encode(
'utf-8').strip())
Comment on lines +89 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with the code so pardon if this is an obvious question.

Are we always supposed to send a MAX_PASSWORD_LENGTH here? If we are, do we check that password does not exceed the length within struct or elsewhere in the code?


m = hashlib.md5()
m.update(challenge)
m.update(padded_pwd)

encoded_pwd = m.digest()
if b'\x00' in encoded_pwd:
encoded_pwd_array = bytearray(encoded_pwd)
encoded_pwd = bytes(encoded_pwd_array.replace(b'\0', b'\1'))
request['digest'] = base64.encodebytes(encoded_pwd).strip().decode('utf-8')
Comment on lines +97 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this part. Is this replacing 'null' (\0) with what \1 is? If yes, what's the reason behind this?


request[__NEXT_OPERATION__] = self.AUTH_CLIENT_AUTH_RESPONSE
return request

def native_auth_client_response (self,request):
throw_if_request_message_is_missing_key(request,
["user_name", "zone_name", "digest"])

server_req = request.copy()
server_req[__NEXT_OPERATION__] = self.AUTH_AGENT_AUTH_RESPONSE
resp = _auth_api_request(self.conn, server_req)

self.loggedIn = 1;
resp [__NEXT_OPERATION__] = __FLOW_COMPLETE__
return resp

#if __name__ == '__main__':
# from sys import argv
Comment on lines +118 to +119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to remove this.

def main(*argv):

User, Zone, Pw = argv[1:4]

import irods.account, irods.pool, irods.connection

account = irods.account.iRODSAccount(
'localhost',1247,
User, Zone,
password = Pw,
irods_authentication_scheme = _scheme
)

# TODO (#499): Here, we could define client & server auth_state classes (ie state machines mimicking the mechanics
# of 4.3+ iCommands/iRods-runtime authentication framework), using this pattern for an inheritance hook.
from . import X as X_base
pool = irods.pool.Pool(account)
connection = irods.connection.Connection(pool, account, #connect=False #TODO delete
)

authenticate_native(
connection,
req = {'user_name': account.proxy_user,
'zone_name': account.proxy_zone} )

class X(X_base):
pass
Loading