[sssd PR#883][opened] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: opened

PR body:
"""
Main purpose of this patches is to resolve: https://pagure.io/SSSD/sssd/issue/4081

It also fixes one coverity issue:
```
Error: DEADCODE (CWE-561):
sssd-2.2.3/src/util/sss_krb5.c:375: cond_const: Condition "(kt_err = krb5_kt_next_entry(ctx, keytab, &entry, &cursor)) == 0", taking true branch. Now the value of "kt_err" is equal to 0.
sssd-2.2.3/src/util/sss_krb5.c:406: const: At condition "kt_err != 0", the value of "kt_err" must be equal to 0.
sssd-2.2.3/src/util/sss_krb5.c:406: dead_error_condition: The condition "kt_err != 0" cannot be true.
sssd-2.2.3/src/util/sss_krb5.c:406: dead_error_line: Execution cannot reach the expression "kt_err != -1765328202L" inside this statement: "if (kt_err != 0 && kt_err !...".
#  404|   
#  405|       /* check if we got any errors from krb5_kt_next_entry */
#  406|->     if (kt_err != 0 && kt_err != KRB5_KT_END) {
#  407|           DEBUG(SSSDBG_CRIT_FAILURE, "Error while reading keytab.\n");
#  408|           goto done;
```
"""

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

[sssd PR#883][synchronized] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: synchronized

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

[sssd PR#883][+Changes requested] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Changes requested

[sssd PR#883][edited] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: edited

Changed field: body
Original value:
"""
Main purpose of this patches is to resolve: https://pagure.io/SSSD/sssd/issue/4081

It also fixes one coverity issue:
```
Error: DEADCODE (CWE-561):
sssd-2.2.3/src/util/sss_krb5.c:375: cond_const: Condition "(kt_err = krb5_kt_next_entry(ctx, keytab, &entry, &cursor)) == 0", taking true branch. Now the value of "kt_err" is equal to 0.
sssd-2.2.3/src/util/sss_krb5.c:406: const: At condition "kt_err != 0", the value of "kt_err" must be equal to 0.
sssd-2.2.3/src/util/sss_krb5.c:406: dead_error_condition: The condition "kt_err != 0" cannot be true.
sssd-2.2.3/src/util/sss_krb5.c:406: dead_error_line: Execution cannot reach the expression "kt_err != -1765328202L" inside this statement: "if (kt_err != 0 && kt_err !...".
#  404|   
#  405|       /* check if we got any errors from krb5_kt_next_entry */
#  406|->     if (kt_err != 0 && kt_err != KRB5_KT_END) {
#  407|           DEBUG(SSSDBG_CRIT_FAILURE, "Error while reading keytab.\n");
#  408|           goto done;
```
"""

[sssd PR#883][synchronized] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: synchronized

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
@sumit-bose, @frozencemetery, thank you for a review.
I think I addressed all found issues.
"""

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

[sssd PR#883][+Waiting for review] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Waiting for review

[sssd PR#883][-Changes requested] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Changes requested

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[frozencemetery]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

frozencemetery commented:
"""
I'm kind of confused that you think that, since we pretty clearly disagree: you claim that you've found a double-free, and I'm telling you that free(NULL) is fine and normal.
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
> I'm kind of confused that you think that, since we pretty clearly disagree: you claim that you've found a double-free, and I'm telling you that free(NULL) is fine and normal.

`free(NULL)` is fine and normal because man page says so explicitly.

`krb5_free_keytab_entry_contents(structure_that_was_memset(0))` is not until docs explicitly say other way.

Lets put it other way: do you have any reasonable objection against patch?
I can edit commit message replacing "fixed error" with "put sanity guard" if you want.
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[frozencemetery]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

frozencemetery commented:
"""
Yes, of course I object!  You're claiming a double free; there's no double free.  You haven't found a security bug; you've found nothing at all.
"""

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

[sssd PR#883][-Waiting for review] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Waiting for review

[sssd PR#883][synchronized] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: synchronized

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
> Yes, of course I object! You're claiming a double free; there's no double free.

Ok, I see your point.

I have rewritten this patch entirely (to avoid treating failure of krb5_kt_end_seq_get() as an fatal error) and removed "double-free claim" from commit message.

Still I am bold in my statement that robust code should not rely on undocumented features (i.e. krb5_free_keytab_entry_contents(null_struct)) and this is still reflected in this patchset.
"""

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

[sssd PR#883][+Waiting for review] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Waiting for review

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
Hi @sumit-bose, am I expected to fix anything else to make next review round possible?

"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

sumit-bose commented:
"""
Hi,

sorry for the delay. The code looks good and Coverity didn't find any new issues and said the deadcode issue is solved.

Also the debug messages are looking better now. But after checking the original ticket which asked for more specific debug messages I wonder if they  can be improved even further by
 - adding a sss_log messages with the final error message to find_principal_in_keytab() as well to make the reason why a keytab cannot be read (not exists, empty or other reason) more clear
 - if the default keytab is used to not just print 'default' in the debug/log messages in select_principal_from_keytab() but the real path of the default keytab.

Please let me know what you think?

bye,
Sumit
"""

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

[sssd PR#883][-Waiting for review] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Waiting for review

[sssd PR#883][+Changes requested] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Changes requested

[sssd PR#883][synchronized] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: synchronized

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

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

please take a look at another attempt.

To address your second remark I have introduced `sss_printable_keytab_name()` helper.
If its impl is ok then I think we should make use of it instead of `KEYTAB_CLEAN_NAME` in ldap/krb5_child as well (but I think as another PR since it goes of scope)

Btw, during testing I have discovered that do-while loop in `select_principal_from_keytab()` is written in a such a way that it may try (primary=null)&(realm=null) at the last step. How do you think, is it an intent? I guess it is an error.

"""

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

[sssd PR#883][+Waiting for review] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Waiting for review

[sssd PR#883][-Changes requested] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Changes requested

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
I also think it is possible to stop trying different `primary`&`realm` after first fatal (i.e. not KRB5_KT_NOTFOUND) error returned by `find_principal_in_keytab()`.

But not sure if it is worth fixing.
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

sumit-bose commented:
"""
Hi,

the patches are looking good and the log messages, e.g shown with `systemctl status sssd`, are now quite descriptive.

I agree that replacing `KEYTAB_CLEAN_NAME` can be handled in the different PR.

Trying with (primary=null)&(realm=null) is intended as a last resort to just pick any principal which would mean to pick the first.

I think it is currently not worth stopping after "fatal" keytab errors, the multiple log messages might even help to make it more clear that there is an issue which should be fixed.

I only have a minor nitpick about ` static char buff[512];`. The `512` comes a bit out of the blue and one might ask why not 256 or 1024? I'd like to ask you to either add a comment that 512 is consider sufficient to store any kind of keytab description or use something like `sizeof("FILE:")+PATH_MAX` as size.

bye,
Sumit
"""

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

[sssd PR#883][-Waiting for review] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Waiting for review

[sssd PR#883][+Changes requested] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Changes requested

[sssd PR#883][synchronized] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: synchronized

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

[sssd PR#883][synchronized] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: synchronized

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
retest this please
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
retest this please
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
Hi,
> I only have a minor nitpick about ` static char buff[512];`. The `512` comes a bit out of the blue and one might ask why not 256 or 1024? I'd like to ask you to either add a comment that 512 is consider sufficient to store any kind of keytab description or use something like `sizeof("FILE:")+PATH_MAX` as size.

Thank you. I opted to add a comment (and rebased on current master).

In regards of PATH_MAX:
1) If I understand correctly, this value is merely max buffer size of some syscalls. It has nothing to do with actual maximum file path length (which can be literally arbitrary). Some details can be found [here](http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html) and [here](https://eklitzke.org/path-max-is-tricky). Thus IMO it doesn't actually guarantee that result will fit a buffer (but rather suggests a false assurance of this and so is a little bit misleading).
2) Additionally posix [doesn't guarantee](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html) PATH_MAX to be constant/defined at all and (even if defined) different platforms use different headers for this definition, so it is tricky to use this properly.
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
retest this please
"""

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

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

alexey-tikhonov commented:
"""
retest this please
"""

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

[sssd PR#883][-Changes requested] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Changes requested

[sssd PR#883][+Waiting for review] Minor fixes to util/sss_krb5

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Waiting for review

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

sumit-bose commented:
"""
Hi Alexey,

thanks for adding the comment. ACK.

bye,
Sumit
"""

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

[sssd PR#883][+Accepted] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Accepted

[sssd PR#883][-Waiting for review] Minor fixes to util/sss_krb5

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Waiting for review

[sssd PR#883][+Ready to push] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Ready to push

[sssd PR#883][comment] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

pbrezina commented:
"""
* `master`
    * e778fa18a8d5f602c7d16087c42ec7861ffceb9f - util/sss_krb5: debug messages fixes
    * 716aebab5de67979f1afd5a14140042fb5e5567c - util/sss_krb5: fixed few memory handling issues
    * 8f275460a8831b2ab1fe3269d9a0ee3d88d3339f - util/sss_krb5: find_principal_in_keytab() was amended
    * 5086353ebad66d430aac40efff9778f0a0d2ce7c - util/sss_krb5.c: elimination of unreachable code

"""

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

[sssd PR#883][+Pushed] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: +Pushed

[sssd PR#883][-Accepted] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Accepted

[sssd PR#883][-Ready to push] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Title: #883: Minor fixes to util/sss_krb5

Label: -Ready to push

[sssd PR#883][closed] Minor fixes to util/sss_krb5

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/883
Author: alexey-tikhonov
 Title: #883: Minor fixes to util/sss_krb5
Action: closed

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