Bug 15104 - --use-kerberos with no arg (as a final option) doesn't fail
Summary: --use-kerberos with no arg (as a final option) doesn't fail
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Tools (show other bugs)
Version: 4.16.2
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andreas Schneider
QA Contact: Samba QA Contact
Depends on:
Reported: 2022-06-22 02:28 UTC by Andrew Bartlett
Modified: 2024-04-23 13:07 UTC (History)
4 users (show)

See Also:

patch for 4.16 (10.42 KB, patch)
2022-06-22 14:53 UTC, Andreas Schneider
asn: review? (metze)
asn: review? (abartlet)
patch for 4.15 (10.42 KB, patch)
2022-06-24 05:36 UTC, Andreas Schneider
asn: review? (metze)
abartlet: review-

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2022-06-22 02:28:27 UTC
The new --use-kerberos option was added to get unambigious behaviour, however if used as the final option it is ignored, rather than failing.  (The 'source4' command line tools had a similar issue with -k). 

This is because the parsing logic is bracketed in 

if (arg != NULL)

For example, this --use-kerberos login to an IP address 'succeeds', when it should fail with a syntax error.

abartlet@addc:/data/samba/git/samba9$ bin/smbclient //$SERVER_IP/netlogon -U$DC_USERNAME%$DC_PASSWORD --use-kerberos
Try "help" to get a list of possible commands.
smb: \> q
abartlet@addc:/data/samba/git/samba9$ echo bin/smbclient //$SERVER_IP/netlogon -U$DC_USERNAME%$DC_PASSWORD --use-kerberos
bin/smbclient // -UAdministrator%locDCpass1 --use-kerberos

The same behaviour is seen in master and Samba 4.15 (at least). 

The issue here is that the user wanted Kerberos, because they explicitly said --use-kerberos, but didn't get it and a fallback to NTLM was done instead.
Comment 1 Samba QA Contact 2022-06-22 11:50:04 UTC
This bug was referenced in samba master:

Comment 2 Andreas Schneider 2022-06-22 14:53:05 UTC
Created attachment 17388 [details]
patch for 4.16
Comment 3 Andreas Schneider 2022-06-24 05:36:22 UTC
Created attachment 17391 [details]
patch for 4.15
Comment 4 Andrew Bartlett 2022-06-26 21:45:03 UTC
Comment on attachment 17391 [details]
patch for 4.15

Per https://gitlab.com/samba-team/samba/-/merge_requests/2592#note_1005887255 I'm not comfortable with what seems like a user-visible behaviour change demonstrated by 

[PATCH 1/4] testprogs: Fix auth with smbclient and krb5 ccache

I think the master patches might need some more thought.   In the source4 code, '-k yes' never required specifying the credentials cache in the past, and '--use-kerberos=required' should behave the same way.
Comment 5 Andreas Schneider 2022-06-27 05:13:08 UTC
The fix here has nothing todo with the UX of the cmdline client. If you want to change that, please work with metze on fixing gse.c and discuss the UX with him.

This fixes a currently broken test.
Comment 6 Andrew Bartlett 2022-06-27 05:14:25 UTC
(In reply to Andreas Schneider from comment #5)
So this patch isn't required for the other changes?
Comment 7 Andreas Schneider 2022-06-27 11:04:45 UTC
It fixes the test. Without it, the test hangs and asks for a user password, but we assume that the KRB5CCACHE is used in the test.

So I need to first fix the tests so I can successfully run tests on my local machine.
Comment 8 Andrew Bartlett 2022-06-27 19:36:14 UTC
(In reply to Andreas Schneider from comment #7)
In which case my NACK holds, because that implies a behaviour change other than with the missing arguments (as those were specified in that test).

I'm sure we can get to the bottom of all this, but so far this isn't making me feel comfortable.
Comment 9 Andrew Bartlett 2022-06-28 02:26:57 UTC
(In reply to Andrew Bartlett from comment #8)
Is this a case of 'needed for operation in a shell (where stdin can block), but was working in the pipeline (where stdin is /dev/null)'?
Comment 10 Andreas Schneider 2022-06-28 13:57:46 UTC
You have this usage everywhere else in blackbox tests. Looks like those two have been missing ...

Yes, this does not block on CI but locally on stdin ...

Also the problem is with --use-kerberos=required is, you do not know if the user wants to be asked for a password and do a kinit or if we wants to use the default ccache. Also we do not have code which actually check if a ccache is has a valid ticket which isn't expired. What to do if it is expired, asked the user again for a password and do a kinit?

If you have a dedicated option foo --use-default-krb5-ccache you know what the user wants.

This should maybe discussed on samba-technical. However not applying this patch means a develper can't run make test locally.