Cleanups - Revision history

Ghudson at 06:40, 29 July 2023

2023-07-29T06:40:09Z

Ghudson at 19:47, 25 February 2018

2018-02-25T19:47:03Z

Ghudson at 14:26, 25 February 2018

2018-02-25T14:26:54Z

Ghudson at 19:41, 28 June 2017

2017-06-28T19:41:09Z

Ghudson at 19:12, 26 June 2017

2017-06-26T19:12:27Z

Ghudson at 20:00, 7 August 2016

2016-08-07T20:00:51Z

Ghudson at 19:54, 7 August 2016

2016-08-07T19:54:31Z

Ghudson at 22:30, 24 June 2016

2016-06-24T22:30:29Z

Ghudson at 17:28, 19 April 2016

2016-04-19T17:28:26Z

Ghudson at 19:09, 2 February 2016

2016-02-02T19:09:22Z

@@ Line 4: / Line 4: @@
 * 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.
-* Not all variables in the build system's Makefile fragments (like pre.in) are used.  For instance, SHLIB_TAIL_COMP and KRB5_SHLIBDIR appear to be unused.  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.
+* 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.
 * 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).
@@ Line 21: / Line 19: @@
 * 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]].
 * 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.
@@ Line 39: / Line 27: @@
 * 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 and KDC_SECONDARY_PORTNAME), or appear to be used but are never really used (like KRB5_DEFAULT_ADMIN_ACL and DEFAULT_ADMIN_ACL).
+* Some parameters in osconf.hin are unused (like KDC_PORTNAME, KRB5_DEFAULT_ADMIN_ACL, and DEFAULT_ADMIN_ACL).
 * kpropd's design can be simplified:
@@ Line 62: / Line 50: @@
 * 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 unnecssarily 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.
+* 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.
@@ Line 86: / Line 70: @@
 * 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 KDC 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.
+* 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.

← Older revision		Revision as of 19:47, 25 February 2018
Line 81:		Line 81:

	* pkinit_crypto_openssl.h does not need to be a header file. Its contents can be part of pkinit_crypto_openssl.c.		* 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.		* 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.

← Older revision		Revision as of 14:26, 25 February 2018
Line 81:		Line 81:

	* pkinit_crypto_openssl.h does not need to be a header file. Its contents can be part of pkinit_crypto_openssl.c.		* 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 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 KDC 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.		* 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.

← Older revision		Revision as of 19:41, 28 June 2017
Line 30:		Line 30:

	* The LDAP KDB module maintains a connection pool, but only ever uses one connection (since our KDC is single-threaded).		* The LDAP KDB module maintains a connection pool, but only ever uses one connection (since our KDC is single-threaded).
−
−	* The LDAP KDB module has a server handle field named server_info_update_pending which is never set to anything but false. The code that would use it in ldap_handle.c is conditional on ASYNC_BIND, which we never define.

	* krb5_rc_default and krb5_rc_default_name are not part of our public API and are not used in the tree.		* krb5_rc_default and krb5_rc_default_name are not part of our public API and are not used in the tree.

← Older revision		Revision as of 19:12, 26 June 2017
Line 30:		Line 30:

	* The LDAP KDB module maintains a connection pool, but only ever uses one connection (since our KDC is single-threaded).		* The LDAP KDB module maintains a connection pool, but only ever uses one connection (since our KDC is single-threaded).
		+
		+	* The LDAP KDB module has a server handle field named server_info_update_pending which is never set to anything but false. The code that would use it in ldap_handle.c is conditional on ASYNC_BIND, which we never define.

	* krb5_rc_default and krb5_rc_default_name are not part of our public API and are not used in the tree.		* krb5_rc_default and krb5_rc_default_name are not part of our public API and are not used in the tree.

← Older revision		Revision as of 20:00, 7 August 2016
Line 104:		Line 104:

	* 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.		* 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 [http://stackoverflow.com/questions/3555859/is-it-possible-to-do-static-initialization-of-mutexes-in-windows 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.

← Older revision		Revision as of 19:54, 7 August 2016
Line 102:		Line 102:

	* 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.		* 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.

← Older revision		Revision as of 22:30, 24 June 2016
Line 57:		Line 57:

	* 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.		* 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.		* 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.

← Older revision		Revision as of 17:28, 19 April 2016
Line 98:		Line 98:

	* 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.		* 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.

← Older revision		Revision as of 19:09, 2 February 2016
Line 59:		Line 59:

	* 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.		* 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 unnecssarily 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.)		* 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.)