On Fri, Feb 05, 2016 at 03:16:18PM +0100, Lukas Slebodnik wrote:
> On (05/02/16 15:10), Jakub Hrozek wrote:
>> On Fri, Feb 05, 2016 at 02:54:53PM +0100, Lukas Slebodnik wrote:
>>> On (05/02/16 13:55), Pavel Reichl wrote:
>>>>
>>>> On 02/05/2016 11:01 AM, Jakub Hrozek wrote:
>>>>> On Tue, Feb 02, 2016 at 08:48:43PM +0100, Pavel Reichl wrote:
>>>> ...
>>>>> I would prefer to split this patch into two, one that patches the
LDAP
>>>>> code to return ERR_ACCOUNT_LOCKED and one that passes on and
displays
>>>>> the message.
>>>> Done.
>>>>>> From 511ef599902827d76193a1e634ace193df15dead Mon Sep 17
00:00:00 2001
>>>>>> From: Pavel Reichl <preichl(a)redhat.com>
>>>>>> Date: Tue, 2 Feb 2016 14:35:15 -0500
>>>>>> Subject: [PATCH] PAM: Notify user of denial due to AD account
lockout
>>>>>>
>>>>>> Resolves:
>>>>>>
https://fedorahosted.org/sssd/ticket/2839
>>>>>> ---
>>>>>> index
2d9b1184f5d30b9df7f1d3e4b980a7e0107c6830..763c5ed050bd482d334ad617349938dfc89f79da 100644
>>>>>> --- a/src/providers/ldap/sdap_async_connection.c
>>>>>> +++ b/src/providers/ldap/sdap_async_connection.c
>>>>>> @@ -754,6 +754,9 @@ static void simple_bind_done(struct sdap_op
*op,
>>>>>>
>>>>>> if (result == LDAP_SUCCESS) {
>>>>>> ret = EOK;
>>>>>> + } else if (result == LDAP_INVALID_CREDENTIALS
>>>>>> + && strstr(errmsg, "data
775,") != NULL) {
>>>>> ~~~~~~~~~~~~~~
>>>>> I don't think this is safe, strstr() doesn't handle NULL
input well.
>>>>> Please add a check for "&& errmgs != NULL" before
calling strstr.
>>>> Sure.
>>>>
>>>>> Otherwise the patch looks good, we just need to also ask some Native
>>>>> speaker for manpage comments..
>>>> I pinged Dan about that.
>>>>
>>>>
>>>> Please see updated patch set, thanks!
>>> >From 4adb25b045be46dc0a178748e2b0aeb5ea09ed1d Mon Sep 17 00:00:00 2001
>>>> From: Pavel Reichl <preichl(a)redhat.com>
>>>> Date: Fri, 5 Feb 2016 07:27:38 -0500
>>>> Subject: [PATCH 1/2] SDAP: Add return code ERR_ACCOUNT_LOCKED
>>>>
>>>> Add code to distinquish state when account is locked in Active
>>>> Directory server.
>>>>
>>>> Resolves:
>>>>
https://fedorahosted.org/sssd/ticket/2839
>>>> ---
>>>> src/providers/data_provider.h | 2 ++
>>>> src/providers/ldap/ldap_auth.c | 4 ++++
>>>> src/providers/ldap/sdap_async_connection.c | 3 +++
>>>> 3 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/src/providers/data_provider.h
b/src/providers/data_provider.h
>>>> index
39051b90c3aad96f62dcbb86a20bcfd8c954879b..7332b677d19f70f4736e4d0b68d55cdd3c67a4af 100644
>>>> --- a/src/providers/data_provider.h
>>>> +++ b/src/providers/data_provider.h
>>>> @@ -182,6 +182,8 @@ struct pam_data {
>>>> bool offline_auth;
>>>> bool last_auth_saved;
>>>> int priv;
>>>> + int account_locked;
>>>> +
>>>> #ifdef USE_KEYRING
>>>> key_serial_t key_serial;
>>>> #endif
>>>> diff --git a/src/providers/ldap/ldap_auth.c
b/src/providers/ldap/ldap_auth.c
>>>> index
c94ba15bb17aa1641eb36781cc59ce158d48ca66..8d6a37b2ceb3347cb8092858889d07e5615e5c77 100644
>>>> --- a/src/providers/ldap/ldap_auth.c
>>>> +++ b/src/providers/ldap/ldap_auth.c
>>>> @@ -1302,6 +1302,10 @@ static void sdap_pam_auth_done(struct tevent_req
*req)
>>>> case ERR_PASSWORD_EXPIRED:
>>>> state->pd->pam_status = PAM_NEW_AUTHTOK_REQD;
>>>> break;
>>>> + case ERR_ACCOUNT_LOCKED:
>>>> + state->pd->account_locked = true;
>>>> + state->pd->pam_status = PAM_PERM_DENIED;
>>>> + break;
>>>> default:
>>>> state->pd->pam_status = PAM_SYSTEM_ERR;
>>>> dp_err = DP_ERR_FATAL;
>>>> diff --git a/src/providers/ldap/sdap_async_connection.c
b/src/providers/ldap/sdap_async_connection.c
>>>> index
2d9b1184f5d30b9df7f1d3e4b980a7e0107c6830..c1735513ff6dcc755daf06cb97da546eaded7eb9 100644
>>>> --- a/src/providers/ldap/sdap_async_connection.c
>>>> +++ b/src/providers/ldap/sdap_async_connection.c
>>>> @@ -754,6 +754,9 @@ static void simple_bind_done(struct sdap_op *op,
>>>>
>>>> if (result == LDAP_SUCCESS) {
>>>> ret = EOK;
>>>> + } else if (result == LDAP_INVALID_CREDENTIALS
>>>> + && errmsg != NULL && strstr(errmsg,
"data 775,") != NULL) {
>>> This searching of string seems to be vague to me.
>>> Will it work with any version of AD?
>> If not, we just don't display a message..I mean, this is already an
>> error path.
>>
> The ticket is about "SSSD should be about to display message to the user when
> the account in Active Directory is 'locked out'"
>
> If the string is not standardized among AD versions
> than this ticket is NOT solved.
I'm not sure we can find official documentation, at least I didn't find
one. We can test with multiple AD versions, but that's about it.
>>> Could you add link to msdn documentation where it is described that this
string
>>> is guaranted to be returned in such case?
>> It's not MSDN, but:
>>
http://www-01.ibm.com/support/docview.wss?uid=swg21290631
> I would prefer official documantation (msdn) in code ar at least in
> commit message.
Yeah, me too, please find one. I couldn't.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org