Difference between revisions of "Projects/Database Access Layer cleanup"

Revision as of 14:16, 2 February 2010

This project page describes ideas for cleaning up the KDB Database Access Layer (DAL). No resources or schedule have been assigned to this project yet. It is likely that this project will be narrowed or split out into sub-projects before implementation.

Master key encryption

DB2 and LDAP encrypt key entries in a master key, which is obtained either through password entry at KDC/kadmind startup time or from a stash file. Encryption and decryption of key data is currently performed explicitly in the KDC and libkadm5.

It is possible to construct a database plugin which does not encrypt key data in a master key, but it is hackish: the plugin overrides the default fetch_master_key function to return a dummy key, and overrides the default dbekd_encrypt_key_data and dbekd_decrypt_key_data functions to perform no encryption or decryption.

It would perhaps be cleaner if key encryption and decryption were performed underneath the DAL. Since the user interface features associated with master keys (kdb5_util functions and the command-line options to krb5kdc and kadmind for keyboard entry of the master password) necessarily exist above the DAL, we would still need entry points to implement those features.

There are opportunities for functional improvements to master keys, which should be noted even if they are out of scope for a DAL cleanup effort. Master keys are a prime candidate for storage on external tokens; we should make it easier to generate them randomly; and in some environments it may make sense to generate the master key randomly and then encrypt it in a keyboard-entered password (combining "what you have" and "what you know" for access to the KDB key data).

If a comprehensive redesign of master key encryption is not performed, there is plenty of opportunity for minor cleanups to the master key functions:

• get_master_key is unused, and its default implementation is invalid. set_master_key is used in the KDC and kadmind, but not productively, and the two modules are inconsistent about whether they copy or alias the provided memory.

• The KDC and kadmind retrieve the master key list from the database using fetch_master_key_list, and then feed that value back into the module with set_master_key_list, so that the module can feed it back out to the KDB keytab via get_master_key_list. This seems unnecessarily complicated; the get and set functions can probably be excised from the DAL. The default implementation of get_master_key_list is invalid.

• store_master_key has a password argument which is not used in the default implementation; this is probably okay because a DB module might want access to it. store_master_key_list also has a password argument, which makes little sense as the key list did not necessarily all derive from the same password.

• verify_master_key only verifies against the most current master key entry; its functionality is pretty much subsumed by fetch_master_key_list. Most of its call sites were ifdef'd out during the master key migration project, except for kdb5_util dump -mkey_convert, which should perhaps be updated.

• fetch_master_key_list allocates memory inside the database module, which is then freed by the caller. This is inconsistent with the design of other DAL functions which take care to allocate and free memory on the same side of the DAL.

DB entry format

The current DB entry structure contains fields specific to the DB2 back end. The tl_data field in particular was introduced in order to make the DB2 back end extensible, and is not a very natural way to access the information outside of that back end. Moreover, some back ends may not contain information in a format which maps naturally to the DB entry fields; for instance, an AD-style back end could contain user passwords rather than enctype-specific key data.

An ambitious goal here might be to make the DB entry completely opaque from the perspective of the libkdb5 consumer, and use accessor functions (provided by the DAL) to retrieve and modify all entries. The back end would allocate and define the layout of the entire structure. A compromise might expose some of the structure but leave the rest to the back end.

A less ambitious, but still work-intensive goal might be to remove the tl_data field and instead add specific fields for the information contained therein, making the back end responsible for encoding and decoding that data as tl_data when storing and reading entries.

Jump points (db_set_option and db_invoke)

The functions db_set_option and db_invoke accept a method number and arguments (a void * for db_set_option, and a pair of krb5_data objects for db_invoke) whose interpretation depends on the method number. They each serve a variety of unrelated purposes. There is no good reason to combine multiple operations into a single vtable entry this way.

db_set_option is unused and should be removed. db_invoke should be split out into separate, type-safe interfaces.

krb5_key_data

The krb5_key_data structure is very representation-oriented. It contains a "version" (separate from the kvno) which indicates whether or not there is an explicit salt associated with the key, and then the type/length/contents pointers are arrays of two elements, the second of which determines the explicit salt. This structure should be redefined in a more logical fashion, and the conversion to the format used by the DB2 back end should be performed underneath the DAL.

Alias-related flags

The flags field for db_get_principal does not explicitly indicate whether aliases are okay. (They should be allowed for TGS requests, and for AS requests if the client indicates a willingness to accept them.) Currently the LDAP back end uses a combination of CLIENT_REFERRALS_ONLY and CANONICALIZE to decide whether to return aliases. There should be an explicit flag.

Separation of KDC and admin DAL interfaces

Currently the KDC and libkadm5srv libraries use the same set of interfaces, each requiring overlapping subsets of the functions. It might be cleaner to have separate vtables for the KDC and for administrative access. This would make the required functionality clearer for modules which only wish to target the KDC, and would also clean up situations where functionality in the overlapping functions (like db_get_principal) varies subtly between KDC access and administrative access scenarios.

Miscellaneous cleanup

The db_get_principal function takes "nentries" and "more" parameters which are never productively used. A principal name can only have one associated principal. The db_get_policy function has a similar argument "cnt" which makes equally little sense.

The db_put_principal function takes a list of krb5_db_entry structures. We never use it to put more than one principal, and we have no expectation of needing transactional updates across principals. It should take only a single krb5_db_entry.

Some krb5_db functions are never used, such as krb5_db_set_option. Since we have no compatibility promises for libkdb across releases, these should be inventoried and removed.

There are two different "not supported" error codes used by krb5_db. The LDAP plugin mostly returns KRB5_PLUGIN_OP_NOTSUPP (except in db_invoke), while kdb5.h and hdb use KRB5_KDB_DBTYPE_NOSUP. kdb5_util dump expects to see KRB5_PLUGIN_OP_NOTSUPP from a database which does not support locks. These error codes should be collapsed into one.

There appears to be no precise rule as to whether kdb5.c will check for the nullity of a function pointer, assert non-nullity, or simply use it without checking. At a minimum, kdb5.c should be consistent about whether to assert non-nullity before using a mandatory function, and certain obviously mandatory functions like db_get_principal should not be treated as optional.

krb5_db_iterate returns success if the module does not support iteration. This is not a correct answer; it should return an error instead. (Existing modules all support iteration, so this would not be a functional change for current modules.)

errcode_2_string is not productively used. In db2 it is not implemented; in LDAP it has the overall effect of calling krb5_get_error_message on the error code and then krb5_set_error_message on the result, which is a complicated no-op. This function and release_errcode_string should be removed.

The purpose of krb5_db_alloc and krb5_db_free is unclear. All current modules implement it with realloc() and free(). There are a handful of callers in libkadm5srv and libkdb. It is possible that this is a workaround for an issue involving different implementations of realloc/free being used by libkdb5 and plugin shared objects, but that would only come into play if memory allocated by libkdb5 or libkadm5srv is being freed by the plugin, which suggests a layering violation of some kind.

db_change_pwd should be named dbe_change_pwd for consistency with other functions operating on in-memory database entries as opposed to the database itself.

kdb5_util specifies a db_args value of "temporary" to instruct the back end to open a side copy of the database, and then calls promote_db to make the database live when the load is complete. Specifying a temporary DB should be accomplished via a symbolic parameter, not via invasion of the db_args namespace. Furthermore, there should be a programmatic way to determine if the module does not support opening a temporary DB, so that kdb5_util can supply a good error message on its own. Currently the LDAP back end returns EINVAL and supplies an error message written with knowledge of the kdb5_util UI.