Re: [SSSD] [PATCH] Add integration tests
by Nikolai Kondrashov
On 04/28/2015 05:49 PM, Michal Židek wrote:
> On 04/27/2015 05:56 PM, Nikolai Kondrashov wrote:
>>> On 04/24/2015 01:44 PM, Nikolai Kondrashov wrote:
>> There is no actual libc interface to check for user existence, so I think
>> having an assertion for that will be misleading. If you don't care what
>> attributes a user has, and only want to check that it exists, just pass an
>> empty pattern:
>>
>> assert_passwd_by_name("user1", {})
>>
>> BTW, I wouldn't like to change "passwd" to "user". I think it will be more
>> straightforward to simply use NSS database names for all entry types.
>
> assert_passwd_by_name is a good name and it is probably better than
> the _has_attributes that I suggested. My strongest request was that
> all assert functions start with the prefix assert_ .
Alright.
>>> Do not worry too much if the identifiers get longer. Sometimes
>>> it is better to sacrifice the keyboard lifetime for the sake of
>>> better readability :) .
>>
>> More verbose interface might be easier to read the first few times, but
>> becomes a burden both to read (more text to scan) and to write (more
>> text to
>> type and make mistakes in), after you learn it.
>
> I tend to disagree here. It is IMO much greater burden if I need
> to check internals of functions just to get an idea
> of what the function is doing.
I don't like that either. I guess my function names and descriptions were just
not clear enough, or maybe we don't match on the acceptable clarity.
>>>> I attempted to describe the logic behind the generic matching function
>>>> "diff", but please ask if anything is not clear. There is one particular
>>>> flaw to it, though: there is no way to match a list partially, e.g. to
>>>> match for presence of just a few group members, instead of all. I
>>>> think we
>>>> can employ tuples for these. I.e. say that a tuple pattern matches a
>>>> list
>>>> if all tuple items match at least one list item, but not necessarily the
>>>> other way around.
>>>
>>> It will be better to remove this limitation, but not with the
>>> tuples.
>>> If think it is not good to have change in behaviour based on whether
>>> the argument is tuple or list. A better approach would be to have
>>> additional boolean argument in diff(), named for example match_all with
>>> default set to False (or True). If the parameter is True, then
>>> all returned entries must match some pattern, if not, it is enough
>>> if each given pattern matches some entries in the returned list of
>>> entries. Would that work for you?
>>
>> I would very much like to avoid this solution. It would be all-or-nothing
>> (i.e. no mixing different conditions in one call, no matter how high-level)
>> and would increase the code size by requiring passing that variable through
>> the stack of many functions.
>
> Well, the parameter match_all would have default value (I think False
> would be a good default) so the user will not have to pass it if
> he is OK with the default behaviour. And the stack of the functions
> is not very high (2) so I do not see this as a big problem.
The stacks are not deep but there are quite a few of them. These functions
would need to have that argument (using current, not projected names), along
with some limitations:
diff - matching all or some entries
passwd_list_assert - matching all or some entries
passwd_diff - matching all or some entries
passwd_assert - matching all or some entries
group_assert_by_name - matching all or some group members
group_assert_by_uid - matching all or some group members
group_list_assert - matching all or some entries and members,
but cannot match all entries with some members, or
some entries with all members
group_name_dict_assert - matching all or some group members
group_gid_dict_assert - matching all or some group members
passwd_diff - matching all or some entries and members,
but cannot match all entries with some members, or
some entries with all members
passwd_assert - matching all or some entries and members,
but cannot match all entries with some members, or
some entries with all members
We can, of course, remove or change some of these functions, but the general
problem will stay and it will reduce flexibility, forcing us to add more
functions eventually.
>> We already have different matching logic for dictionaries and lists. Adding
>> tuples doesn't make it much more complex, and keeps the patterns
>> succinct and
>> expressive.
>
> The difference between lists and dictionaries is visually big
> enough (if the dictionary is not empty) to make sure the user will
> not mistake the two and get the expected behaviour. In case
> of tuples and lists, the visual difference is not so big.
>
> I think it will be better if we get rid of this behaviour where
> different parameter types do these subtle diffrencies in
> behavior.
What if we think about these patterns not just as fixed format arguments, but
rather similar to, say, regexes? Sure, regexes are cryptic, and subtle
differences in them introduce significant behavior changes, like omitting or
inserting an extra backslash, using '*', instead of '+', etc. Yet, they're
also powerful.
I understand your motivation to keep the interface accessible and safe, and
I'm willing to implement it your way, if this message doesn't change your
mind.
May I suggest another solution, which might make it more palatable? What if we
replace lists/tuples with functions or class constructors, which would
annotate what's happening?
So, instead of writing this rather unusual, but illustrative example:
pattern = (
{name='group1', gid=1001, mem=['userA', 'userB', 'userC']},
{name='group2', gid=1002, mem=('userX', 'userY')},
)
ent.group_assert(pattern)
You would write something like this:
pattern = ent.some(
{name='group1', gid=1001, mem=ent.all('userA', 'userB', 'userC')},
{name='group2', gid=1002, mem=ent.some('userX', 'userY')}
)
ent.group_assert(pattern)
Theoretically, we can also make a distinction of matching some/all of
dictionary members, but I don't see that as useful ATM.
> We can add "type" parameter to the diff function and init
> the messages based on this parameter. This way we will not
> have to copy-paste the code and we could use more specific
> messages.
Hmm, I guess that might go some way. I'll try it, thanks.
> Also do not forget to rebase the patches on top of the
> current master. There were some changes in the build
> files recently.
Sure, of course.
Nick
8 years, 11 months
[PATCH] AD GPO: Change default to "enforcing"
by Stephen Gallagher
When a user enrolls a system against Active Directory, the expectation
is that the client will honor the centrally-managed settings. In the
past, we avoided changing the default (and left it in permissive mode,
to warn admins that the security policy wasn't being honored) in order
to avoid breaking existing Active Directory enrollments.
However, sufficient time has likely passed for users to become
accustomed to using GPOs to manage access-control for their systems.
This patch changes the default to enforcing and adds a configure flag
for distributions to use if they wish to provide a different default
value.
8 years, 11 months
design discussion: Authenticate against cache in SSSD
by Pavel Reichl
Hello,
I have prepared wiki page with proposal how to implement the feature from the subject.
https://fedorahosted.org/sssd/wiki/DesignDocs/CachedAuthentication
For your convenience I paste the content here.
Thanks!
= Authenticate against cache in SSSD =
Related ticket(s):
* https://fedorahosted.org/sssd/ticket/1807
=== Problem statement ===
SSSD should allow cache authentication instead of authenticating directly against network server every time. Authenticating against the network many times can cause excessive application latency.
=== Use cases ===
In environments with tens of thousands of users log in process may become inappropriately long, when servers are running under high workload (e.g. during classes, when may users log in simultaneously).
=== Overview of the solution ===
Add new domain option `cached_authentication_timeout` describing how long can be cached credentials used for cached authentication before on-line authentication must be performed. Update PAM responder functionality for forwarding requests to domains by checking if request can be served from cache and if so execute same code branch as for off-line authentication instead of contacting the domain.
=== Implementation details ===
* extend ''struct pam_auth_req''
* add new field `use_cached_auth` (default value is false)
* extend ''pam_dom_forwarder()''
* obtain value of domain option `cached_authentication_timeout`
* do not forward request to domain if
* domain uses cached credentials and
* `cached_authentication_timeout` is greater than 0 and
* last online log in of user who is being authenticated is not stale (< ''now()'' - `cached_authentication_timeout`) and
* PAM request can be handled from cache (PAM command is SSS_PAM_AUTHENTICATE or SSS_PAM_ACCT_MGMT)
* then set `use_cached_auth` to true
* call ''pam_reply()''
* extend ''pam_reply()''
* extend condition for entering into block processing case when pam_status is PAM_AUTHINFO_UNAVAIL even for `use_cached_auth` being true
* while in this block and if PAM command is SSS_PAM_AUTHENTICATE then set `use_cached_auth` to false to avoid cyclic recursion call of ''pam_reply()'' which is subsequently called from ''pam_handle_cached_login()''.
* introduce function ''sysdb_get_user_lastlogin()''
* returning time of last online performed log in for given user
=== Configuration changes ===
A new domain option `cached_authentication_timeout` will be added. The value of this option is time period for which cached authentication can be used. After this period is exceeded on-line authentication must be performed. The default value would be 0, which implies that this feature is by default disabled.
=== How To Test ===
1. set `cached_authentication_timeout` in sssd.conf to some non-null value (e.g. 120)
1. erase SSSD caches and restart SSSD
1. log in as user from domain which stores credentials and then log out and log in again. The second log in should use cached credentials. Output should by similar to this, especially note the line starting with: '''Authenticated with cached credentials'''
{{{
devel@dev $ su john
Password:
john@dev $ exit
devel@dev $ su john
Password:
Authenticated with cached credentials, your cached password will expire at: Wed 22 Apr 2015 08:47:29 AM EDT.
john@dev $
}}}
1. for the `cached_authentication_timeout` seconds since the 1st log in all subsequent log in attempts (for the same user) should be served from cache and domain should not be contacted, this can be verified by changing password at server.
1. after passing more than `cached_authentication_timeout` seconds since the 1st log in an on-line log in should be performed and new password must be used.
=== Authors ===
* Pavel Reichl <preichl(a)redhat.com>
8 years, 11 months
[PRELIMINARY][PATCH] ifp users and groups
by Pavel Březina
Hi,
I'm sending these incomplete patches for review just to get some feed back.
What is missing:
- support for user's extraAttributes (although get invoker is there)
- ListByNameFilter for both groups and users
8 years, 11 months
[PATCH] LDAP: warn about lockout option being deprecated
by Pavel Reichl
Hello,
attached patch deprecates lockout option in 1-12 branch.
This was discussed in thread: SDAP: Lock out ssh keys when account naturally expires
This patch implements point number 2.
>> I would prefer if we didn't add a new option as well, but since we released
>> a version that only supported the lockout and not any other semantics,
>> I don't think we can get away with just changing the functionality. A
>> minor version can break functionality. But a major version can
>>
>> So I propose the following:
>> 1) Add a new value for ldap_access_order called "ppolicy" that would
>> evaluate the pwdAccountLockedTime fully, including the new
>> functionality in this patchset
>> 2) In 1.12, deprecate the "lockout" option and log a warning that it
>> will be removed in future relase and users should migrate to "ppolicy"
>> option
>> 3) In master (1.13), remove the "lockout" ldap_access_order value
I'll send patch for point number 3 in separate thread.
Thanks!
8 years, 11 months
[PATCHES] Add special PAM prompting for 2FA authentication
by Sumit Bose
Hi,
this series of patches tries to add special prompting in the PAM dialog
for 2-Factor-Authentication (2FA) and use it where suitable. As
discussed on
https://fedorahosted.org/sssd/wiki/DesignDocs/PAMConversationForOTP if
long-term password and one-time component are entered in a single prompt
SSSD is in general not able to split them and use the long-term password
of offline authentication or some user keyrings. With this patch-set an
IPA user with configured OTP token will be prompted to enter the two
factors separately (if the login application supports it) and the first
factor (long-term password) will be save into the cache for offline
authentication if it is long enough.
Two new internals are needed for this. A special authentication token
type for 2FA and a PAM pre-auth request to check if the given user user
a standard password or 2FA/OTP.
The first two patches enhance the authtok module to handle 2FA.
Patches 4-6 add the pre-auth request to PAM client, responder and IPA
backend. Patches 7-12 add the prompting, the minimal length check and
the caching of the first factor. Finally tests for the PAM responder are
added which should test existing and new behaviour.
bye,
Sumit
8 years, 11 months