[SSSD] [PATCHES] PAM: authenticate agains cache

Lukas Slebodnik lslebodn at redhat.com
Fri Jun 26 09:00:06 UTC 2015


On (18/06/15 10:02), Pavel Reichl wrote:
>On 06/05/2015 07:45 PM, Lukas Slebodnik wrote:
>>>+errno_t
>>>+sysdb_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>>>+                                           const char *username,
>>>+                                           uint64_t value);
>>>+errno_t
>>>+sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>>>+                                            const char *username);
>>>+
>>>+errno_t
>>>+sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>>>+                                           const char *name,
>>>+                                           uint64_t *_value);
>>>+
>>Should we also create following sydb functions?
>>
>>sysdb_set_last_online_auth
>>sysdb_null_last_online_auth
>>sysdb_get_last_online_auth
>>sysdb_set_last_failed_login
>>sysdb_null_last_failed_login
>>sysdb_get_last_failed_login
>>sysdb_set_data_expire_timestamp
>>sysdb_null_data_expire_timestamp
>>sysdb_get_data_expire_timestamp
>>sysdb_set_initgroups_expire_timestamp
>>sysdb_null_initgroups_expire_timestamp
>>sysdb_get_initgroups_expire_timestamp
>>...
>>
>>LS
>Please do whatever you find to be the best for the code.
>
>If you have comments to the patch please be more specific and state what and
>why you don't like so I don't have to guess and waste neither my time nor
>yours. Thanks!

So maybe I just had higher expectation but I do not have a problem to
elaborate. Your added new functions evoke object oriented paradigm(OOP).
It's called encapsulation in OOP. You hide internal data types and write
getters/setters (properties in some languages) to hide internal data
representation. The main benefit is to have free hands in future and
change internal representations without changing public API.

But encapsulation is just one part of OOP it should be combined with
composition, inheritance and polymorphism to have real OOP result.

However sssd is written in C and in procedural (imperative) paradigm.
Even though I prefer other paradigm as well. I will not try to
create an abortive hybrid.

It's fine to use "decoupling" and have small functions. I give you
+1 for this. There is also dark side of this change. Functions in
imperative paradigm should be reusable especially public ones.
So there is small difference between OOP and procedural paradigm
(encapsulation vs. reusability).

I consider sysdb as internal library even though it is a part of
libssss_util.so. We have many reusable functions there which are well tested.
If your aim was to add test to the patches then it's fine.
But you should have focused on different part. You should have
write tests for fast offline authentication. There is already cmocka test
for pam_srv. Moreover you have very simplified task. The most of your changes
was on pam responder side which can be easily tested (without mocking
ldap searches).

As I already wrote I like small functions. but public ones should be reusable.
In your case you could use these hepler functions as static directly
in pam responder + write tests for new functionality.
So you would "kill two birds with one stone". The decoupling would be used
and these static function would be indirectly tested as a part of fast
authentication tests.


On (25/06/15 17:05), Pavel Reichl wrote:
>Please see updated patchset. I created a ticket to not forget to add unit
>tests (https://fedorahosted.org/sssd/ticket/2697).

>From fca6a4275a4cfb298641949cd5b8b799d5eddd22 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          | 10 ++++++
> src/db/sysdb_ops.c      | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/db/sysdb_private.h  |  5 +++
> src/tests/sysdb-tests.c | 76 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 186 insertions(+)
>
>diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>index 4dc382f6fb3f1b6bd084312463edf1b6189ebba9..2319a0cae1bdff65453f69c4a5667fb178556fce 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"
>@@ -904,6 +905,15 @@ int sysdb_cache_password_ex(struct sss_domain_info *domain,
>                             enum sss_authtok_type authtok_type,
>                             size_t second_factor_size);
> 
>+errno_t
>+sysdb_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>+                                            const char *username);
>+
>+errno_t
>+sysdb_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>+                                           const char *name,
>+                                           uint64_t *_value);
>+
> errno_t check_failed_login_attempts(struct confdb_ctx *cdb,
>                                     struct ldb_message *ldb_msg,
>                                     uint32_t *failed_login_attempts,

As you can see you defined setters and getters and in the same time
you defined public constant SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN.
It's sort of equivalent to creating getter and setter for public variable
in object.

I'm not aggainst getters and setters but there should be a RFC + design page
and broad discussion for removing all "SYSDB_*" constants and replace them with
functions. But this version of patch is unacceptable.

LS



More information about the sssd-devel mailing list