On 2017-11-07 10:55:08, Patrik Lundin wrote:
>
> 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:
>
I now realize that this patch alone is not good enough.
Doing some more tests I noticed that if I run with this patch and do
"cpw" of an existing principal with --keepold it will instantly prune
the newly archived keyset if the "Last password change" field was
older than the maximum ticket lifetime of the keys being rotated.
The reason for this instant pruning is that
hdb_add_current_keys_to_history() called as a part of "cpw" will use the
timestamp recorded in "Last password change" as the timestamp written to
the archived keyset, and then call hdb_prune_keys() which will see this
timestamp as too old and directly discard it.
This makes me wonder if the timestamp in an archived keyset should
reflect the "Last password change" field of the key it represents, or if
the timestamp should instead reflect the point in time that it got
archived. If the latter one makes sense that would probably make the
earlier mentioned patch work as expected since it would then reflect the
last time it could have been used to issue tickets.
An updated patch that seems to work better:
===
diff --git a/lib/hdb/keys.c b/lib/hdb/keys.c
index 96c221e..81468ff 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
@@ -309,9 +309,7 @@ hdb_add_current_keys_to_history(krb5_context context,
hdb_entry *entry)
/*
* Copy in newest old keyset
*/
- ret = hdb_entry_get_pw_change_time(entry, &newtime);
- if (ret)
- goto out;
+ time(&newtime);
memset(&newkeyset, 0, sizeof(newkeyset));
newkeyset.keys = entry->keys;
===
This of course changes the semantics of what an archived keyset
contains, and maby there are other parts of ecosystem that would get
confused by this.
--
Patrik Lundin
