On 2017-11-06 13:08:25, Viktor Dukhovni wrote:
>
> See:
>
> https://github.com/heimdal/heimdal/commit/d2130e3312089a3142e89b316d2900fa21855726
>
Interesting! I notice that patch does not apply to 7.4.0 as-is, is there
a new release planned soon or am I better off spending time getting it
to apply to the current release?
>
> I'd also like to recommend the "prune-key-history" setting...
>
I have completely missed that functionality, thanks for pointing it out.
After testing it for a bit I have a few questions:
The documentation and commit messages only mentions that keys are pruned
when you add new keys, but I noticed that doing a "get" with kadmin will
also prune keys (because it is called in kadm5_s_get_principal()). Since
I have traditionally seen "get" as a read-only operation maby this
should be mentioned? This means that you can not inspect the database
(short of dumping it with kadmin -l dump) without possibly altering it
which might not be expected (though I do see the helpful side of being
able to easily prune keys on demand).
Additionally, I see that the prune code basically figures out the most
recent keyset timestamp that is older than the ticket maximum lifetime and
then removes all keysets with a timestamp older than that one. This
means that this too old but most recent keyset will be kept, but since
it was calculated to be older than max-lifetime, shouldnt it be removed
as well? As it stands now there will always remain at least one keyset
(depending on if there are more than one keyset with the same timestamp)
too old to be useful.
Would it make sense to alter the code to also remove that most recent
(but too old) keyset? The only functional difference of the diff below
is the "<" to "<=" change at the bottom, but it felt like it would
warrant a more fitting variable name as well:
===
diff --git a/lib/hdb/keys.c b/lib/hdb/keys.c
index 96c221e..44de789 100644
--- a/lib/hdb/keys.c
+++ b/lib/hdb/keys.c
@@ -237,7 +237,7 @@ hdb_prune_keys(krb5_context context, hdb_entry *entry)
"kadmin", "prune-key-history", NULL)) {
hdb_keyset *elem;
time_t ceiling = time(NULL) - *entry->max_life;
- time_t keep_time = 0;
+ time_t prune_time = 0;
size_t i;
/*
@@ -247,15 +247,15 @@ hdb_prune_keys(krb5_context context, hdb_entry *entry)
for (i = 0; i < nelem; ++i) {
elem = &keys->val[i];
if (elem->set_time && *elem->set_time < ceiling
- && (keep_time == 0 || *elem->set_time > keep_time))
- keep_time = *elem->set_time;
+ && (prune_time == 0 || *elem->set_time > prune_time))
+ prune_time = *elem->set_time;
}
/* Drop obsolete entries */
- if (keep_time) {
+ if (prune_time) {
for (i = 0; i < nelem; /* see below */) {
elem = &keys->val[i];
- if (elem->set_time && *elem->set_time < keep_time) {
+ if (elem->set_time && *elem->set_time <= prune_time) {
remove_HDB_Ext_KeySet(keys, i);
/*
* Removing the i'th element shifts the tail down, continue
===
I can open a PR at github if this makes sense to you. I have been able to test
it successfully against 7.4.0 at least.
--
Patrik Lundin
