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

Add legacy PAM plugin with new authentication plugin framework (main) #6358

Merged
merged 2 commits into from
May 6, 2022

Conversation

alanking
Copy link
Contributor

I'm going to leave this in draft for now as this is still being tested.

Currently, the name of this plugin is "pam". We have discussed using "pam_password" for this as the "interactive" PAM plugin being developed in the Authentication Working Group will completely replace this later, and the existing implementation is a simple password verification.

Also up for discussion: Should this be a separate plugin which exists outside of the main server?

@alanking alanking marked this pull request as draft April 26, 2022 21:20
@korydraughn
Copy link
Collaborator

I don't see any reason why it couldn't live in its own repo. After all, it is a plugin.

We'd need to weight the pros and cons of that.

Copy link
Contributor Author

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Some things I noticed and... pre-defensive comments :)

lib/core/include/irods/authentication_plugin_framework.hpp Outdated Show resolved Hide resolved
lib/core/include/irods/authentication_plugin_framework.hpp Outdated Show resolved Hide resolved
lib/core/include/irods/authentication_plugin_framework.hpp Outdated Show resolved Hide resolved
plugins/auth/src/native.cpp Show resolved Hide resolved
plugins/auth/src/pam.cpp Outdated Show resolved Hide resolved
plugins/auth/src/pam.cpp Show resolved Hide resolved
plugins/auth/src/pam.cpp Show resolved Hide resolved
plugins/auth/src/pam.cpp Outdated Show resolved Hide resolved
plugins/auth/src/pam.cpp Outdated Show resolved Hide resolved
lib/core/src/clientLogin.cpp Outdated Show resolved Hide resolved
@alanking alanking marked this pull request as ready for review May 4, 2022 19:27
@alanking
Copy link
Contributor Author

alanking commented May 4, 2022

Need to run this through tests a couple of times and try the PAM plugin manually, but I think this is ready for review now.

@alanking
Copy link
Contributor Author

alanking commented May 4, 2022

By the way, still okay with throwing this into its own repository as a separate plugin. We should at least have a Great Naming for the plugin because it cannot change once we ship it. The work done in this branch leading up to the actual PAM plugin needs to be merged as soon as it's ready. We can always remove the PAM plugin from the main repository before we ship 4.3.0.

@alanking
Copy link
Contributor Author

alanking commented May 5, 2022

I've seen all of the tests pass at this point, but I'm now seeing some failures when the tests are run in a certain order, so I'm looking into this.

@alanking
Copy link
Contributor Author

alanking commented May 6, 2022

The tests which were previously failing are now passing.

It seems that some iCommands may be using various methods of specifying an authentication scheme and so we need to guard against this in the client library (clientLogin) wrapping the call to the new authentication framework library call.

@korydraughn
Copy link
Collaborator

Do you see potential issues around the handling of the auth scheme across various icommands

Are they blockers?

@alanking
Copy link
Contributor Author

alanking commented May 6, 2022

No, the handling should now be at least as robust as things have ever been as of the latest commit.

@korydraughn
Copy link
Collaborator

Ok. Given that all of the tests are now passing, I think we're good to merge this.

Adds documentation to existing authentication plugin framework class and
free functions as well as improving usability and some formatting
issues.

Adds a new function to the authentication plugin framework header which
allows a client or server to determine which authentication method it
ought to be using in a unified fashion. The implementation at this time
is, of course, a simple version check, but it could be based on other
criteria which can be queried from an RcComm.

The original auth plugin framework implementation includes some common
functions which are now available to any plugin implementing the
framework. These include a request message verification function and a
convenience function for invoking the client API endpoint for performing
an authentication plugin operation.
This ports the legacy PAM authentication plugin to the new
authentication plugin framework. It performs a simple password check
using the PAM API in a separately built application which is called from
the plugin on the server. Then, the native authentication flow is
employed to complete authentication with the iRODS server.
@alanking
Copy link
Contributor Author

alanking commented May 6, 2022

Okay, confirmed that all the tests passed (in 2 hours 45 minutes!) and squashed commits are okay. Adding #'s and will merge on green unless there are objections/conflicts

@alanking alanking merged commit 419734e into irods:main May 6, 2022
@alanking alanking deleted the 5479.pam branch May 6, 2022 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants