Remove client-side authentication expiration

[alanking]

The plugin independently tracks the "expiration" of a PAM session by recording an __expire__ key in the .irodsA.json file and checking this to see whether the user needs to re-authenticate with PAM. The expressed purpose of this functionality is to avoid the authentication error which comes from iRODS when the randomly generated password expires: https://github.com/irods/irods_auth_plugin_pam_interactive/blob/09978a3e572b4e950484be1075c04be6abafda2d/plugin/src/main.cpp#L208-L213

This is done by recording the TTL in seconds in the JSON response which is later used to record the value into the .irodsA.json file. There are a number of checks here which are omitted (most notably, integer overflow). I think we need to at least include some safety checks around this feature. Perhaps we remove this feature. Just writing this down so attention is drawn to it.

[korydraughn]

Nice observation. I agree.

[alanking]

Just writing down some words about how this feature works...

It works by setting an expiration time in the future based on a calculation of a value called ttl_seconds. This value is first based on the TTL value stored in the request JSON structure: https://github.com/irods/irods_auth_plugin_pam_interactive/blob/17eb83e4421940dfffa8e38585e0fa76bab6ca38/plugin/src/main.cpp#L211

This value is the TTL passed in by the caller via the irods::AUTH_TTL_KEY, multiplied by 3600 to get it in units of seconds (because TTL is assumed to be in hours... see https://github.com/irods/irods/issues/7342): https://github.com/irods/irods_auth_plugin_pam_interactive/blob/17eb83e4421940dfffa8e38585e0fa76bab6ca38/plugin/src/main.cpp#L682

The TTL to add to the current time is calculated here based on this value: https://github.com/irods/irods_auth_plugin_pam_interactive/blob/17eb83e4421940dfffa8e38585e0fa76bab6ca38/plugin/src/main.cpp#L217-L223

First, pam_time_to_live_offset (constant value of 60) is subtracted from the value. If this causes the ttl_seconds value to be negative, 0 is used (meaning, the password is considered expired immediately). If the ttl_seconds value minus pam_time_to_live_offset (constant value of 60) causes the ttl_seconds value to be non-positive, a default value of pam_time_to_live_default (constant value of 3600) is used.

In the case where TTL is passed in, the minimum time you can pass in is 1 hour (3600 seconds). So, in the case where TTL of 1 hour is passed in, this feature would cause the password to expire on the client side in... 3540 seconds (3600 - pam_time_to_live_offset... 59 minutes). Once the user attempts to authenticate after 59 minutes, the client-side plugin will catch it and present the configured PAM flow rather than the iRODS authentication error. This seems to work well, although the 60 second window seems pretty arbitrary (why not 1 second?).

In the case where no TTL is passed in (ttl_seconds == 0), the password will be considered expired in 3600 seconds (1 hour). In this case, the server will check the password after password_min_time which defaults to 121 seconds. So, if password_min_time is configured to be less than 3600 and the user attempts to authenticate after password_min_time seconds but after less than 3600 seconds, the iRODS authentication error is presented instead of the configured PAM flow. If the user attempts to authenticate after 3600 seconds, the client-side plugin will catch it and present the configured PAM flow rather than the iRODS authentication error.

This last point is a bug, in my opinion. The point of the feature is to prevent showing the iRODS authentication error (which can be confusing... see https://github.com/irods/irods/issues/7344) but the error still appears if password_min_time is configured to a value less than 3600 seconds. Automated testing would not be practical until we can use TTL values less than 1 hour (again, see https://github.com/irods/irods/issues/7342).

[trel]

I also think the 60 seconds seems arbitrary - and not sure the use case it is designed for. I'm leaning towards removing it completely.

[alanking]

Had some discussion about this...

This seems to work well, although the 60 second window seems pretty arbitrary (why not 1 second?).

I think we figured out the answer to this. There is a window between when the client-side plugin determines that the user's PAM session is not expired and the server checking the randomly generated password in which the password could expire. Here's the check in the client-side plugin... https://github.com/irods/irods_auth_plugin_pam_interactive/blob/17eb83e4421940dfffa8e38585e0fa76bab6ca38/plugin/src/main.cpp#L174-L199

This 60 second value effectively accounts for some minor processing time and network latency. If this is true, 60 seconds is definitely more than enough if not too much.

While hashing that out, we wondered why we need to support this feature in the plugin. As far as we can tell, this feature exists for user convenience. The effect of the feature is to prevent showing the iRODS error to the user when the password expires when running non-iinit iCommands, forcing the user to run iinit again to re-authenticate with PAM. This prevented behavior has been the historical behavior of all supported iRODS authentication schemes - I think - so this is new / unexplored territory.

@HarryKodden / @ccacciari - We think the __expire__ key feature should be removed until a compelling reason is given to keep it. What do you think? Can you think of a use case which would be broken by removing this feature (beyond simple user convenience)? Do you agree or disagree that it should be removed? Thanks!

[ccacciari]

Harry is on holiday now. It is nice to propose to the user to authenticate again, instead of just an error message, however I agree with you that it would be not consistent with the rest of the icommands, so it seems fine to drop that feature.

[alanking]

I've updated the title of the issue to reflect the action taken. We can of course discuss re-introducing this feature at another time. Closing.