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 on attachment 11650 [details] winbindd_pam DIR krb5 ccache patch The DEBUG statement might be superfluous.
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.
Created attachment 11653 [details] winbindd_pam DIR krb5 ccache patch v2 this version checks if the DIR doesn't exist yet before creating it
Created attachment 11654 [details] winbindd_pam DIR krb5 ccache patch v3 this version applies cleanly against the current master
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.
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 on attachment 11654 [details] winbindd_pam DIR krb5 ccache patch v3 You should use directory_create_or_exist()
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.
(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 on attachment 11667 [details] winbindd_pam DIR krb5 ccache patch v4 You should still use directory_create_or_exist() and not mkdir() ;)
In opposite to mkdir() directory_create_or_exist() returns true even if the directory already exits. Or am I getting the code wrong?
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().
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().
With a permission mask of 0700 it doesn't look like the group would matter in this case ;)
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)
Created attachment 11690 [details] winbindd_pam DIR krb5 ccache patch v5
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 ...
One quick comment - returns from talloc_asprintf() need checking for NULL returns (out of memory errors) ! Jeremy.