[sssd PR#958][opened] ldap_child: do not try PKINIT

[sumit-bose]

   URL: https://github.com/SSSD/sssd/pull/958
Author: sumit-bose
 Title: #958: ldap_child: do not try PKINIT
Action: opened

PR body:
"""
if the PKINIT plugin is installed and pkinit_identities is set in
/etc/krb5.conf libkrb5 will try to do PKINIT although ldap_child only
wants to authenticate with a keytab. As a result ldap_child might try to
access a Smartcard which is either not allowed at all or might cause
unexpected delays.

To avoid this the current patch sets pkinit_identities for LDAP child
explicitly to make the PKINIT plugin fail because if installed libkrb5
will always use it.

It turned out the setting pre-authentication options requires some
internal flags to be set and krb5_get_init_creds_opt_alloc() must be
used to initialize the options struct.

Related to https://pagure.io/SSSD/sssd/issue/4126
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/958/head:pr958
git checkout pr958

[sssd PR#958][+Waiting for review] ldap_child: do not try PKINIT

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

Label: +Waiting for review

[sssd PR#958][comment] ldap_child: do not try PKINIT

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

alexey-tikhonov commented:
"""
Hi @sumit-bose,

Patch LGTM, I have only one really minor nitpick. Covscan is clean. Results of manual
verification are as expected.

If you prefer to not address my nitpick, please let me know and I will mark PR accepted as
is.
"""

See the full comment at https://github.com/SSSD/sssd/pull/958#issuecomment-565199767

[sssd PR#958][-Waiting for review] ldap_child: do not try PKINIT

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

Label: -Waiting for review

[sssd PR#958][comment] ldap_child: do not try PKINIT

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

sumit-bose commented:
"""
Hi @alexey-tikhonov,

I agree with your general precaution with 3rd party libraries. But I think we can make
exceptions e.g. for libkrb5 when we have the confidence or the confirmation from
developers that it is expected that functions that free data all handle NULL. Having
explicitly mention this in the documentation is preferred, of course, but even then it is
only documentation and the implementation might have issues nonetheless.

Would do you think if we e.g. explicitly state in
https://docs.pagure.org/SSSD.sssd/developers/coding_style.html or even somewhere in the
code that it can be safely assumed that "krb5 functions that free data all handle
NULL" (https://github.com/SSSD/sssd/pull/883#discussion_r325748177) and that it is a
bug in libkrb5 if it is not the case? Or as an alternative add configure checks where we
call the krb5_*_free() calls we use with NULL to make sure it is safe?

Please note, I see libkrb5 here as an example, we can do this with other libraries as
well. But I think adding NULL checks before free calls make the code less readable
(although only a little bit) and should be avoided.

bye,
Sumit 
"""

See the full comment at https://github.com/SSSD/sssd/pull/958#issuecomment-565372718

[sssd PR#958][+Accepted] ldap_child: do not try PKINIT

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

Label: +Accepted

[sssd PR#958][comment] ldap_child: do not try PKINIT

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

alexey-tikhonov commented:
"""
Fine, as I wrote, this was really insignificant comment thus patch is accepted.

I do not think this is matter of coding *style*.

Configure check is an interesting idea but I do not think it justifies an effort.


>  Having explicitly mention this in the documentation is preferred, of
> course, but even then it is only documentation and the implementation might have issues
> nonetheless.
>  
...
>  that it can be safely assumed that "krb5 functions that free
> data all handle NULL" and that it is a bug in libkrb5 if it is not the case?
>  
From my point of view, only having this info stated in krb5 docs explicitly would justify
such a statement (kind of "liability shift"). 

But then again, I agree this is not something that have to be seriously addressed.
"""

See the full comment at https://github.com/SSSD/sssd/pull/958#issuecomment-565392480

[sssd PR#958][comment] ldap_child: do not try PKINIT

[mzidek-rh]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

mzidek-rh commented:
"""
master:
* 580d61884b6c0a81357d8f9fa69fe69d1f017185
"""

See the full comment at https://github.com/SSSD/sssd/pull/958#issuecomment-565662911

[sssd PR#958][+Pushed] ldap_child: do not try PKINIT

[mzidek-rh]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

Label: +Pushed

[sssd PR#958][-Accepted] ldap_child: do not try PKINIT

[mzidek-rh]

  URL: https://github.com/SSSD/sssd/pull/958
Title: #958: ldap_child: do not try PKINIT

Label: -Accepted

[sssd PR#958][closed] ldap_child: do not try PKINIT

[mzidek-rh]

   URL: https://github.com/SSSD/sssd/pull/958
Author: sumit-bose
 Title: #958: ldap_child: do not try PKINIT
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/958/head:pr958
git checkout pr958