[RFC] kdb: store mkey list in context and permit NULL mkey for kdb_dbe_decrypt_key_data

[Greg Hudson]

For purposes of discussion, I want to call out the performance impact.
Currently, the caller of krb5_dbe_decrypt_key_data is required to
identify the correct master key using the entry's tl-data.
Unfortunately, that tl-data is not available to
krb5_dbe_decrypt_key_data given its current API, so it will try all
master keys in order.  During a master key transition, that will result
in some extra decrypt operations on a running KDC.

We can re-optimize this case by adding an optional argument to
krb5_dbe_decrypt_key_data for the full DB entry, and passing that
argument at all performance-sensitive call sites where we pass a null
mkey.  As I told Sam earlier, I am happy considering that
re-optimization to be future work, and think we might be able to fit it
into the margin for 1.9.

On a separate note, set_master_key_list and get_master_key_list should
be removed from the DAL table.  (The major version has already been
bumped for 1.9.)  I can take care of that, though.

On Sat, 2010-09-11 at 12:37 -0400, Sam Hartman wrote:
> In addition, the correct ideom for decrypting key data is too
> complicated and is unavailable to plugins that do not have access to
> the master key.

It's definitely too complicated, but I'll be picky and point out that
it's not entirely unavailable to plugins.  A plugin could call
krb5_db_get_mkey_list on the context to get the master key list, and can
even call fetch/set_mkey_list to refresh it.

> +/**
> + * Free a master keylist. The dal_handle holds onto the most recent master
> + * keylist that has been fetched throughout the lifetime of the context; if
> + * this function is called on that keylist, then the dal_handle is updated to
> + * indicate that the keylist should be freed on next call to
> + * krb5_db_fetch_mkey_list() or when the database is closed. Otherwise, the
> + * master_keylist is freed. Either way, the caller must not access this master
> + * keylist after calling this function.
> + */

These aliasing semantics are not caller-visible and should not be
documented here.

I am uncomfortable with the aliasing games in general, but I can see a
variety of reasons why copying the master key list gets messy.  I think
there should be brief comments in krb5_db_fetch_mkey_list and
krb5_db_free_mkey_list explaining their part of the aliasing game.

> +again: for (;n; n = n->next) {

I'm pretty strongly opposed to using "goto" except to a (unique) cleanup
handler.  We don't have an explicit coding practice prohibiting it, but
I think we should.  I would encourage the creation of a helper function
to hold the for loop.

I don't entirely understand the need for the status and error message
games here.