[SSSD] [PATCHES] PAM: authenticate agains cache

Jakub Hrozek jhrozek at redhat.com
Tue Jun 30 10:10:27 UTC 2015


On Tue, Jun 30, 2015 at 09:41:19AM +0200, Lukas Slebodnik wrote:
> On (29/06/15 17:23), Jakub Hrozek wrote:
> >On Mon, Jun 29, 2015 at 11:56:01AM +0200, Lukas Slebodnik wrote:
> >> On (26/06/15 17:31), Pavel Reichl wrote:
> >> >
> >> So it seems you still do not understand what's wrong from desing point of view.
> >> If you do not understand something please ask it before. We would save your
> >> time with preparing patches and time of other developers reviewing unnecessary
> >> round of patches.
> >> 
> >> I would like to repeat what was not right from design point of view in previous
> >> version. You used totally different approach for functions. They weren't wrong
> >> they just did not fit among other sysdb functions.
> >> 
> >> Let's imagine pure object oriented library and someone decides to create new
> >> class with just static functions. He wrote this class because he wanted to
> >> emulate procedural paradigm. It wouldn't be a problem if other classes
> >> were written in such way. But mixing them together make a stinky code.
> >> 
> >> It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
> >> can clap as well but it doesn't fit among other people. It would be fine
> >> if auditorious contained just chimpanzees.
> >> 
> >> And now why is current version even worse then previous one from design point
> >> of view. You renamed public functions and moved them to tottaly unrelated part
> >> of code. It is an equivalent of get dressed chimpanzee to different clothes and
> >> let it sit in city council. It still does not fit there.
> >> 
> >> 
> >> 
> >> >From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17 00:00:00 2001
> >> >From: Pavel Reichl <preichl at redhat.com>
> >> >Date: Wed, 24 Jun 2015 09:07:55 -0400
> >> >Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
> >> >
> >> >Introduce new user attribute lastOnlineAuthWithCurrentToken.
> >> >
> >> >Resolves:
> >> >https://fedorahosted.org/sssd/ticket/1807
> >> >---
> >> > src/db/sysdb.h                  |  1 +
> >> > src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
> >> > src/responder/pam/pam_helpers.h | 15 +++++++
> >> > src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
> >> > 4 files changed, 150 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> >> >index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..f309c9e0c542f8b9c02e48f74a9cefdb1e0a1752 100644
> >> >--- a/src/db/sysdb.h
> >> >+++ b/src/db/sysdb.h
> >> >@@ -87,6 +87,7 @@
> >> > #define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
> >> > #define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
> >> > #define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
> >> >+#define SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN "lastOnlineAuthWithCurrentToken"
> >> > 
> >> > #define SYSDB_LAST_UPDATE "lastUpdate"
> >> > #define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
> >> >diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
> >> >index 1e1d914dfe3cd358972cc1ba6deb683a49b68235..b23713c2b51185ca11369b17f1ca30e295d1cb21 100644
> >> >--- a/src/responder/pam/pam_helpers.h
> >> >+++ b/src/responder/pam/pam_helpers.h
> >> >@@ -23,6 +23,8 @@
> >> > #ifndef PAM_HELPERS_H_
> >> > #define PAM_HELPERS_H_
> >> > 
> >> >+#include "util/util.h"
> >> >+
> >> > errno_t pam_initgr_cache_set(struct tevent_context *ev,
> >> >                              hash_table_t *id_table,
> >> >                              char *name,
> >> >@@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
> >> > errno_t pam_initgr_check_timeout(hash_table_t *id_table,
> >> >                               char *name);
> >> > 
> >> >+errno_t
> >> >+pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >> >+                                          const char *username);
> >> >+
> >> >+errno_t
> >> >+pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >> >+                                         const char *name,
> >> >+                                         uint64_t *_value);
> >> >+errno_t
> >> >+pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
> >> >+                                         const char *username,
> >> >+                                         uint64_t value);
> >> >+
> >> > #endif /* PAM_HELPERS_H_ */
> >> 
> >> Here are three proposal which you can use.
> >> a) create reusable public functions in sysdb and not a single purpose
> >>    functions. Reusable public functions would fit to current sysdb API.
> >> b) create internal helper functions in pam_responder. They should be internal
> >>    (static) and they purpose should be to improve readalibity of code; avoid
> >>    long functions. If you want them in separate module then create a new file
> >>    for fast pam authentication with public pam functions.
> >
> >I don't think it's such a big deal at this point because the getsetters
> >are in the PAM tree and don't pullute the sysdb API. It's just a helper
> >now.
> >
> Yes, but they are still sysdb public functions but in wrong place
> and moreover there is still public constant
> SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN which can be used with other sysdb
> functions. So one developer can use old way of manipulating with
> this attribute and other can use getter/setter. That's the problem.
> It isn't encapsulation. It's inconsistent and thus unacceptable.

I more or less agree with the reasoning (except I don't think it's such
a big deal and I'd let it go myself). What Sumit brought up on IRC the
other day and what's true is that unless you define the attribute as a
constant, then there's no way to query as part of an array.

(Yes, we probably won't need it for this attribute in particular, but
for others it's true)

> 
> >Yes, I guess even more structured approach would be to have a PAM responder
> >module that handles the cached authentication, but then the question
> >is whether this refactoring wouldn't be too invasive.
> >
> Adding new functionality needn't be invasive. In this case it isn't.
> Here is a statistic of last two patches. (excluding public sysdb functions)

No, I meant something else. I meant that the best way might be to create a
module where all the cached auth logic would be contained instead of
pamsrv_cmd. But that would be refactoring and that would be invasive.

> 
>  src/confdb/confdb.c                  |  62 +++++++++++++++++++++++++++++++
>  src/confdb/confdb.h                  |   2 ++
>  src/config/SSSDConfig/__init__.py.in |   1 +
>  src/config/SSSDConfigTest.py         |   6 ++--
>  src/config/etc/sssd.api.conf         |   1 +
>  src/db/sysdb_ops.c                   |   1 -
>  src/man/sssd.conf.5.xml              |  24 +++++++++++++
>  src/responder/pam/pamsrv.h           |   3 ++
>  src/responder/pam/pamsrv_cmd.c       | 115 +++++++++++++++++++++++++++++++
>  9 files changed, 205 insertions(+), 10 deletions(-)
> 
> 
> >On the other hand, the tests for the utilities are not too critical, so
> >if you feel strongly about these three helpers, then making them static
> >would be less invasive than creating the module, unless there are tests.
> >
> 3 public functions would make sense if they was part of sysdb public API
> (point c). But all these functions are single purpose and will never be
> used on other places (excluding unit test). They are used JUST on one place.
> So they are definitely simple wrappers (helper functions) and the only purpose
> is to improve readability. No reason to make them public.

Well, they are "public" only in the scope of the PAM responder now..

> BTW functions pam_set_last_online_auth_with_curr_token
> pam_null_last_online_auth_with_curr_token can be merged. We needn't
> have such granularity for helper functions.

Again, I wouldn't mind too much here, but if you feel so strongly..

> 
> >I'm totally fine with refactoring code that is well covered with tests,
> >but I don't think we're there yet with PAM responder.
> >
> Pavel didn't refactor the code. He just added 115 lines to pamsrv_cmd.c.
> He added a new functionality and moreover he needn't change anything if
> he want to test fast authentication. There is already pam_srv unit test.
> It just need to be extended.

Again, I meant the new pamsrv_cached_auth.c module (or similar).



More information about the sssd-devel mailing list