kadmin selective prune of historic key for principal #415

bodik · 2018-09-13T08:18:49Z

Hello,

please consider and at least comment this PR.

In current implementation (without PR)

the old keys either stays in the database forever or are prunned
automagically based on max_lifetime (based on prune-key-history), but
only on cpw, and yet it left the old key in history forever.
Rotating krbtgt/ (or other principals) wihtout --keepold would disrupt
realm operations, eg. long computing grid jobs.

The added functionality:

can be used to prune historic keys from principal
history by selective kvno. The main usecase is rollover krbtgt/ principals
(self and crossrealms).
it should be backward compatible afaik despite new opcode
kadm_ops struct. During operation a 'modify' type is logged into iprop
journal but data already contains and ipro processes full key history (KEY_TL_DATA),
so even using patched kadmin --local on unpatched kadmind would work
as expected on the slaves.

kadmin/server.c

lib/hdb/keys.c

nicowilliams · 2018-12-26T23:46:04Z

lib/kadm5/prune_c.c

+        krb5_clear_error_message(context->context);
+        return ENOMEM;
+    }
+    krb5_store_int32(sp, kadm_prune);


Error checking is needed.

code is copied from other features. i'm willing to work it out.

Thanks for pointing that out. I'll fix the others, but do please fix this one.

remarked as unresolved to keep track of the things i've promissed/asked to fix

@nicowilliams please review commit 0d655f5 and resolve

kadmin/server.c

nicowilliams · 2018-12-27T05:17:26Z

Oh, I'd completely forgotten, but @vdukhovni wrote a prune history function two years ago. The idea is to delete keys that are too old to be needed for decrypting potentially still-extant tickets.

Given that, do we still need pruning by kvno?

nicowilliams · 2018-12-27T05:19:51Z

Also, maybe we should make --keepold on by default and enable prune-key-history by default as well, and maybe add an argument to cpw/randkey to --dontprune and --pruneallold or something.

bodik · 2018-12-27T16:01:01Z

Oh, I'd completely forgotten, but @vdukhovni wrote a prune history function two years ago. The idea is to delete keys that are too old to be needed for decrypting potentially still-extant tickets.

Given that, do we still need pruning by kvno?

this is true, but as I stated in the original PR, there is a problem with that feature. prune-key-history works 'automagically' only during cpw command. So in case of the key rollover, the feature is somewhat useless, because it would leave two keys (new one and the old one) for rotated principal. There's no way how to get rid of the old key once you start rolling-over with --keepold.

nicowilliams · 2018-12-27T16:22:40Z

@bodik We prune keys on every key change operation, not just cpw:

Functions calling this function: hdb_prune_keys

  File    Function                        Line
0 keys.c  hdb_add_current_keys_to_history 332 ret = hdb_prune_keys(context, entry);
1 get_s.c kadm5_s_get_principal           266 ret = hdb_prune_keys(context->context, &ent.entry);

Functions calling this function: hdb_add_current_keys_to_history

0 hdb-mitdb.c _hdb_mdb_value2entry              619 ret = hdb_add_current_keys_to_history(context, entry);
1 keys.c      hdb_change_kvno                   449 ret = hdb_add_current_keys_to_history(context, entry);
2 chpass_s.c  change                             75 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
3 chpass_s.c  kadm5_s_chpass_principal_with_key 228 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
4 randkey_s.c kadm5_s_randkey_principal          73 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
5 setkey3_s.c kadm5_s_setkey_principal_3         74 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);

In fact, we prune key history on every cpw, randkey, and on any direct calls to kadm5_setkey_principal*(). We also do it when you change a principal's kvno because Heimdal allows you to add keys for a new kvno without taking them into effect (leaving the principal's current kvno alone).

bodik · 2018-12-27T16:28:27Z

No, we prune keys on every key change operation, not just cpw:

0 hdb-mitdb.c _hdb_mdb_value2entry              619 ret = hdb_add_current_keys_to_history(context, entry);
1 keys.c      hdb_change_kvno                   449 ret = hdb_add_current_keys_to_history(context, entry);
2 chpass_s.c  change                             75 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
3 chpass_s.c  kadm5_s_chpass_principal_with_key 228 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
4 randkey_s.c kadm5_s_randkey_principal          73 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
5 setkey3_s.c kadm5_s_setkey_principal_3         74 ret = hdb_add_current_keys_to_history(context->context, &ent.entry);

well might be. but i see the rollover procedure as

generate new key to service keytab (done with ktutil)
cpw --keepold newkey to add new key to KDC
wait until all old tickets expires
?? what ops goes here to prune the old key ??

nicowilliams · 2018-12-27T16:40:01Z

@bodik You just rotate keys weekly or whatever. Yes, you'll generally have one old keyset you no longer need the KDC to know about. But even then, if you want to prune that keyset, how would you know its kvno? Why not just expose the current prune-old-keys operation and call that? Then the procedure would be:

cpw/randkey --keepold newkey to add new key to KDC (this will prune some but not all old keys)
wait until all old tickets expires
prune (no kvno needed) (this will prune all remaining old keys)

For non-krbtgt principals we only need their keys for S4U2Proxy, so we can decrypt evidence tickets, but if a realm is not using S4U2Proxy or a service is known to not use it, then we can prune its key history more aggressively, in which case the sequence would be just (1). (In principle even non-krbtgt tickets can be renewable/postdated; in practice no one has nor uses support for that in clients.)

Also, while we're here, your sequence has you generate keytab entries locally, then change them on the KDC. Now that Heimdal lets you set new keys without changing the kvno, you could first set them on the KDC, then write them to the keytab, then update the kvno to make the new keyset active. We are missing kadmin command options for this though!!

nicowilliams · 2018-12-27T16:48:23Z

