[PATCH] Plugin Interface Change

[Nathaniel McCallum]

https://github.com/npmccallum/krb5-anonsvn/commits/verto

I believe I've addressed all the issues excepting:
  1. return_edata()
  2. lookaside cache

I plan on addressing these last two as soon as I return (10/10), if not
before.

Nathaniel

On Thu, 2011-09-15 at 12:31 -0400, Greg Hudson wrote:
> I've looked over all of the code and run it through the test suite.  I
> may have additional feedback regarding memory leaks if I find any.  For
> now, going from larger issues to smaller ones:
> 
> * The largest issue is with the use of KDC global variables indexed by
> kdc_active_realm.  In 1995 the KDC code was augmented to support
> multiple realms on the same port, but rather than modify all of the code
> to stop using global variables, realm-dependent globals were #defined in
> extern.h to be fields of kdc_active_realm.  If a preauth's verify method
> defers a reply until after the KDC has finished processing the packet,
> kdc_active_realm won't be set correctly in the responder function.  The
> most commonly used global is kdc_context, which implicitly contains the
> database state.
> 
> The best solution would be to start passing a realm parameter around to
> KDC functions and stop using the global variables.  A stopgap would be
> to record the realm pointer in state structures and resetting
> kdc_active_realm in responder functions.
> 
> * We will need an enhancement to the lookaside cache (replay.c) to
> support requests in progress.  A request should be added with no
> response as soon as it is received, and then its entry should be
> augmented to contain the response after we have one.  If a request
> arrives which has a lookaside cache entry with no response, we should
> drop the request (we'll respond to it later).  Without this enhancement,
> KDCs will start processing retransmitted requests, which is a
> regression.
> 
> * Patch 4 converts the verify function of KDC preauth plugins to use a
> responder.  I think the edata function needs to be converted as well;
> think 4-pass OTP where the username needs to be looked up in an external
> database in order to generate the challenge.  The return_padata function
> is another candidate for conversion, although it may be less important.
> Because we're going to be making the preauth plugin interface public in
> 1.10, it will be less elegant to do these conversions after the release.
> 
> * verify_enc_ts is not converted by patch 4, so encrypted timestamp
> verification is broken.
> 
> * Patch 3 casts the finish_preauth function pointer in process_as_req.
> This is not correct C (and is also very ugly since there's no typedef
> name for the callback type).  finish_preauth should take a void pointer
> argument and cast it internally.
> 
> * "misc" as a name component is content-free.  I realize there's not a
> lot you can say about the content of structures like that in the name,
> but "state" would at least convey that it's information needing to
> preserved between start and finish.
> 
> * Multiple exit labels can be a maintenance hazard.  If they're
> well-named they aren't too bad of a trap, although their necessity
> indicates a need to refactor the function.  schpw.c:dispatch (patch 1),
> process_tcp_response (patch 1), and finish_process_as_req (patch 3) use
> multiple exit labels unnecessarily.
> 
> * In patch 1, make_tcp_misc() is named as if it were a simple
> constructor but also takes responsibility for destroying ev.  Comments
> are definitely needed, and possibly a better name.
> 
> * We have several gratuitously different names for variables holding
> krb5_error_code values in our code base, which can become a maintenance
> irritant.  Let's not add a new one.  Use "code" instead of "ec".
> 
> * Don't put a space after cast operators
> (http://k5wiki.kerberos.org/wiki/Coding_style/Formatting#Horizontal_white_space).  Don't omit the space before the asterix in (type *) casts.
> 
> * There are no-op return statements at the end of
> process_packet_response (patch 1), process_packet (patch 1),
> kdc_verify_preauth (patch 4), and pkinit_server_verify_padata (patch 4)
> left over from when you were using the "return
> function_returning_void(...);" GCC extension.
> 
> * Patch 1 creates a long line in process_packet_response, and patch 3 in
> finish_process_as_req.
> 
>