Fix issues with username not changing with set_credentials

[regiontog]

I have some problems with the set_credendials function, changing the password works fine but not the username. It seems like pam only ever calls the echo_prompt once. I think the reason for this is that e.g. pam_unix.so uses the pam_get_user function. The documentation says that the returned value is the user specified by pam_start, and if that's null the PAM_USER item, and finally if that's null it tries the pam conversation. The documentation also says it sets the PAM_USER item. So the next time authentication is attempted the user is just read from PAM_USER, not from the conversation.

[1wilkens]

Thanks for the PR! The issue is that the username is not queried again via the prompt if authentication fails once right??

I am actually not sure what should happen in this case as it depends on how the pam_handle is used in the application. If you spin up a new handle for each authentication request, if does not matter. If you reuse the handle, than you maybe should not pass the user to pam_start as it will then always take precedence. But that would probably still only call the prompt once as afterwards PAM_USER is set and reused afterwards.

In any case I am not sure if manually calling prompt_echo here is the correct solution. Maybe we need to find more examples of how pam is used in e.g. other DMs or even better other scenarios?

[regiontog]

Another more intuitive way would be to set the PAM_USER in the set_credentials function. And that is what I wanted to do originally. The problem with that is that the PasswordConv does not have access to the pam_handle. The problem happens even when no user is given in pam_start. The prompt_echo function will never be called more that once by PAM(so long as all modules use pam_get_user). I think it would be fine to create a new handle for each user, but then I feel like the set_credentials function should not accept username? Or at least document the fact that the username won't take effect if an authentication has been attempted.

[1wilkens]

Another more intuitive way would be to set the PAM_USER in the set_credentials function.

That's most certainly a good idea anyways. We should probably do that.

The problem with that is that the PasswordConv does not have access to the pam_handle.

That can be fixed by passing the handle as appdata_ptr although I am not sure if we can call arbitrary PAM functions inside of the conversation function. The man page for pam_conv doesn't mention this as a limitation though..

[regiontog]

That can be fixed by passing the handle as appdata_ptr although I am not sure if we can call arbitrary PAM functions inside of the conversation function. The man page for pam_conv doesn't mention this as a limitation though..

Could it? If so I don't understand how. Afaik if we want the PAM_USER item to be set in set_credentials then I think the handle needs to be in the PasswordConv struct, appdata_ptr would only ever accessible in the prompt_*/ffi functions I think.

[1wilkens]

You're completely right, I forgot that we pass the Converse object via appdata_ptr. We would have to copy the handle to the PasswordConv struct which would certainly be possible but I am not sure if every handler would need the PamHandle. If so, we could generalize that concept somehow (possibly via another intermediate struct that holds the trait object.

[regiontog]

We would have to copy the handle to the PasswordConv struct which would certainly be possible

set_item needs a mutable reference to the PamHandle so I had to make Authenticator access the handle through the intermediate struct.

I am not sure if every handler would need the PamHandle. If so, we could generalize that concept somehow (possibly via another intermediate struct that holds the trait object.

I tried to make a Handler struct that holds both the PamHandle and the Converse object, but since external crates cannot impl for this crate's types, I think it has to be a Associated Type on the Converse trait so that the implementor of Converse can choose a type that they can implement whatever they wish on. The types got pretty messy because of this, they would probably be cleaner if you're fine with Authenticator<'a, DefaultHandler<PasswordConv>> over Authenticator<'a, PasswordConv>

[1wilkens]

Some minor comments and afterwards this should be ready.
I am not quite sure about the API, so I might iterate some more on it (pam is 0.X for a reason ;)) but I'll merge it for now and see what I can come up with.

[1wilkens]

I would call this new to align with existing conventions

Suggested change

	fn create(conversation: Box<C>, handle: &'a mut PamHandle) -> Self;
	fn new(conversation: Box<C>, handle: &'a mut PamHandle) -> Self;

[1wilkens]

Might be my OCD kicking in, but would you swap there two lines? You already implement those two methods in the other order below.

[1wilkens]

Not quite convinced by that name, but I can't come up with something better..

[regiontog]

Some minor comments and afterwards this should be ready.
I am not quite sure about the API, so I might iterate some more on it (pam is 0.X for a reason ;)) but I'll merge it for now and see what I can come up with.

Yeah, I don't really like it either. I think maybe it would be better to just document the way it works for now. And then you can experiment some with the API to find something that is more intuitive. Sorry about the churn though. And I'll be sure to let you know if I find another solution, for now I think I will just be using the 'hack' in open_session for my own purposes.