Bug 11631 - DIR krb5 ccache not covered
DIR krb5 ccache not covered
Status: NEW
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind
4.2.4
All All
: P5 enhancement
: ---
Assigned To: Samba QA Contact
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-02 22:55 UTC by Lars Müller
Modified: 2015-12-09 00:46 UTC (History)
3 users (show)

See Also:


Attachments
winbindd_pam DIR krb5 ccache patch (927 bytes, patch)
2015-12-02 22:55 UTC, Lars Müller
no flags Details
winbindd_pam DIR krb5 ccache patch v2 (1.23 KB, patch)
2015-12-03 21:56 UTC, Lars Müller
no flags Details
winbindd_pam DIR krb5 ccache patch v3 (1.22 KB, patch)
2015-12-03 22:11 UTC, Lars Müller
no flags Details
winbindd_pam DIR krb5 ccache patch v4 (966 bytes, patch)
2015-12-04 13:25 UTC, Lars Müller
asn: review-
Details
winbindd_pam DIR krb5 ccache patch v5 (1.04 KB, patch)
2015-12-08 13:01 UTC, Lars Müller
lars: review? (gd)
asn: review-
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Müller 2015-12-02 22:55:50 UTC
Created attachment 11650 [details]
winbindd_pam DIR krb5 ccache patch

FILE and KEYRING are working as krb5 ccache type.  DIR isn't covered by
source3/winbindd/winbindd_pam.c

The attached patch fixes the general missing feature.  It also ensures
to work if /run/user/%d isn't yet available from the systemd side.
Comment 1 Lars Müller 2015-12-02 22:58:09 UTC
Comment on attachment 11650 [details]
winbindd_pam DIR krb5 ccache patch

The DEBUG statement might be superfluous.
Comment 2 Lars Müller 2015-12-03 15:15:52 UTC
Comment on attachment 11650 [details]
winbindd_pam DIR krb5 ccache patch

Also it might be of value to verify if run_user_dir doesn't already
exist.
Comment 3 Lars Müller 2015-12-03 21:56:57 UTC
Created attachment 11653 [details]
winbindd_pam DIR krb5 ccache patch v2

this version checks if the DIR doesn't exist yet before creating it
Comment 4 Lars Müller 2015-12-03 22:11:45 UTC
Created attachment 11654 [details]
winbindd_pam DIR krb5 ccache patch v3

this version applies cleanly against the current master
Comment 5 Lars Müller 2015-12-03 22:50:27 UTC
Instead of checking the return code of open() if the directory already
exists we might create the directory and only adjust the permissions if
the operation succeeded.

This would safe us the extra open(), close(), and include.
Comment 6 Andreas Schneider 2015-12-04 08:36:02 UTC
IIRC that the directory didn't exist yet was a race condition with systemd. There was no real or good solution for that so we decided not to use DIR at all and implement the KEYRING feature. You should go with KEYRING in SUSE too. The DIR option is simply to racy with systemd.

See:

https://bugzilla.redhat.com/show_bug.cgi?id=985107#c18
Comment 7 Andreas Schneider 2015-12-04 12:58:57 UTC
Comment on attachment 11654 [details]
winbindd_pam DIR krb5 ccache patch v3

You should use directory_create_or_exist()
Comment 8 Lars Müller 2015-12-04 13:25:26 UTC
Created attachment 11667 [details]
winbindd_pam DIR krb5 ccache patch v4

This version checks if mkdir() succeeds and only in this case changes
the owner.
Comment 9 Lars Müller 2015-12-04 13:27:13 UTC
(In reply to Andreas Schneider from comment #6)

Going with keyring isn't an option for older products where this kernel
feature wasn't available.
Comment 10 Andreas Schneider 2015-12-04 13:28:24 UTC
Comment on attachment 11667 [details]
winbindd_pam DIR krb5 ccache patch v4

You should still use directory_create_or_exist() and not mkdir() ;)
Comment 11 Lars Müller 2015-12-04 13:46:49 UTC
In opposite to mkdir() directory_create_or_exist() returns true even if
the directory already exits.  Or am I getting the code wrong?
Comment 12 Andreas Schneider 2015-12-04 16:15:00 UTC
Yes, that's true. I don't think that the chown hurts on an already existing directory. But I might be wrong. However the function deals with umask() correctly and check that is has been created correctly. That's why we have
directory_create_or_exist() and 
directory_create_or_exist_strict().
Comment 13 Lars Müller 2015-12-04 17:36:45 UTC
The chown() "hurts" as is forces the root owning group to a directory
which formerly might have been owned by the users primary group.

That was the reason to add the check if the directory already exists.
If it exits there is no need to modify the permissions.  Think only of a
user which adds some special hooks to modify the permissions in a way we
havn't yet in mind.

If you see an easy approach to pass the correct group as third argument
to chown() I'm happy to simplify the patch.  But currently I see only
the option to pass -1 as third arg to chown().
Comment 14 Andreas Schneider 2015-12-04 21:08:01 UTC
With a permission mask of 0700 it doesn't look like the group would matter in this case ;)
Comment 15 Andreas Schneider 2015-12-07 18:38:17 UTC
Comment on attachment 11667 [details]
winbindd_pam DIR krb5 ccache patch v4

If you do not want to use directory_create_or_exist() you should set umask(0) before you're doing mkdir() an reset the mask afterwards. Also please do not use.

if (mkdir())

Use

rc = mkdir()
if (rc == 0)
Comment 16 Lars Müller 2015-12-08 13:01:44 UTC
Created attachment 11690 [details]
winbindd_pam DIR krb5 ccache patch v5
Comment 17 Andreas Schneider 2015-12-08 17:47:53 UTC
Comment on attachment 11690 [details]
winbindd_pam DIR krb5 ccache patch v5

The patch looks fine now, but it needs to be a 'git format-patch' for the master branch and it doesn't update the manpage :)

See f942d019d183f2f6acb7c9a93f0128d22ba93b7a

However, we had support for this, but we removed it, see

13094dc8f6777e6d3d17cfd30fa6adf670702949

The reason is that this is not the default directory, there is not default. It is for distributions with systemd but not for others and obviously not for BSD.

So we shouldn't need to configure this via pam_winbind.

The right thing to do for all cases, FILE, DIR, KEYRING would be to read the krb5.conf file and get the value from the 'default_ccache_name' option.

/etc/krb5.conf example:
default_ccache_name = KEYRING:persistent:%{uid}

Code to read it:
krb5_init_context(&ctx);
ret = krb5_get_profile(ctx, &p);
ret = profile_get_string(p, "libdefaults", "default_ccache_name", NULL, NULL, &value);
krb5_free_context(ctx);

if no name is specified, we could fall back to the code you added and the others ...
Comment 18 Jeremy Allison 2015-12-09 00:46:59 UTC
One quick comment - returns from talloc_asprintf() need checking for NULL returns (out of memory errors) !

Jeremy.