Dne 24.11.2014 v 15:12 Alexander Bokovoy napsal(a):
On Mon, 24 Nov 2014, Jan Cholasta wrote:
https://fedorahosted.org/freeipa/ticket/4713
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c
b/daemons/ipa-kdb/ipa_kdb_mspac.c
index c8f6c76..debcd1b 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -2071,6 +2071,9 @@ krb5_error_code
ipadb_sign_authdata(krb5_context
context,
                           ipactx->kdc_hostname,
strlen(ipactx->kdc_hostname),
                           NULL, NULL, &result) == 0) {
               kerr = ipadb_reinit_mspac(ipactx, true);
+                if (kerr != 0 && kerr != ENOENT) {
+                    goto done;
+                }
           }
       }

I'm not sure we should drop the sign_authdata request here. If we
were
able to re-initialize our view of trusted domains, we simply cannot
re-sign incoming PAC but this is handled in ipadb_verify_pac() and
ipadb_sign_pac() and if the former returns NULL value for PAC, we
exit
with a return code of 0 while this change will fail a cross-realm
TGT
request unconditionally.


OK, what would be a proper fix? Just ignore the return value of
ipadb_reinit_mspac here?

Guys, I did not see the code but all instances of "ignore return
code" I have
seen were wrong, including cases where code comment explicitly said
"we ignore
return code on purpose" :-)

At least log an error message if you can't think of anything better
...


I don't disagree, if that's the proper fix.

Alexander, or someone else, could you please finish the review of the
patches? Thanks.
I've ACKed other patches but I'm not going to accept this patch. Rest is
fine.

That's fine with me, but I'm still going to need a little hint on how
this should in fact be fixed.
Right. My main issue is that the change in this patch actually changes
the flow for signing MS-PAC in case we were unable to retrieve our
defaults. We need to ignore the return code of ipadb_reinit_mspac() here
because we are simply not going to add MS PAC with our signature, not
fully denying getting the ticket (that check is in a separate place)
because this affects also our realm's principals. Consider a case when
ipa-adtrust-install wasn't even run, this is where we'll get non-working
MS-PAC defaults and we silently drop the MS-PAC signing.

OK, fixed, updated patch attached.

--
Jan Cholasta
>From bf1132192a9a0ac3ee41f24c56de6e911af51b78 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 10 Nov 2014 18:10:27 +0000
Subject: [PATCH 4/8] Fix unchecked return value in ipa-kdb

https://fedorahosted.org/freeipa/ticket/4713
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index c8f6c76..a450007 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -2070,7 +2070,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
                             krb5_princ_component(context, ks_client_princ, 1)->length,
                             ipactx->kdc_hostname, strlen(ipactx->kdc_hostname),
                             NULL, NULL, &result) == 0) {
-                kerr = ipadb_reinit_mspac(ipactx, true);
+                (void)ipadb_reinit_mspac(ipactx, true);
             }
         }
 
-- 
2.1.0

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to