-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/15/2010 08:49 AM, Sumit Bose wrote:
Hi,
this series for patches add support for automatic Kerberos ticket
renewal, see also trac ticket #369.
There are several things I like to discuss:
- in the ticket a separate process which should handle the renewal was
mentioned. Currently the patches just create a timed task in the krb5
provider because I think most of the typically uses cases do not
justify to overhead we create with a separate process. But I'm open
for other arguments.
- I have added option to request TGT with a specific lifetime/renewal
time. The corresponding option in krb5.conf have a trailing letter
indicating the time unit. I have copied this behaviour to help
migrations although we typically use only seconds without a unit in
sssd.conf. Is this a good idea or shall I change it to seconds or do
we want to support both formats.
- Currently everything is held in RAM and after a restart nothing is
renewed automatically. I plan to send a new patch which checks all
ccfiles we have in the cache and if renewal is possible it adds them
to the list at startup. I think this approach makes more sense than
writing the list of renewable ticket to disk. Do you agree?
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Nack. Please check spelling/grammar in the manpage entry.
("possible" is misspelled and "hour" should be plural)
Also, please use something more descriptive than "dummy". That moniker
should be reserved for actual dummy variables whose contents we don't
care about.
I'm wondering whether it makes sense to have the renewable length as an
environment variable. I think it might be wiser to pass that as an
argument. There may be times in the future where we want to
differentiate renewable length by group or other distinguishing feature
of an account.
Patch 0005: Nack for most of the same reasons as patch 0004
Patch 0006: Ack
Patch 0007: Nack
This looks wrong to me:
SAFEALIGN_COPY_INT32(&int64_value, buf+p, NULL);
If I'm misreading it and your intention actually is to ignore a leading
32-bit value here, please add a comment explaining it.
Also, I'm concerned about casting a 64-bit value to time_t (since time_t
is "long int" on most platforms, which means only 32-bits on i386).
Probably we want to add a routine to check the range and clamp it down
to time_t max if it's too large. Also, instead of using int64_value,
please use different variables with clearer names.
Patch 0008: Nack.
This might be a matter of optimizing too early, but why do we have a
renew interval instead of just setting the tevent timer based on the
next ticket that's due to be renewed? Is there any advantage to
maintaining the hash table of entries? I think it would make more sense
to just create timed events, rather than adding TGTs to a hash table.
Then we don't have to iterate across all of them if only one is due for
update.
Also, if we're offline when the renewal period hits, we should add a
renewal action to the list of going_online callbacks, rather than just
rescheduling and hoping for the best. This way, if we happen to go
online two minutes before the expire time would be up, they'll get the
renewal in on time.
So to summarize: my view would be that when we get a TGT, we should just
create a timed event that would fire at max_renewal_time/2. If this
renewal succeeds, reschedule the event for new_max_renewal_time/2, and
if we're offline, add it to going_online callbacks so it will be tried
as soon as we go online after that.
Patch 0009: Nack.
str[strlen(str)] is always going to be '\0', unless I miss my guess. So
I don't think this if statement does what you think it does.
- --
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/
iEYEARECAAYFAkz2qPgACgkQeiVVYja6o6MtRQCfRU/v+uRS4xC/5v0wPonLxiZ5
H9cAnAvdkHgkkE1WyMRp+Wzz3NdHP79M
=fZDQ
-----END PGP SIGNATURE-----