On Tue, Dec 02, 2014 at 09:40:42PM +0100, Sumit Bose wrote:
On Tue, Dec 02, 2014 at 02:54:42PM +0100, Jakub Hrozek wrote:
> Hi,
>
> these patches depend on Sumit's "[PATCHES] ldap_child, krb5_child: copy
> keytab and FAST ccache into memory".
>
> When applied, the FAST ccache is created as the SSSD so that no Kerberos
> networking code runs as the root user. In order to do that, the
> krb5_child receives the SSSD user IDs as parameters.
>
> I also have tests for the other functions in the child_common.c module
> but I will send them separately to speed up the review.
Patches are working well in my tests, I have a few comments to the 3rd
patch.
[...]
> From 74ae76ecea1d469ff9dc8937eacdd077707352b9 Mon Sep 17
00:00:00 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com>
> Date: Fri, 28 Nov 2014 13:04:42 +0100
> Subject: [PATCH 3/3] KRB5: Create the fast ccache in a child process
[...]
> - kerr = get_and_save_tgt_with_keytab(ctx, client_princ,
keytab, ccname);
> - if (kerr != 0) {
> - DEBUG(SSSDBG_CRIT_FAILURE, "get_and_save_tgt_with_keytab
failed.\n");
> - goto done;
> + /* Need to recreate the FAST ccache */
> + fchild_pid = fork();
> + switch (fchild_pid) {
> + case -1:
> + DEBUG(SSSDBG_CRIT_FAILURE, "fork failed\n");
> + kerr = EIO;
> + goto done;
> + case 0:
> + /* Child */
please change debug_prg_name for the child
Done. I hope keeping the same program name is OK since we just fork the
same process.
> + kerr = become_user(fast_uid, fast_gid);
> + if (kerr != 0) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed: %d\n",
kerr);
> + exit(1);
> + }
> + DEBUG(SSSDBG_TRACE_INTERNAL,
> + "Running as
[%"SPRIuid"][%"SPRIgid"].\n", geteuid(), getegid());
> +
> + kerr = get_and_save_tgt_with_keytab(ctx, client_princ,
> + keytab, ccname);
> + if (kerr != 0) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "get_and_save_tgt_with_keytab failed: %d\n",
kerr);
> + exit(2);
> + }
> + exit(0);
> + default:
> + /* Parent */
> + do {
> + errno = 0;
> + kerr = waitpid(fchild_pid, &status, 0);
> + } while (kerr == -1 && errno == EINTR);
> +
> + if (kerr > 0) {
> + kerr = EIO;
> + if (WIFEXITED(status)) {
> + kerr = WEXITSTATUS(status);
> + /* Don't blindly fail if the child fails, but check
> + * the ccache again */
> + if (kerr != 0) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "Creating FAST ccache failed, krb5_child will
"
> + "likely fail!\n");
> + }
> + }
please add and else with a message the the child terminated
unexpectedly.
Done.
> + } else {
> + DEBUG(SSSDBG_FUNC_DATA,
> + "Failed to wait for children %d\n", fchild_pid);
> + kerr = EIO;
> + }
> + }
> +
> + /* Check the ccache times again. Should be updated ... */
> + memset(&tgtt, 0, sizeof(tgtt));
> + kerr = get_tgt_times(ctx, ccname, server_princ, client_princ, &tgtt);
> + if (kerr == 0) {
> + if (tgtt.endtime > time(NULL)) {
> + DEBUG(SSSDBG_FUNC_DATA, "FAST TGT was successfully
recreated!\n");
> + goto done;
> + } else {
> + kerr = ERR_CREDS_EXPIRED;
> + goto done;
> + }
> }
>
> kerr = 0;
> @@ -1889,7 +1932,8 @@ static int k5c_setup_fast(struct krb5_req *kr, bool demand)
> fast_principal = NULL;
> }
>
> - kerr = check_fast_ccache(kr, kr->ctx, fast_principal, fast_principal_realm,
> + kerr = check_fast_ccache(kr, kr->ctx, kr->fast_uid, kr->fast_gid,
> + fast_principal, fast_principal_realm,
> kr->keytab, &kr->fast_ccname);
> if (kerr != 0) {
> DEBUG(SSSDBG_CRIT_FAILURE, "check_fast_ccache failed.\n");
> @@ -2253,6 +2297,9 @@ int main(int argc, const char *argv[])
> int debug_fd = -1;
> errno_t ret;
> krb5_error_code kerr;
> + char *mem_keytab;
unused
Oops, bad merge with your patches. Fixed.
> + uid_t fast_uid;
> + gid_t fast_gid;
>
> struct poptOption long_options[] = {
> POPT_AUTOHELP
> @@ -2267,6 +2314,10 @@ int main(int argc, const char *argv[])
> {"debug-to-stderr", 0, POPT_ARG_NONE | POPT_ARGFLAG_DOC_HIDDEN,
> &debug_to_stderr, 0,
> _("Send the debug output to stderr directly."), NULL },
> + {"fast-ccache-uid", 0, POPT_ARG_INT, &fast_uid, 0,
> + _("The user to create FAST ccache as"), NULL},
> + {"fast-ccache-gid", 0, POPT_ARG_INT, &fast_gid, 0,
> + _("The group to create FAST ccache as"), NULL},
> POPT_TABLEEND
> };
>
Thank you for the review. New patches are attached.