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

Refactor authentication plugin interface leveraging Authentication Working Group design #5479

Closed
3 tasks done
jasoncoposky opened this issue Mar 9, 2021 · 14 comments
Closed
3 tasks done
Assignees
Milestone

Comments

@jasoncoposky
Copy link
Member

jasoncoposky commented Mar 9, 2021

A new authentication plugin interface has been created via the AWG. This new mechanism will be ported to the main iRODS repository.

Consider backwards compatibility with older clients as well as federation.

  • Native
  • PAM
  • Kerberos

The following will not be carried forward into 4.3.x (see https://groups.google.com/g/iROD-Chat/c/sSlBMvI-21o):

  • OS Auth
  • GSI
@jasoncoposky jasoncoposky added this to the 4.3.0 milestone Mar 9, 2021
@jasoncoposky jasoncoposky self-assigned this Mar 9, 2021
@alanking alanking self-assigned this Jan 25, 2022
@alanking
Copy link
Contributor

Just leaving a note here for tracking progress...

  • Pulled all code from AWG into irods/irods
  • Code builds
  • Wired native authentication plugin into clientLogin so that iCommands use new authentication method

Now I'm experiencing SYS_UNMATCHED_API_NUM when attempting to start a catalog consumer server.

@trel
Copy link
Member

trel commented Jan 31, 2022

That's... allow list?

@alanking
Copy link
Contributor

No, this is just not finding the API number for the new API plugin. I have confirmed that I'm running the same SHA on the provider, which works, and the API plugin exists in the expected location on the consumer. I think this may point to another spot in our logic where the API table is not being loaded in all the places it should. Current guess is that the API table which loads plugins is being initialized on agent startup, not on server startup.

@trel
Copy link
Member

trel commented Jan 31, 2022

ah.

aka, too late.

@trel
Copy link
Member

trel commented Jan 31, 2022

but also, so that it's dynamic... hmm...

@alanking
Copy link
Contributor

alanking commented Feb 1, 2022

The issue with CSC servers has been resolved. The client API table needs to be loaded when standing up the server in order to use the authentication plugins when querying the CSP for information.

Now investigating server authentication failures across federation.

alanking added a commit to alanking/irods_client_icommands that referenced this issue Feb 3, 2022
trel pushed a commit to irods/irods_client_icommands that referenced this issue Feb 3, 2022
alanking added a commit to alanking/irods that referenced this issue Feb 14, 2022
Adds a new method to version.hpp which takes a string and constructs an
irods::version instance. If the instance cannot be constructed due to a
malformed input or an error, std::nullopt is returned. Also adds unit
tests for the new function.
alanking added a commit to alanking/irods that referenced this issue Feb 14, 2022
This change adds a new authentication plugin framework. The basis for
this work was originally developed for the Authentication Working Group
and its implementation and documentation can be found here:

  https://github.com/irods-contrib/irods_working_group_authentication

The framework provides a single function called authenticate_client
which allows a connected client user to be authenticated. The only
plugin which has been implemented using the new framework so far is the
iRODS native authentication. The legacy authentication plugin will
remain in place in order to support talking to older servers with newer
clients and servers.

This change also adds a new API plugin which loads an auth plugin and
calls the operation specified by "next_operation" in the JSON input.
This is used by the authentication plugins within the framework to make
requests and receive responses from the server.

The new authentication plugin framework has been integrated with
clientLogin so that all authentication requests are routed through the
new framework. Authentication requests for clients connected to servers
before 4.3.0 will use the older authentication flow.

As a result of using the authentication framework, a new requirement for
server startup has been introduced: API plugins must be loaded at server
startup in order for the server to start. So, part of this change also
loads the client and server API tables on server startup so that any
server startup operations which require API plugins can be performed.
The delay server also requires that the client API table be loaded on
startup so that any API plugins required for communication with the
server can be loaded.
alanking added a commit to alanking/irods that referenced this issue Feb 14, 2022
alanking added a commit that referenced this issue Feb 14, 2022
Adds a new method to version.hpp which takes a string and constructs an
irods::version instance. If the instance cannot be constructed due to a
malformed input or an error, std::nullopt is returned. Also adds unit
tests for the new function.
alanking added a commit that referenced this issue Feb 14, 2022
This change adds a new authentication plugin framework. The basis for
this work was originally developed for the Authentication Working Group
and its implementation and documentation can be found here:

  https://github.com/irods-contrib/irods_working_group_authentication

The framework provides a single function called authenticate_client
which allows a connected client user to be authenticated. The only
plugin which has been implemented using the new framework so far is the
iRODS native authentication. The legacy authentication plugin will
remain in place in order to support talking to older servers with newer
clients and servers.

This change also adds a new API plugin which loads an auth plugin and
calls the operation specified by "next_operation" in the JSON input.
This is used by the authentication plugins within the framework to make
requests and receive responses from the server.

The new authentication plugin framework has been integrated with
clientLogin so that all authentication requests are routed through the
new framework. Authentication requests for clients connected to servers
before 4.3.0 will use the older authentication flow.

As a result of using the authentication framework, a new requirement for
server startup has been introduced: API plugins must be loaded at server
startup in order for the server to start. So, part of this change also
loads the client and server API tables on server startup so that any
server startup operations which require API plugins can be performed.
The delay server also requires that the client API table be loaded on
startup so that any API plugins required for communication with the
server can be loaded.
alanking added a commit that referenced this issue Feb 14, 2022
alanking added a commit to alanking/irods_client_icommands that referenced this issue Feb 14, 2022
alanking added a commit to irods/irods_client_icommands that referenced this issue Feb 14, 2022
@alanking
Copy link
Contributor

I broke the following unit tests as they are not loading the client API plugins:

==== begin test run results ====
-----
results for [ubuntu-1804-postgres-1012_irods-catalog-provider_1]
        test list:
                [irods_client_connection]
                [irods_connection_pool]
                [irods_query_builder]
                [irods_resource_administration]
                [irods_user_administration]
                [irods_zone_report]
        failed tests:
                [irods_client_connection]
                [irods_connection_pool]
                [irods_query_builder]
                [irods_resource_administration]
                [irods_user_administration]
                [irods_zone_report]
        return code:[1]
-----
List of failed tests:
        irods_connection_pool irods_query_builder irods_resource_administration irods_user_administration irods_zone_report
Return code:[1]
==== end of test run results ====

alanking added a commit to alanking/irods that referenced this issue Feb 16, 2022
Some unit tests require a running server so that they can test client
APIs through a client connection. The authentication in 4.3.0 requires
that client API plugins be loaded so that the new authentication plugin
framework can be used. Calls to do so have been added to all affected
unit tests.
alanking added a commit that referenced this issue Feb 17, 2022
Some unit tests require a running server so that they can test client
APIs through a client connection. The authentication in 4.3.0 requires
that client API plugins be loaded so that the new authentication plugin
framework can be used. Calls to do so have been added to all affected
unit tests.
@alanking
Copy link
Contributor

alanking commented Mar 1, 2022

At the most recent AWG meeting (https://github.com/irods-contrib/irods_working_group_authentication/blob/main/20220125-minutes.md), we discussed implementing the legacy PAM auth plugin using the new plugin framework. This is now done.

Presently, the PAM plugin is very closely tied to the native authentication plugin, to the point where certain database operations only used in the native authentication plugin are PAM-aware. The flow goes something like this:

  1. The PAM user must run iinit. An initial call to clientLogin initiates the PAM flow wherein the PAM password is prompted and used to generate a password which is stored in the catalog. This is the only place where the PAM plugin operations are actually invoked. At this point, we are dealing only in PAM credentials and authentication with the server has not occurred. Furthermore, the RcComm is not marked as loggedIn by the plugin upon completion.
  2. iinit continues on and authenticates with the server with a second clientLogin call using native authentication with the generated password.
  3. The client is then authenticated and all other iCommands use native authentication to authenticate with the server until the password expires according to the TTL parameter, at which time the user must run iinit again.

There is now a fork in the road here as to how we choose to support this:

  1. We keep things as they are now and the PAM plugin has to "override" the scheme provided in the environment with native.
  2. The PAM authentication plugin absorbs the native authentication plugin. This can be done by simply copying and pasting the operations from the native plugin into the PAM plugin.
  3. The PAM authentication plugin can attempt to initiate the native plugin flow FROM the PAM plugin (i.e. load the native plugin and call the start operation from there).

After the legacy PAM authentication plugin is completed using one of the 3 options above, we can start talking about how we would like to implement a new PAM plugin that makes more sense :)

@alanking
Copy link
Contributor

alanking commented Mar 1, 2022

From the "fork in the road" options above, I have successfully demonstrated implementations for 1 and 3. I am leaning toward option 3 at this point because I really don't like the partial authentication implementation provided by the legacy PAM plugin. Thoughts?

@trel
Copy link
Member

trel commented Mar 2, 2022

3 feels... the most robust, if only because of a single source of truth for 'how to native'.

alanking added a commit to irods/irods_client_icommands that referenced this issue May 6, 2022
When using the new PAM plugin which conforms to the authentication
plugin framework for 4.3.0, there is a new flow of which iinit needs to
be made aware. However, we need to be able to authenticate with servers
using the older plugin as well, so this change threads the needle to add
support for the new plugin while maintaining support for the old.
alanking added a commit to alanking/irods_auth_plugin_kerberos that referenced this issue May 7, 2022
alanking added a commit to alanking/irods_auth_plugin_kerberos that referenced this issue May 7, 2022
alanking added a commit to irods/irods_auth_plugin_kerberos that referenced this issue May 7, 2022
alanking added a commit to irods/irods_auth_plugin_kerberos that referenced this issue May 7, 2022
@alanking
Copy link
Contributor

alanking commented May 7, 2022

All that's left is taking a pass at iinit to remove the plugin-specific references. This can also be done at a later time, potentially

alanking added a commit to alanking/irods that referenced this issue May 14, 2022
alanking added a commit to alanking/irods that referenced this issue May 14, 2022
- Fixed JSON object building for auth plugin context
- Removed useless messages when -V option used with iinit
- Added newline after password prompt in native auth plugin
alanking added a commit to alanking/irods that referenced this issue May 14, 2022
Also add test_auth to the list of tests to be run.
alanking added a commit to alanking/irods_client_icommands that referenced this issue May 14, 2022
alanking added a commit to alanking/irods_client_icommands that referenced this issue May 14, 2022
alanking added a commit to alanking/irods that referenced this issue May 16, 2022
Also add test_auth to the list of tests to be run.
alanking added a commit that referenced this issue May 16, 2022
alanking added a commit that referenced this issue May 16, 2022
- Fixed JSON object building for auth plugin context
- Removed useless messages when -V option used with iinit
- Added newline after password prompt in native auth plugin
alanking added a commit that referenced this issue May 16, 2022
Also add test_auth to the list of tests to be run.
alanking added a commit to irods/irods_client_icommands that referenced this issue May 16, 2022
alanking added a commit to irods/irods_client_icommands that referenced this issue May 16, 2022
@alanking
Copy link
Contributor

Okay, I lied. There is one last task: renaming the new pam plugin to pam_password. I have this change in a branch and will open a PR once I confirm that tests are passing.

alanking added a commit to alanking/irods that referenced this issue May 19, 2022
The server-side authentication operations are not implemented or
supported in the new authentication plugin framework. This is partially
because the most-used/supported plugins do not implement these
operations and the ones that do implement the operations can probably
invoke the server-side functionality as part of the client
authentication flow. For now, we will not be compiling these bits as
they will be unused in 4.3.0.
alanking added a commit to alanking/irods that referenced this issue May 19, 2022
alanking added a commit to alanking/irods that referenced this issue May 19, 2022
alanking added a commit that referenced this issue May 19, 2022
The server-side authentication operations are not implemented or
supported in the new authentication plugin framework. This is partially
because the most-used/supported plugins do not implement these
operations and the ones that do implement the operations can probably
invoke the server-side functionality as part of the client
authentication flow. For now, we will not be compiling these bits as
they will be unused in 4.3.0.
alanking added a commit to alanking/irods_client_icommands that referenced this issue May 19, 2022
Due to the nature of iinit, certain code paths are taken for certain
auth plugins and not others. These are referenced directly by name in
the client application and not done in any sort of generic fashion. We
plan to implement a new iinit in the future with the iRODS client CLI,
so for now we will go along with the current implementation and directly
reference the pam_password plugin as one which should not display a
password prompt.
alanking added a commit to irods/irods_client_icommands that referenced this issue May 19, 2022
Due to the nature of iinit, certain code paths are taken for certain
auth plugins and not others. These are referenced directly by name in
the client application and not done in any sort of generic fashion. We
plan to implement a new iinit in the future with the iRODS client CLI,
so for now we will go along with the current implementation and directly
reference the pam_password plugin as one which should not display a
password prompt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants