Exit status when user is not member of group

[candlerb]

Perhaps I am being extremely dense, but I am scratching my head when I look at the code here:

	if len(group) == 0 || isMemberOf(group) {
		return authenticate(w, uid, username, userCA, authorizedPrincipals)
	}

	return AuthSuccess

It seems to me that if the "group" option is set, but the user is not a member of that group, the module will unconditionally return AuthSuccess - which implies that sudo access is granted.

I was expecting the final statement to say return AuthError. If that's true, it could be more clearly expressed as:

	if len(group) > 0 && isMemberOf(group) != true {
		return AuthError
	}

	return authenticate(w, uid, username, userCA, authorizedPrincipals)

However if the current behaviour is as intended, then I think the documentation of the "group" option could be made much clearer. "Users who are in this group must authenticate with ssh-agent; all other users are granted access without authenticating at all"

[srstsavage]

Yeah, this seems strange to me too. Maybe the last line in pamAuthenticate should just be changed to return AuthError?

[pmoody-]

this whole bit of code can probably be removed. I added it initially because I wanted to make the deployment of pam_ussh as easy as possible, so we could say that if a user wasn't in a particular group then they'd use whatever the "old" pam sudo configuration was.

this was mitigation for deploying a potentially breaking change in prod.

is this option useful to you at all?

[pmoody-]

also, I left uber ~6 months ago or so and this project hasn't been maintained by anyone since I left. I'm going to fork this, and probably rename it.

[candlerb]

I don't need this option.

It just seemed weird; I would have thought that if the intention was to fall through to subsequent modules, it would have returned PAM_IGNORE rather than PAM_SUCCESS.

[srstsavage]

Yeah, I think the pam configuration itself should be adjusted with required/requisite if other modules are to be used instead of pam-ussh returning success if no auth check was actually performed.

also, I left uber ~6 months ago or so and this project hasn't been maintained by anyone since I left. I'm going to fork this, and probably rename it.

That's good to hear, I've been evaluating if this project is useful for us and likely have a few pull requests coming, was worried that it may have been abandoned. Can you ping this issue when you fork the repo?

[jessespears]

@shane-axiom I will be available to review any pull requests that come this way. @pmoody- if you decide to fork this repo then I will consider your repo to be the upstream.