On Wed, Dec 01, 2010 at 02:58:48PM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE-----
On 11/15/2010 08:49 AM, Sumit Bose wrote:
> 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
I'm wondering whether it makes sense to have the renewable length
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.
opened ticket #697 to track this
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.
sorry, this was a copy-and-paste error, the following
SAFEALIGN_COPY_INT64 is only needed
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
we discussed this on irc and agreed that the current approach has its
values too, e.g. helps to reduce the number of requests to the KDC. So
we keep it in the current state.
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.
ah, sorry, forgot the -1, also fixed the 'unused variable' warning.
Thanks for the review, new versions attached.
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
-----END PGP SIGNATURE-----
sssd-devel mailing list