#92 Added PAM authentication support
Closed by tkopecek. Opened by ctria.
ctria/koji pam_authentication  into  master

Download 92.patch

Adds support for PAM authentication for the koji-hub and BasicAuth for the koji-web.

This is useful for our internal use case as it allows us to login without the overhead of setting up either a CA or a kerberos realm for our users.

The configuration is backwards compatible and hopefully similar to the other authentication methods.

To active PAM support on hub you define the option:
PAMService = koji
in hub.conf. The value will be the name of the PAM service. Note the call to the PAM module is done via unprivileged call thus the use of pam_unix
won't be possible.

Note that activating this option will have as result that username/password combinations from the DB will no longer be checked (similarly to when activating kerberos or SSL client auth).

The BasicAuth for koji-web requires 2 changes:
a) To enable WSGIPassAuthorization for /koji/login in httpd configuration. That passes the authorization variable from the apache to the application.
b) Set the "BasicAuthRealm" option to the Basic Authentication Realm that will be presented to the user to login.

Finally python-pam package has been added to the hub's dependencies.

Thanks for the patch. At a glance the code looks fine, though I have a few concerns.

I'll start with the easy one. I'd rather not add a new dependency to cover this case. I think most people will continue to use other forms of auth. We could make this a soft dep, wrap the import in a try, and only error out if the option is actually enabled.

Now on to the harder one. The current password auth in koji is terrible and I recommend folks not use it (except perhaps for testing/debugging). I'm hesitant to add a feature like this without making regular password auth usable, particularly in the cli

Thank you for the review!

For the first one, agreed. I'll change that.

For the second I'm not sure I understand what you mean, would a password prompt instead of expecting --password do the trick?

2 new commits added

  • Removed python-pam requirement, dynamically enable PAM support instead
  • Prompt for username/password if they are not provided

@mikem does updated commit cover your concerns? Let me know if there is anything else that will be needed.

@mikem @ctria Any chance this would get merged in any time soon? The changes look fine to me, just from taking a look at the change diff.

I'm waiting on a new review from @mikem. Happy to rebase it to current master if that is the issue.

@mikem do you have any news on that? How should we proceed? Happy to rebase it if that is the only blocking issue.

rebased

rebased

https://pagure.io/koji/issue/1556

@ctria I'm going through the Koji backlog... is this issue still something that you are looking for?

@tkopecek let's drop this pr as it's quite old and unclear whether it's still needed.

Pull-Request has been closed by tkopecek

Metadata