Store password to cache after passing all pam check

[jakechenTW]

The credential will be cached even if it didn't pass all pam checks, the user might be able to login if he try it twice

[adelton]

Thank you for raising this issue. I'm not completely convinced about the solution though because _PAM_STEP_AUTH might not have been invoked at all (only _PAM_STEP_ACCOUNT) and yet we'd be attempting to cache the password.

Would you be able to amend the test to actually show the problem, logging in with the second attempt after the first one failed in the _PAM_STEP_ACCOUNT? We will likely need the test anyway for any code change, and it would be a good step to show the problem.

Incidently this might be losely related to https://github.com/adelton/mod_authnz_pam/pull/5 and the way auth + account PAM steps interact with Apache's basic authentication.

[jakechenTW]

I have to say that I'm not familiar with the entire pam login process, just thought that we should cache the password only before returning AUTH_GRANTED.

I'll try to elaborate the problem, thanks.

[jakechenTW]

I've made the following changes:

1. Make sure we passed pam_authenticate before storing password to cache
2. Add test to show the problem that logging in with the second attempt after the first one failed in the _PAM_STEP_ACCOUNT

The failing check shouldn't be a problem, it failed at the build phase. Let me know if there's any problem, thanks!

[adelton]

I've rerun the test.

[adelton]

Thanks for the work and sorry it took me so long to start focusing on it. I'd start with commit a19b73d1eb7e525932954e3758d0a16bf6576252 -- that one I'd push in first since it's general hardening of the tests but I'd change it with

--- a/tests/run.sh
+++ b/tests/run.sh
@@ -22,10 +22,11 @@ curl -u alice:Tajnost -s http://localhost/authz | tee /dev/stderr | grep 'User a
 echo "Testing AuthBasicProvider PAM"
 curl -s -D /dev/stdout -o /dev/null http://localhost/authn | tee /dev/stderr | grep 401
 curl -u bob:Secret -s -D /dev/stdout -o /dev/null http://localhost/authn | tee /dev/stderr | grep 401
-touch /etc/pam-account/bob
-curl -u bob:Secret -s -D /dev/stdout -o /dev/null http://localhost/authn | tee /dev/stderr | grep 401
 touch /etc/pam-auth/bob
+curl -u bob:Secret -s -D /dev/stdout -o /dev/null http://localhost/authn | tee /dev/stderr | grep 401
 echo Secret > /etc/pam-auth/bob
+curl -u bob:Secret -s -D /dev/stdout -o /dev/null http://localhost/authn | tee /dev/stderr | grep 401
+touch /etc/pam-account/bob
 curl -u bob:Secret -s http://localhost/authn | tee /dev/stderr | grep 'User bob'
 echo Secret2 > /etc/pam-auth/bob
 curl -u bob:Secret -s -D /dev/stdout -o /dev/null http://localhost/authn | tee /dev/stderr | grep 401

to really show that the auth passing is not enough and that account is executed as well. This is primarily related to https://github.com/adelton/mod_authnz_pam/pull/5 which I plan to address with an option to disable the account step, so I'd like to have the behaviour in the Basic Auth situation covered.

[adelton]

As for f24916b4cc97359173ca6b52735e09f9bb252947 and 5c1e18e9cf2a570121dee51a18afc18b28df74d6, would the most consise approach be something like

--- a/mod_authnz_pam.c
+++ b/mod_authnz_pam.c
@@ -197,11 +197,6 @@ static authn_status pam_authenticate_with_login_password(request_rec * r, const
            param = login;
            stage = "PAM authentication failed for user";
            ret = pam_authenticate(pamh, PAM_SILENT | PAM_DISALLOW_NULL_AUTHTOK);
-#if AP_MODULE_MAGIC_AT_LEAST(20100625,0)
-           if (ret == PAM_SUCCESS) {
-               store_password_to_cache(r, login, password);
-           }
-#endif
        }
        if ((ret == PAM_SUCCESS) && (steps & _PAM_STEP_ACCOUNT)) {
            param = login;
@@ -232,6 +227,11 @@ static authn_status pam_authenticate_with_login_password(request_rec * r, const
    r->user = apr_pstrdup(r->pool, login);
    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, SHOW_MODULE "PAM authentication passed for user %s", login);
    pam_end(pamh, ret);
+#if AP_MODULE_MAGIC_AT_LEAST(20100625,0)
+   if (steps & _PAM_STEP_AUTH) {
+       store_password_to_cache(r, login, password);
+   }
+#endif
    return AUTH_GRANTED;
 }

? We are right before the return AUTH_GRANTED so we know that all the previous steps have passed, and the only question is, was one of the steps authentication.

[adelton]

I've edited your commits based on my comments above so the approach could be like one shown in https://github.com/adelton/mod_authnz_pam/pull/13.

[jakechenTW]

I'm fine with the modifications, you can continue this in #13

[adelton]

Thanks for the review, I've merged https://github.com/adelton/mod_authnz_pam/pull/13 in to master.