Return different PAM error codes

Toinews commented 7 years ago

Hi ,

Most of the time, the pam module is returning PAM_SERVICE_ERR or PAM_SUCESS, this is not very helpful when trying to configure different behaviours for the authentication process.

For example, if the user is denied by Duo policy would be better to return a PAM_PERM_DENIED rather than a PAM_SERVICE_ERR.

The same appears if the user declares the login as fraudulent, it would be very useful to stop directly the authentication process (no password fallback, exit the PAM stack directly). But this could only be achieved only if error codes are different. PAM_ABORT should trigger here.

I detected this issue when trying to configure authentication for sudo with duo and I think we could fix this with the following error codes: (It is an example based on the sudo config I want to achieve but the error codes makes sense for every similar situation I think)

If Duo push is OK : 
  PAM_SUCCESS (continue stack or done)
elif Duo servers unreachable or duo push timed out:
  PAM_CRED_UNAVAIL (retry or continue the current pam stack)
elif user rejected auth by mistake:
  PAM_PERM_DENIED (continue through pam stack or die)
elif denied by duo policy:
  PAM_PERM_DENIED (continue through pam stack or die)
else (fraudulent) :  
  PAM_ABORT (exit pam stack immediately)

Thank you very much,

xdesai commented 7 years ago

Hi Antoine, Thanks for this suggestion. Just wanted to drop you a line to let you know we're tracking this. If you have time a PR might speed up the process ;)

Toinews commented 7 years ago

Hi Xander,

I forked your repository (https://github.com/Toinews/duo_unix) to work on a patch because I don't know if I can push to a branch on yours.

Right now, I managed to :

Remove the lib directory, it is replaced by a git submodule to the duo lib C ( no more duplicated code and move from api v1 => api v2) ;
Reconfigure autotools to compile with the submodule ;
Patch an unhandled case in lib_duo (forked too, I'll make a linked PR : https://github.com/Toinews/libduo )
make changes to pam_duo module to use api V2 lib;
Added different PAM error code regarding of the context ;
make the same changes to login_duo.

My only concern is that it is not heavily tested yet. Can you explain me a bit more how your test suite is working cause I didn't manage to make it work on my side? In the worst case, I can make you a PR that you will merge on a feature branch to test and fix.

Thanks,

xdesai commented 7 years ago

Wow! This is a lot of feature work. I appreciate the time and effort you put in to working on it! Some of these things we'd even like to include eventually. I'll be totally honest with you though, the amount of content here is far beyond what I expected based on the initial issue. We sadly won't have the resources to make these kinds of changes to Duo Unix at this time. Is there any way you could make a pull request with just the changes that adjust the relevant pam_duo return codes?

ps. To answer your questions about tests you can run them by simply doing $make check pps. If this functionality is something that you want for yourself even though we can't pull it all in to the main repo you can always create a Duo Unix tarball yourself to be used. This is done by:

$ ./bootstrap
$ ./configure --with-pam --prefix=/usr
$ make
$ make distcheck

Toinews commented 7 years ago

The issue is, at the beginning, I didn't want to do so much work on the project but the API v1 doesn't provide a simple way to detect if it is a fraud, a locked out account or not, etc. The status is a full message (equivalent of status_msg in the API v2) and I wasn't sure the content is immutable and not relying on something specific to the context of the request. This is why I moved to V2 which provides a deterministic way to check the status.

Anyway, I'll see what I can do for just this issue but I'm not sure I'll have time before next weekend. And if you agree, I'll submit a full PR when the tests will be expanded (and run) to handle the new cases.

Thanks for the tests instruction, I'll try it.

Toinews commented 7 years ago

Hello again,

Sorry I didn't have time to work more on this for the past two months... :(

Also I saw that #106 was requesting the port to the V2 API which is a work I did but which needs intensive security review.

Also it seems that the test suite is expecting a Duo server running locally. Could you tell me more about this because I don't find out how to start it ? Is it something you use internally ?

Thanks,

xdesai commented 7 years ago

Oh that's right, I had forgotten that you had moved to v2 with some of your work. I imagine @gregprosser might be interested in seeing that.

The local duo server is just a small python program that mocks out the needed functionality from Duo. If you look in some of the test files like pam_duo-3.t you will see the commands we use to start it up.

$ cd {Your dir}/duo_unix/tests
$ python mockduo.py certs/mockduo.pem

should do the trick :)

AaronAtDuo commented 9 months ago

Right now, this is not on the roadmap, if that changes we can revisit this issue

duosecurity / duo_unix

Return different PAM error codes #102