[sssd PR#804][opened] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/804
Author: jhrozek
 Title: #804: PAM: Also cache SSS_PAM_PREAUTH
Action: opened

PR body:
"""
Related: https://pagure.io/SSSD/sssd/issue/3960

Even if cached_auth_timeout was set, the pam responder would still forward
the preauthentication requests to the back end. This could trigger unwanted
traffic towards the KDCs.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/804/head:pr804
git checkout pr804

[sssd PR#804][comment] PAM: Also cache SSS_PAM_PREAUTH

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

sumit-bose commented:
"""
Hi Jakub,

the patch is working as expected, I only added a minor comment to the code.

While testing I came across two issues where I wonder if you would like to fix them with
this patch as well. 

First, if a wrong password is given cached authentication currently does not fail but
falls back to online authentication. I think this behavior make sense, but might be
unexpected. A sentence in the man page describing this behavior would be useful imo.

Second, there is no clear debug message from the PAM responder that cached authentication
is used. Instead there is

```
(Fri May  3 13:05:11 2019) [sssd[pam]] [pam_reply] (0x0200): pam_reply called with result
[4]: Systemfehler.
```

and later on there are some message from `sysdb_cache_auth()`. Maybe in `pam_reply()` the
`called with result` message can be skipped for cached auth and a more suitable message
can be shown?

Both are not related to the issue at hand so feel free to open a new ticket or ignore
them.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/804#issuecomment-489064168

[sssd PR#804][comment] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

jhrozek commented:
"""
Thank you, I'll look at the System Error. I saw it in my testing, but I stopped after
I realised this was not caused by my patches. I should have at least filed a ticket :-)

About the fallback to online auth - I would actually not expect this myself. But at the
same time, I also see no pressing reason to change this behaviour (if we agreed it should
be changed),, so I agree making this clear in the man page is good enough for now. If our
users or customers would be irritated by the behaviour, we can change it later.
"""

See the full comment at https://github.com/SSSD/sssd/pull/804#issuecomment-489103525

[sssd PR#804][+Changes requested] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

Label: +Changes requested

[sssd PR#804][synchronized] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/804
Author: jhrozek
 Title: #804: PAM: Also cache SSS_PAM_PREAUTH
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/804/head:pr804
git checkout pr804

[sssd PR#804][comment] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

jhrozek commented:
"""
Thank you for the comments, here is the diff:
```
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index c12fef731..03d3a104c 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -3046,7 +3046,9 @@ subdomain_inherit = ldap_purge_cache_timeout
                             Specifies time in seconds since last successful
                             online authentication for which user will be
                             authenticated using cached credentials while
-                            SSSD is in the online mode.
+                            SSSD is in the online mode. If the credentials
+                            are incorrect, SSSD falls back to online
+                            authentication.
                         </para>
                         <para>
                             This option's value is inherited by all trusted
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index a70b342b1..89bdb78a1 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -808,8 +808,9 @@ static void pam_reply(struct pam_auth_req *preq)
         pam_verbosity = DEFAULT_PAM_VERBOSITY;
     }

-    DEBUG(SSSDBG_FUNC_DATA,
-          "pam_reply called with result [%d]: %s.\n",
+    DEBUG(SSSDBG_TRACE_ALL,
+          "pam_reply initially called with result [%d]: %s. "
+          "this result might be changed during processing\n",
           pd->pam_status, pam_strerror(NULL, pd->pam_status));

     if (pd->cmd == SSS_PAM_AUTHENTICATE
@@ -891,6 +892,7 @@ static void pam_reply(struct pam_auth_req *preq)
             break;
 /* TODO: we need the pam session cookie here to make sure that cached
  * authentication was successful */
+        case SSS_PAM_PREAUTH:
         case SSS_PAM_SETCRED:
         case SSS_PAM_ACCT_MGMT:
         case SSS_PAM_OPEN_SESSION:
@@ -1072,6 +1074,8 @@ static void pam_reply(struct pam_auth_req *preq)
     }

 done:
+    DEBUG(SSSDBG_FUNC_DATA, "Returning [%d]: %s to the client\n",
+          pd->pam_status, pam_strerror(NULL, pd->pam_status));
     sss_cmd_done(cctx, preq);
 }

@@ -2022,8 +2026,7 @@ static bool pam_can_user_cache_auth(struct sss_domain_info
*domain,
         return false;
     }

-    if (domain->cache_credentials == false
-            || domain->cached_auth_timeout <= 0) {
+    if (!domain->cache_credentials || domain->cached_auth_timeout <= 0) {
         return false;
     }


```
"""

See the full comment at https://github.com/SSSD/sssd/pull/804#issuecomment-493042534

[sssd PR#804][-Changes requested] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

Label: -Changes requested

[sssd PR#804][comment] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

jhrozek commented:
"""
On 5/3/19 1:24 PM, sumit-bose wrote:
>  Hi Jakub,
>
>  the patch is working as expected, I only added a minor comment to the 
>  code.
>
>  While testing I came across two issues where I wonder if you would 
>  like to fix them with this patch as well.
>
>  First, if a wrong password is given cached authentication currently 
>  does not fail but falls back to online authentication. I think this 
>  behavior make sense, but might be unexpected. A sentence in the man 
>  page describing this behavior would be useful imo.
>  
OK, added.


>  Second, there is no clear debug message from the PAM responder that 
>  cached authentication is used. Instead there is
>
>  |(Fri May 3 13:05:11 2019) [sssd[pam]] [pam_reply] (0x0200): pam_reply 
>  called with result [4]: Systemfehler. | 

|I thought the message might be sometimes useful, so I only changed its 
level to be lowest debug level and reworded the message to make it clear 
that this is an intermediate result only.|

||

>  and later on there are some message from |sysdb_cache_auth()|. Maybe
>
>  in |pam_reply()| the |called with result| message can be skipped for 
>  cached auth and a more suitable message can be shown?
>
>  Both are not related to the issue at hand so feel free to open a new 
>  ticket or ignore them.
>  
I also added PREAUTH to the list of PAM commands that just return 
PAM_SUCCESS during cached authentication. I hope that's correct.

"""

See the full comment at https://github.com/SSSD/sssd/pull/804#issuecomment-493043523

[sssd PR#804][comment] PAM: Also cache SSS_PAM_PREAUTH

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

sumit-bose commented:
"""
Hi,

the patch works for me and the log messages are looking less irritating now.

About:

>  I also added PREAUTH to the list of PAM commands that just return
> PAM_SUCCESS during cached authentication. I hope that's correct.
>  
That's fine, pam_sss ignores all error during pre-auth and falls back default
behavior, but having PAM_SUCCESS here make things more clear.

ACK

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/804#issuecomment-493859934

[sssd PR#804][+Accepted] PAM: Also cache SSS_PAM_PREAUTH

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

Label: +Accepted

[sssd PR#804][comment] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

jhrozek commented:
"""
* master: c911562
* sssd-1-16: 0a637ff
"""

See the full comment at https://github.com/SSSD/sssd/pull/804#issuecomment-493910515

[sssd PR#804][closed] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/804
Author: jhrozek
 Title: #804: PAM: Also cache SSS_PAM_PREAUTH
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/804/head:pr804
git checkout pr804

[sssd PR#804][+Pushed] PAM: Also cache SSS_PAM_PREAUTH

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/804
Title: #804: PAM: Also cache SSS_PAM_PREAUTH

Label: +Pushed