[SSSD] [PATCHES] PAM: authenticate agains cache

Lukas Slebodnik lslebodn at redhat.com
Tue Jun 30 21:59:12 UTC 2015


On (30/06/15 12:10), Jakub Hrozek wrote:
>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.
>
So it means that getters and setters are suitable for sysdb API.

This is another proof that functions *last_online_auth_with*
are just wrappers (helper functions) and should not be part of
public API. They cannot be used for proper encapsulation without
exponential count of new function.
   //sysdb_set_username_gid_uid_..._()

>(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.
>
I have to miss something.

There were added 115 lines to pamsrv_cmd.c. They can be added to
new module as public functions (as You propoesd) and can be called
from pamsrv_cmd.c.

I do not understand what would cause invasive change.
It would be a problem if there were many changes in old code
(many removed and adde lines)

>> 
>>  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..
>
We do not have such convency and nobody check it in review
which public function is used from which module.

>> 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..
>

Michal has patches for simplification of sysdb API with using
fqnames. But if you think we needn't care about public API
and we can complicate it with other patches feel free to push
any patches you want.

LS



More information about the sssd-devel mailing list