Difference between revisions of "Cleanups"

Latest revision as of 01:40, 29 July 2023

This page describes opportunities for internal code cleanups. Anything to do with a user-facing or developer-facing behavior should go elsewhere (perhaps in an RT ticket, or somewhere related to the roadmap).

• sam2_process (preauth_sam2.c) should use a cleanup label instead of repeated free invocations. It is also long and should be broken up into smaller pieces if possible. (See Manual Testing for guidance on how to test at least part of this function.)

• kadmind's main() should not have repeated blocks of calls to release resources. It's probably unnecessary to release resources on error exit (plenty of other programs don't bother).

• The build system used to have multiple configure.in files making use of an aclocal.m4. Now that we only have one configure.in, we don't really need a separate aclocal.m4.

• In the build system, some variables like INSTALL_PREFIX are just name variants of other variables and are only used in one or two places under the variant name.

• Account lockout decision logic is duplicated in the DB2 and LDAP KDB modules.

• The serialization framework has several levels of unnecessary nesting and should use a cleanup label rather than the repeated "if (!kret)" pattern.

• lib/krb5/os appears to have a couple of different functions to generate ADDRTYPE_ADDRPORT addresses (one in mkfaddr.c and another in full_ipaddr.c).

• klist and kinit (at least) still have some internal vestiges from when they supported krb4 and krb5 credentials, and could be simplified.

• The kdcpreauth pluggable interface uses the new plugin support functions, but does not have a proper consumer interface as described in Projects/Plugin_support_improvements#Consumer_interface.

• It's no longer our practice to use krb5_x() to make calls through function pointers, but it's still used in ktfns.c.

• The LDAP KDB module maintains a connection pool, but only ever uses one connection (since our KDC is single-threaded).

• lib/gssapi/mechglue should use the MIT krb5 style, not the Solaris coding style with tabs.

• lib/gssapi/generic contains some utility functions which are specific to the krb5 mech and should be located in lib/gssapi/krb5.

• Some parameters in osconf.hin are unused (like KDC_PORTNAME, KRB5_DEFAULT_ADMIN_ACL, and DEFAULT_ADMIN_ACL).

• kpropd's design can be simplified:
  • Currently, it forks to handle each incoming connection, but then waits for the child process. This could be done without forking, although doit() would have to be modified to avoid using exit() or an aborting SIGALRM handler.
  • Currently kpropd relies on dual-stack support to handle both IPv4 and IPv6 connections so that it can use a single blocking accept() call to wait for incoming connections. Some platforms such as OpenBSD don't have dual-stack support. We should open a listening socket for each address family and use libverto to wait for connections on both of them.
  • Currently, if iprop is enabled, a child process is used to handle incoming connections, but using libverto, the same process should be able to handle both. The process wouldn't ask for iprop updates while processing a kprop connection (since that handling is synchronous), but that should be okay.

• The libkdb5 interface includes krb5_db_setup_lib_handle(), which has no caller-visible semantics (it is invoked internally when needed). That function can be removed from the interface, since we don't guarantee interface stability for that library.

• krb5_db_get_age() and its associated DAL interface can be removed.

• The krb5_db_entry "len" field has no caller-useful semantics. krb5_db_put_principal will abort with the DB2 back end if the len field is not set to the magic value KRB5_KDB_V1_BASE_LENGTH. This field and the associated constant can be removed.

• The krb5_db_entry "n_tl_data" field is redundant with the length of the tl_data linked list, and isn't likely to have a significant performance impact since tl_data lists are short. It can be removed.

• The constants KRB5_TL_KADM5_E_DATA, KRB5_TL_RB1_CHALLENGE, KRB5_TL_USER_CERTIFICATE, KRB5_TL_LM_KEY, and KRB5_TL_X509_SUBJECT_ISSUER_NAME are unused and can likely be removed.

• The osa_policy_ent_rec "version" field has no caller-visible semantics; it is used internally by the DB2 module's xdr_osa_policy_ent_rec(), but a local variable would serve. The field can be removed.

• adb_openclose.c maintains a static linked list mapping filenames to lock structures, in an attempt to work around bad POSIX file locking semantics. This list serves no useful purpose because there is no equivalent code for the main DB2 lockfile, and OFD locks (where they are available) are a much better solution. The list can be removed and the locking code simplified.

• krb5_db_iter_policy takes a match_expr parameter and passes it through to the back end, but both back ends ignore it. The parameter can be removed.

• add_to_history() in svr_principal.c stores password history entries in a circular queue, which is unnecessarily complicated because the data structure is serialized after every password change. We can simplify this code by always generating a list in sorted order, although we must remain bidirectionally compatible with the existing code. We can either sort the entries in the structure we read in, or we can use the old_key_next value to identify which nhist-2 entries to preserve.

• alt_prof.c contains functions for opening krb5.conf and kdc.conf, and for translating string, boolean, deltat, and integer values. These are largely redundant with normal profile handling functions. (One difference: alt_prof.c selects the last relation for a single-valued field, while regular profile functions select the first one. This is probably ignorable.)

• krb5_dbe_fetch_act_key_list is used only from kadmind, which immediately calls krb5_dbe_find_act_mkey and frees the list. This API could be replaced with an API to retrieve the active master key and its kvno, so that kadmind does not need to know about the existence of the master key activation list. The new API could also be used in kdb5_util's kdb5_update_princ_encryption in place of the three calls it uses to get the active master key.

• krb5_free_cred_enc_part does not obey the usual contract, as it frees only the contents of the container passed in, not the container itself. It is not a public API, so its behavior can be changed as long as all of the in-tree callers are adjusted to match.

• krb5_rd_req_decoded and krb5_rd_req_decoded_anyflags have unnecessary output parameters which receive a copy of the ticket. The ticket (with decrypted enc_part if we could decrypt it) is in req->ticket, so the caller does not need a copy. The following cleanups could simplify the code and slightly improve performance:
  • krb5_rd_req could pass a NULL ticket argument, and instead steal the ticket pointer before freeing request.
  • kg_accept_krb5 could pass a NULL ticket argument, and instead refer to request->ticket (possibly via an alias pointer).
  • kdc_rd_ap_req could have its ticket parameter removed. kdc_process_tgs_req could refer to apreq->ticket and could steal that pointer for its own ticket output parameter before freeing apreq.
  • Since krb5_rd_req_decoded/krb5_rd_req_decoded_anyflags are no longer public interfaces (they were purged in the great 1.2.2 cleanup, and their prototypes are completely gone from krb5.h since 1.7), we could rename them and remove their ticket output parameters.

• pkinit_crypto_openssl.h does not need to be a header file. Its contents can be part of pkinit_crypto_openssl.c.

• pkinit_identity_crypto_context contains a stack of certs (my_certs) and an index (cert_index), but the stack only ever contains one cert and the index is always 0.

• pkinit_identity_crypto_context contains a cert collection with a different lifetime from the object itself, managed by crypto_load_certs() and crypto_free_cert_info(). The collection is only used briefly by pkinit_identity_initialize() and pkinit_identity_prompt(). It would be better represented with a separate object type, whose lifetime is managed via local variables in the pkinit_identity functions.

• The PKINIT KDC and PKINIT client share a number of identity requirements such as anchors, intermediates, and CRLs, but they have rather different logic for certificates. The client can select from a collection of certs and may need to prompt for passwords in order to access private keys. The KDC never prompts for a password and has just one cert. The KDC code path would be easier to understand if it did not go through all of the same identity initialization interface as the client, with the anchor/intermediate/CRL implementation shared between the two via a helper function.

• If k5_vset_error allowed a null ep parameter and did nothing (like krb5_set_error_message allows a null context), then callers of the plugins.c functions could be simplified when they don't want extended error information, as in the g_initialize.c macros.

• The libprofile iterator logic around skip_num is not very intuitive and may not always be correct. iter->num counts up when a match is returned--that is, deleted nodes are not considered. If iter->num is used for skip_num because the file generation has changed, it counts down in the search loop *before* deleted nodes are ignored. This behavior is required for the first test in test_prof1 which deletes all nodes in an iterator, because the first call to profile_update_relation changes the file generation and also deletes the first matching node.

• libprofile's prof_tree.c contains several copies of loops to search for a name in a node list, and its prof_set.c contains several copies of loops to look up parent sections. These could be factored out; however, the skip_num logic in profile_node_iterator currently prevents factoring out that copy of the search loop unless skip_num becomes a parameter of the factored-out helper function.

• lib/kadm5/srv/server_acl.c contains many opportunities for cleanup:
  • The acl_*_msg string variables should be removed and their values inlined into the log statements.
  • The ae_ prefix on acl_entry field names is unnecessary.
  • The kadm5int_acl_ prefix on static functions is unnecessary.
  • The name, target, and restrictions strings could be evaluated immediately at parsing time.
  • A lot of code could be de-indented.
  • The ae_name_bad, ae_target_bad, and ae_restrictions_bad fields are redundant, as setting any of them results in setting ae_name_bad which disables the entry. They may wind up all being unnecessary if we stop doing lazy evaluation of those strings.
  • The structure of kadm5int_acl_parse_restrictions makes it difficult to identify which part of the restrictions string is invalid.
  • The file uses K&R-style function definitions.
  • Several functions operate multiple times on their output arguments, contrary to current practice.

• kadm5_policy_t is a typedef for char *, representing a policy name. It is used in two prototypes in admin.h but not elsewhere. It should be replaced with const char * in those prototypes, and the typedef should be commented as being deprecated.

• krb5_string_to_deltat() is implemented in libkrb5 using a bison grammar, which adds to the complexity of our repository and build system. It could be rewritten in C with no loss of clarity, as only a few interval formats are supported.

• The library initialization support in k5-platform.h uses different strategies on Windows and POSIX platforms. Windows (as of Vista and Server 2008) supports a function InitOnceExecuteOnce() which works like pthread_once(). Using that function we could make a Windows version of k5_once() and use the same library initialization strategy on all platforms. Library finalization support would remain conditionalized by platform.

• It is also possible to emulate static mutex initialization on Windows, using a wrapper lock function which does InterlockedCompareExchangePointer(), as detailed here. With static mutex initializers we might not need as many library initializers, although we would still need library finalizers to clean up any initialized mutexes.