[PATCH] Plugin Interface Change

[Greg Hudson]

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.