Added authgroup PAM option for challenge-response mode

[JonnyWhatshisface]

Okay, my apologies on how long it took me to get back to this. I moved across the globe and have been settling in and adjusting. Between the new job and trying to get situated, time has been a little chaotic.

I can't recall all the recommendations you originally made when I first submitted this patch, but I did my best to make sure I covered as much as I could remember. I used getgrnam() to pull a list of members in the group and compared the user to that versus getting a list of groups the member was in. I also eliminated the malloc()/free() as it wasn't needed. I've added D(()) and DBG(()) statements as well.

Please let me know if you see anything else you can point out/improve on. I'm currently using this pam module on two OS X machines and a Slackware box. So far, so good.

I had botched up the repo and really didn't want to go through the hassle of cleaning it up, so I simply dropped the fork and recreated, pushed the changes and requested another pull.

[coveralls]

Coverage decreased (-1.1%) to 49.188% when pulling c8826b4 on JonnyWhatshisface:master into b7e7da4 on Yubico:master.

[klali]

I'm not convinced PAM_SUCCESS is the appropriate return value here, PAM_IGNORE or PAM_USER_UNKNOWN might be more appropriate (but might also require more complex config).

[JonnyWhatshisface]

I think you're right. I believe PAM_IGNORE would be more appropriate, as PAM_USER_UNKNOWN is going to have an adverse affect if the particular auth module is required by the PAM config. If PAM_IGNORE is returned, it should force PAM to skip it - whether it's required or optional... I'll do some playing with it.

[klali]

No worries on the time, stuff happens.

I've left a couple of minor inline comments, please fix them or tell me that I'm wrong.
When you're fixing them you can amend (or rebase) your earlier commit and force push it so the pull request will be updated.

[JonnyWhatshisface]

Klas, getgrnam_r() requires I pass it the buffer I want to store the group structure in (which I'm sure you're aware of). Right now as it stands, if I get an ERANGE error (indicating I did not allocate a big enough buffer), I handle it by multiplying the size of the buffer by 2 (starts at 1024). How many times would you suggest this be done before calling it gives and returning a PAM_IGNORE ? I think perhaps checking the groups the user is in might be more feasible due to this caveat: I currently work in an infra where a single group literally has 70k users... getgrnam_r() in an environment like that seems a bit inefficient.

Is there a benefit over enumerating the users of the group versus enumerating the groups of the user?

[klali]

You mean by using getgrouplist()?
Since it's not portable.. Does it exist on all platforms (atleast linux, osx, freebsd & netbsd) we care about? Does it behave in a coherent way on them?
my thinking with getgrnam_r() was to just give it a big enough buffer.. but that might be unworkable and need realloc in a loop instead.

[JonnyWhatshisface]

getgrouplist() is on Linux, OSX, FBSD and NBSD from what I can tell in man pages/digging. I've tested it on Linux, OSX and FreeBSD - I don't have a NetBSD installation to test on, but can always spin up a VM.

The getgrnam_r() does require a realloc, the concern is how many times it might require it. Also, not certain why or where I came up with a 70k user group - I didn't realize I typed that. It's actually 700 users in the single group, and I didn't catch that until I re-read it. But either way, 700 is a lot of users and a lot of realloc's to size the buffer appropriately... Plus, it results in a lot more data to sift through, reducing the performance.

Generally, you're more likely to have a scenario where members of a specific group is a much larger count that groups of a member. So, I think getgrouplist() might be a bit of a better fit with regards to aiming towards larger organizations...

Another way it can be handled would be for me to check in advance for the existence of getgrouplist() and use it if it's there. If not, check for getgrnam_r() or getgrent(), as I believe getgrouplist() doesn't exist on AIX and I believe getgrnam_r() is also broken on AIX. It would add some ifdef's to the equation, but would be worth it in the long run.

I'm also writing an autogen.sh that is going to handle checking for and downloading/installing all of the pre-requisites for compile. Getting everything put together to compile properly on OS X was a very long and drawn out process, and I'm sure the same fun will get encountered when I spin up the NetBSD VM :)

[klali]

I think we should avoid having different implementations ifdef'd, the performance hit for that loop should be unnoticeable, but the complexity of it might push us away from it.
If all platforms we care about have getgrouplist() and it behaves similar enough on them lets use it (add a configure check and error for non-existance). It might also be a good idea to add this as a function in util and have a very simple test for it (but I can do that workaround later).

Please add the autogen.sh contribution in a pull request of it's own =)