[SSSD] [PATCHES] PAM: authenticate agains cache

Jakub Hrozek jhrozek at redhat.com
Fri Jun 26 11:49:57 UTC 2015


On Fri, Jun 26, 2015 at 11:00:06AM +0200, Lukas Slebodnik wrote:
> 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.

Well, then let's move these functions to pam_helpers.c along with the
tests for now. 

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

Yes, long term it would be better to have a more layered sysdb API,
currently it's just a bunch of functions. Something like:
    sysdb_attrs - create attr, set, del, ...
    sysdb_users - create user, delete user, ..
    sysdb_user_attrs - getsetters for attributes

...and responders/providers would only include the sysdb_user_attrs
header.

But I don't think we have the time for this now. In the end, we have
to compromise and do the best to keep the code maintainable with the
limited time.

I think the interfaces between subystems (DP interface, resolver
interface, ...) are much more important to keep lean and clean. There
you can really influence maintainability of the code than with an
attribute setter.

>But this version of patch is unacceptable.

I got a great book from Andreas recently:
    http://shop.oreilly.com/product/0636920018025.do
I already finished it, so I'll lend it to you next. 



More information about the sssd-devel mailing list