URL:
https://github.com/SSSD/sssd/pull/5450
Title: #5450: kcm: add support for kerberos tgt renewals
pbrezina commented:
"""
Thanks a lot Pavel for the further review, it is better to get
everything resolved now than having to fix issues later.
> If the cache contains uid that is not resolvable then kcm fails to start:
> ```
> [root /var/log/sssd]# /usr/libexec/sssd/sssd_kcm --uid 0 --gid 0 --debug-level
0xfff0
> (2021-03-25 12:12:49:260824): [sssd] [become_user] (0x0200): Trying to become user
[0][0].
> (2021-03-25 12:12:49:260883): [sssd] [become_user] (0x0200): Already user [0].
> (2021-03-25 12:12:49:263412): [kcm] [ldb] (0x0400): server_sort:Unable to register
control with rootdse!
> (2021-03-25 12:12:49): [kcm] [server_setup] (0x0040): Starting with debug level =
0xfff0
> (2021-03-25 12:12:49): [kcm] [server_setup] (0x0400): CONFDB:
/var/lib/sss/db/config.ldb
> (2021-03-25 12:12:49): [kcm] [kcm_get_ccdb_be] (0x0100): KCM database type: secdb
> (2021-03-25 12:12:49): [kcm] [kcm_ccdb_init] (0x0200): KCM back end:
libsss_secrets
> (2021-03-25 12:12:49): [kcm] [ccdb_secdb_init] (0x2000): secdb initialized
> (2021-03-25 12:12:49): [kcm] [sss_sec_list_cc_uids] (0x2000): uid: [91600000]
> (2021-03-25 12:12:49): [kcm] [sss_sec_list_cc_uids] (0x2000): uid: [1000]
> (2021-03-25 12:12:49): [kcm] [ccdb_secdb_renew_init] (0x2000): Found [2] ccache
uids
> (2021-03-25 12:12:49): [kcm] [renew_check_ccaches] (0x0040): Failed to get pwd entry
for [91600000]
> (2021-03-25 12:12:49): [kcm] [ccdb_secdb_renew_init] (0x0040): Error checking
ccaches in secdb
> (2021-03-25 12:12:49): [kcm] [kcm_ccdb_renew_init] (0x0020): Failure to execute ccdb
renewal init
> (2021-03-25 12:12:49): [kcm] [kcm_process_init] (0x0010): fatal error initializing
KCM ccdb renewals
> (2021-03-25 12:12:49): [kcm] [kcm_responder_ctx_destructor] (0x0400): Responder is
being shut down
> ```
Is it valid to treat renewal failures as not fatal, and return EOK from
`kcm_ccdb_renew_init` instead of current behavior shown below? If not what is the
preferred way to handle this?
Fail to setup renewals should be fatal. But invalid uid inside the cache is not fatal, it
may happen quite easily -- user was removed from LDAP and then from SSSD cache but
kdestroy was not called before the user was removed. In the future we should probably
purge such ccaches.
> It might be better to move this to a function on its own,
something like:
> ```c
> errno_t kcm_renewals_init(...)
> {
> #ifndef HAVE_KCM_RENEWAL
> return EOK;
> #else
> do stuff
> #endif
> }
> ```
Can you help me understand what is the benefit of this change?
The benefit is that the logic is confined to the function and not to its caller. The
current code requires #ifdefs on two places. If you move it to a separate function, you
only need one #ifdef and the caller does not have to care about the support. It is not
that important at this place, but it is generally a good practice. The caller should know
how to call the function not when to call it.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/5450#issuecomment-808156268