-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/03/2010 05:12 AM, Sumit Bose wrote:
> On Wed, Dec 01, 2010 at 03:12:02PM -0500, Stephen Gallagher wrote:
> Patch 0001: Ack.
> Patch 0002: This patch won't apply. Is it dependent on another patch
> currently in review? I tried atop master and atop the nine patches for
> TGT renewal, but it is still missing blobs according to git.
> 
>> This need to be applied on top of the ticket renewal patches, because
>> both change krb5_child. Maybe some changes in master causes this issue,
>> I have attached rebased versions of the patches.


Patch 0001: Ack.
Patch 0002: Nack.

+        if (strncmp(krb5_princ_realm(ctx, entry.principal)->data, realm,
+                    krb5_princ_realm(ctx, entry.principal)->length) == 0) {

I don't think you want to use strncmp() here. If the keytab has a
principal for realm MYCOMPANY, but you're looking for a key for
MYCOMPANY-IT, this comparison is going to get a false positive.

If you really want strncmp(), base the length on the sought-after realm,
not the one in the keytab.


Please explain the rationale for arbitrarily taking the last entry in
the keytab, regardless of whether it's our realm. This doesn't make much
sense to me. I'd think that if we got to the end and hadn't matched our
realm, that would be an error.

Can we avoid the stat() in check_fast_ccache()? It's a race-condition
bug waiting to happen (we stat, then the file gets deleted before we
read it). If we're just testing for existence, isn't it just as easy to
rely on get_tgt_times() returning krberr from krb5_cc_resolve()?

The manpage says that setting use_fast to "try" or "demand" should throw
an error on systems that don't support it. However,
sss_krb5_get_init_creds_opt_set_fast_ccache_name() returns 0. It should
return KRB5KDC_ERR_BADOPTION, probably (maybe there's a better code for
this?)


+
+    use_fast_str = dp_opt_get_string(opts, KRB5_USE_FAST);
+    if (use_fast_str != NULL) {
+        ret = check_fast(use_fast_str, &krb5_ctx->use_fast);
+        if (ret != EOK) {
+            DEBUG(1, ("check_fast failed.\n"));
+            return ret;
+        }
+
+        ret = setenv(SSSD_KRB5_USE_FAST, use_fast_str, 1);
+        if (ret != EOK) {
+            DEBUG(2, ("setenv [%s] failed.\n", SSSD_KRB5_USE_FAST));
+        }
+    }
+

If we're not setting the environment variable when use_fast_str == NULL,
we should probably also avoid setting it if krb5_ctx->use_fast has
returned false. Just to avoid an unnecessary and slow syscall.


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkz5DJMACgkQeiVVYja6o6PxXwCfYu0cQcCmdFYIDT8h5//7CKTb
hrYAn2T0g4Q2ZUQQrRq+yLHJLtKy79EI
=TIMw
-----END PGP SIGNATURE-----
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

Reply via email to