Projects/Database Access Layer cleanup

An announcement has been sent to krbdev@mit.edu starting a review of this project. That review will conclude on July 9, 2010.

Comments can be sent to krbdev@mit.edu.

This project page describes ideas and a prioritized plan for cleaning up the KDB Database Access Layer (DAL).

Analysis of 1.8 DAL

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. To avoid decrypting all key entries on every lookup, all accesses to key data would need to go through the DAL--in a sense, this is already the case because of the need to call dbekd_decrypt_key_data, but the accessor interface could be simplified by removing the need to pass in the correct master key.

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.

The memory allocation barrier

Particularly on Windows, it is possible for different build environments to use different malloc/free implementations. Because of this, there is some attention paid in Kerberos to making sure that memory is allocated and freed on the same side of any public interface, including plugin provider interfaces.

The DAL pays attention to this requirement to some degree. Principally, the krb5_db_alloc() and krb5_db_free() functions can be used to allocate and free memory within the KDB module; this is mainly used by libkadm5srv when it wants to create or modify a krb5_db_entry which will later be freed by the KDB module's free_principal method. Using krb5_db_alloc can be cumbersome; for instance, there is a 50+ line function in libkadm5srv to copy a principal into storage allocated by the DB module.

There are numerous violations of the memory allocation barrier, some inherent in the DAL interfaces and some due to implementation sloppiness. Because the DAL is not currently used under Windows, these violations are not discovered in testing and create minimal pain, but do render moot the efforts made by other pieces of code to obey the memory allocation barrier as it applies to the DAL.

This problem can be addressed in either direction. The DAL can be declared not to be a memory allocation barrier, in which case krb5_db_alloc and krb5_db_free can go away (and some code can be simplified), or all of the violations of the barrier can be fixed.

A partial list of violations follows:

• fetch_master_key_list allocates memory inside the DB module, which is later freed by callers outside of the DB module.

• The CHECK_POLICY_AS and CHECK_POLICY_TGS methods of db_invoke allocate memory for status inside the DB module; it is freed by the KDC.

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.

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. (On failure, the same "temporary" argument instructs destroy to remove the temporary copy of the DB.) Specifying a temporary DB should ideally 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.

Prioritization

The following plan gives a prioritized list of cleanup proposals; the goal is to implement as many of them as possible for 1.9, realizing that the ones near the end of the list may be deferred. When changes are proposed to DAL interfaces, assume that the corresponding krb5_db interface will also be changed except when otherwise noted.

Safety Mechanisms

Define a constant in kdb.h which is the YYYYMMDD date of the most recent incompatible modification to the DAL vtable. Pass this constant to the plugin's init_library function. The plugin can compare the value of this constant to the one it was compiled with, and detect whether it is out of date. If the plugin returns an error, the DB module will not be used. (It would be possible to alter the initialization mechanism so that a plugin could support multiple versions of the KDC. Such a mechanism will be deferred until the more general plugin framework plan is better defined.)

Define a KDB API version in kdb.h to allow external users of libkdb5 to conditionalize across incompatible changes to the API. This version is intended to be in sync with the library ABI version number, so will start at 5.

Simple Cleanups

• Remove the following interfaces, which are either unused or not productively used:
  • db_supported_realms
  • db_free_supported_realms
  • errcode_2_string
  • get_master_key
  • release_errcode_string
  • setup_master_key_name
  • set_master_key
  • set_option

• Rename the following library APIs:
  • krb5_dbekd_decrypt_key_data -> krb5_dbe_decrypt_key_data
  • krb5_dbekd_encrypt_key_data -> krb5_dbe_encrypt_key_data

• Make the following library APIs return void:
  • krb5_db_free_principal
  • krb5_db_free_mkey_list

• Remove the db_ or similar prefix from all interfaces in the DAL (but leave it in the library APIs).

• Remove verify_master_key, updating kdb5_util mkey_convert to use krb5_db_fetch_mkey_list instead.

• Remove store_master_key, and implement krb5_db_store_master_key in terms of store_master_key_list.

• Change all uses of KRB5_KDB_DBTYPE_NOSUP to KRB5_PLUGIN_OP_NOTSUPP.

• Return KRB5_KDB_DBTYPE_NOSUP from krb5_db_iterate if the module does not implement db_iterate.

• Remove the default implementations of get_master_key_list, set_master_key_list, and promote_db, and make the KDB APIs for those functions return KRB5_PLUGIN_OP_NOTSUPP if the module does not implement them.

• Remove assertions in kdb5.c for init_module and fini_module being non-null.

Principal and Policy API Changes

• Remove the nentries and more parameters from get_principal, and make it return an error if the principal does not exist.

• Remove the library API krb5_db_get_principal_ext, and add its flags argument to krb5_db_get_principal.

• Remove the count parameter from free_principal.

• Remove the nentries parameter from put_principal.

• Make get_principal and free_principal responsible for allocating the krb5_db_entry container (so get_principal's output argument would gain an extra level of indirection). This allows for more robust cleanup and is consistent with get_policy.

• Remove the nentries parameter from delete_principal, and make it return an error if the principal did not exist.

• Remove checks for nullity of get_principal and free_principal, as they are mandatory for KDC operation.

• Remove the cnt parameter from get_policy, and make it return an error if the policy did not exist.

• Define a flag KRB5_KDB_ALIAS_OK for use with get_principal. Set it for TGS requests, for AS requests when the canonicalize flag was requested, and for requests via libkadm5.

Split db_invoke

Replace db_invoke and its associated structures with the following interfaces:

• sign_auth_data
• check_transited_realms
• check_policy_as
• audit_as
• audit_tgs
• refresh_policy
• check_allowed_to_delegate

Master Key Redesign (separate project)

Redesign the master key interfaces. The precise design is deferred to a separate project writeup, with the following desirables:

• Modules which don't use master key encryption can simply decline to implement the relevant interfaces, rather than having to fake them out.

• Memory allocated by fetch_master_key_list (or equivalent) should be freed inside the module, not by the caller.

• Eliminate set_master_key_list and get_master_key_list, making the caller responsible for caching those.

DB Entry Redesign (separate project)

Redesign the krb5_db_entry structure to be independent of its representation within the DB2 module. The precise design is deferred to a separate project writeup, with the following desirables:

• Eliminate e_length, e_data, and tl_data, replacing them with the logical elements currently stored using tl_data.

• Provide a well-defined place for module-specific entry data.

• Abstract key data accesses through a functional interface, to better handle AD-style modules which store the password rather than a list of keys.

Separate KDC and Admin Interfaces (separate project)

Subject to further review, define separate plugin interfaces for the KDC and kadmin, allowing bridge-style modules to provide a more narrow set of interfaces. The precise design is deferred to a separate project.

Review

This section documents the review of the project according to Project policy. It is divided into multiple sections. First, approvals should be listed. To list an approval type

#~~~~

(hash mark followed by four tilde characters) on its own line. The next section is for summarizing discussion, which should take place on krbdev@mit.edu. Provide links to the archive at http://mailman.mit.edu/pipermail/krbdev/ if appropriate. Blocking objections can be noted with {{project-block}}.