See also #480.

lib/hdb/keys.c

nicowilliams · 2018-12-27T19:02:40Z

@bodik I'm happy with this PR, but I've requested one change: that if kvno == 0 then you end up calling hdb_prune_keys(). That is a significant usability improvement. Then users can:

cpw/randkey;
wait for old tickets to expire;
prune 0 (no need to look at the keytab to find out what kvnos to prune).

kadmin/kadmin-commands.in

nicowilliams · 2018-12-27T19:04:25Z

We'll integrate this for Heimdal 8.

nicowilliams · 2018-12-29T01:25:13Z

@bodik I've pushed some commits to your branch.

One merges hdb_prune_keys() and hdb_prune_keys_kvno(), and has a stub hdb_prune_keys() that calls hdb_prune_keys_kvno() (with kvno = 0) if the automatic key pruning feature is enabled.

Another renames all the prunekvno variables/arguments to just kvno.

Another one adds documentation.

Please review. We should squash all of these and merge.

lib/kadm5/prune_c.c

bodik · 2018-12-31T14:35:00Z

@bodik I've pushed some commits to your branch. ... Please review. We should squash all of these and merge.

@nicowilliams I've:

added two patches to make error handling in server and client as you requested. In the surrounding code there are not many examples so I tried to do my best. You have to review them and resolve corresponding conversations i've left open for you
I've refactored prunekvno to be optional argument of the prune command and tested that it works as expected (testsuite and manual testing done)
rebased onto current master and added krb5_enomem() usage on applicable places

once you review the changes I could squash commits and prepare branch for merging

nicowilliams · 2018-12-31T18:37:30Z

kadmin/prune.c

@@ -0,0 +1,35 @@
+/*
+ * TODO licence


So, this needs a copyright notice and license. Please pick something friendly to Heimdal, preferably this one:

* Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * * 1. Redistributions of source code must retain the above copyright * notice, this list of conditions and the following disclaimer. * * 2. Redistributions in binary form must reproduce the above copyright * notice, this list of conditions and the following disclaimer in the * documentation and/or other materials provided with the distribution. * * 3. Neither the name of the Institute nor the names of its contributors * may be used to endorse or promote products derived from this software * without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE INSTITUTE AND CONTRIBUTORS ``AS IS'' AND * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE * ARE DISCLAIMED. IN NO EVENT SHALL THE INSTITUTE OR CONTRIBUTORS BE LIABLE * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE.

(You can change point 3 to refer to yourself if you wish to retain the copyright.)

resolved (commit e973b7e)

Thank you! This is now ready. I'll squash and merge next.

nicowilliams · 2018-12-31T19:33:07Z

@bodik I've pushed to your branch a) a rebase-and-squash, b) one more cleanup commit (just using if (ret == 0) ... style). I'll squash that and push in an hour or so.

nicowilliams · 2018-12-31T20:17:33Z

Thanks @bodik! This is a very nice contribution.

nicowilliams self-assigned this Dec 14, 2018

jaltman requested a review from nicowilliams December 14, 2018 22:46

jaltman added the feature request label Dec 14, 2018

jaltman added this to the Heimdal 9 milestone Dec 14, 2018

nicowilliams reviewed Dec 26, 2018

View reviewed changes

kadmin/server.c Show resolved Hide resolved

nicowilliams reviewed Dec 26, 2018

View reviewed changes

lib/hdb/keys.c Outdated Show resolved Hide resolved

nicowilliams reviewed Dec 26, 2018

View reviewed changes

kadmin/server.c Show resolved Hide resolved

nicowilliams mentioned this pull request Dec 27, 2018

Make --keepold and prune-key-history enabled by default in Heimdal 8 #477

Closed

nicowilliams reviewed Dec 27, 2018

View reviewed changes

lib/hdb/keys.c Outdated Show resolved Hide resolved

nicowilliams reviewed Dec 27, 2018

View reviewed changes

kadmin/kadmin-commands.in Outdated Show resolved Hide resolved

nicowilliams modified the milestones: Heimdal 9, Heimdal 8 Dec 27, 2018

bodik commented Dec 31, 2018

View reviewed changes

lib/kadm5/prune_c.c Outdated Show resolved Hide resolved

nicowilliams reviewed Dec 31, 2018

View reviewed changes

kadmin selective prune of historic key for principal

875d58d

nicowilliams merged commit f3f06fc into heimdal:master Dec 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kadmin selective prune of historic key for principal #415

kadmin selective prune of historic key for principal #415

bodik commented Sep 13, 2018

nicowilliams Dec 26, 2018

bodik Dec 27, 2018

nicowilliams Dec 27, 2018

bodik Dec 27, 2018

bodik Dec 30, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

bodik commented Dec 27, 2018 •

edited

nicowilliams commented Dec 27, 2018 •

edited

bodik commented Dec 27, 2018 •

edited

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 29, 2018

bodik commented Dec 31, 2018

nicowilliams Dec 31, 2018

bodik Dec 31, 2018

nicowilliams Dec 31, 2018

nicowilliams commented Dec 31, 2018

nicowilliams commented Dec 31, 2018

kadmin selective prune of historic key for principal #415

kadmin selective prune of historic key for principal #415

Conversation

bodik commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

bodik commented Dec 27, 2018 • edited

nicowilliams commented Dec 27, 2018 • edited

bodik commented Dec 27, 2018 • edited

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 27, 2018

nicowilliams commented Dec 29, 2018

bodik commented Dec 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicowilliams commented Dec 31, 2018

nicowilliams commented Dec 31, 2018

bodik commented Dec 27, 2018 •

edited

nicowilliams commented Dec 27, 2018 •

edited

bodik commented Dec 27, 2018 •

edited