pam_pkcs11 icon indicating copy to clipboard operation
pam_pkcs11 copied to clipboard

pam_pkcs11.c: Reduce verbosity when authenticating

Open andsens opened this issue 2 years ago • 8 comments

Respect the quiet config flag in more places. The "Welcome ..." message has been integrated into the "PIN: ..." prompt.

I am a complete novice at C, so kindly take extra care in verifying this, especially the PIN prompt.
I don't know if the change introduces an overflow issue because the token label can have an arbitrary length.

andsens avatar Jul 13 '23 11:07 andsens

ping Any chance of this getting reviewed/merged?

andsens avatar Sep 21 '23 08:09 andsens

Hi. In PAM there are two types of messages (informational, error) and two types of user conversation queries (entering cleartext data and entering secret data). All of them are needed as the front-end applications could display different kinds of messages in different windows, etc. So it is OK to have a "Welcome" informational message.

On the other hand, there is a problem with the use of quiet option in code: somewhere it blocks syslog output, somewhere it blocks PAM informational (and sometimes error!) messages, somewhere both. Probably, it would be better to have two independent configuration options: one for syslog and another for PAM informational messages. Is there a real need to block PAM error messages? No, I don't think so…

wolneykien avatar Sep 21 '23 13:09 wolneykien

The primary motivation for this PR is simply that running sudo something.sh ends up clobbering the output. Users know that [sudo] password for user: in the output when running a script is not actually the script, but them running sudo. But once you add up to 3 other informational messages (with no [sudo] prefix), it becomes an unnecessary mental load to filter that out when parsing output.

I understand that the messages could be useful informational output in other contexts (like a GDM password prompt), but even then I wouldn't want it to output anything. One principle of a unix tool is to be quiet when everything went well and to be noisy when things go awry.

andsens avatar Sep 21 '23 13:09 andsens

As a first step, it would be okay to make quiet configuration option to suppress PAM informational (but not error!) messages along with syslog. Can you update the code that way?

wolneykien avatar Sep 21 '23 13:09 wolneykien

I'm sorry but I fail to see which one of the changes affects an error message. Maybe you could add a note to the line(s) that is the issue?

andsens avatar Sep 21 '23 14:09 andsens

Sorry, I've changed my mind. Now I think, that the effect of quiet option should be left unchanged: let it suppress error messages (both in syslog and in PAM). However, your goal is to suppress informational messages and it's better to have a separate option for that, say noinfo?

A nice thing is that all such messages are displayed via pam_prompt() proc. So, in order to suppess PAM_TEXT_INFO messages it would be enough to filter them in that single place. And in order to know whether they should be suppressed, we need a configuration handle. So, the proc should be modified in a way something like this:

static int pam_prompt(struct configuration_st *configuration, pam_handle_t *pamh, int style, char **response, char *fmt, ...)
{
...
    if (configuration->noinfo && style == PAM_TEXT_INFO)
        return;
}

I.e., it should return with no output if noinfo is set and the style of message is PAM_TEXT_INFO. Of course, you then need to pass configuration to all invocations of pam_prompt() and to add noinfo itself to the configuration structure and its parser.

wolneykien avatar Sep 21 '23 15:09 wolneykien

I can see the advantage of doing it like that (alternatively you could maybe mask PAM_TEXT_INFO based on the config). However, that is quite outside the scope of both the PR and my capabilities.
Would it make sense to merge this as a stop-gap measure until the new pam_prompt() is implemented?

andsens avatar Sep 27 '23 07:09 andsens

*ping

andsens avatar Oct 18 '23 09:10 andsens