Services4User review

[Greg Hudson]

I have made a first pass over about half of the changes on your user
branch.  I will need to do a more comprehensive second pass after doing
some more background reading (I read the Microsoft S4U specification,
but there are limitations to my GSSAPI understanding, and I also simply
need to block out more time).  But since you're going to be doing a
bunch of ongoing krb5 work, I want to bring up some coding practices
issues.  Some of this is nitpicky.

* Do not make lines overflowing 79 columns if possible.  Examples:
  - kdc_util.c line 923
  - kdc_process_s4u2self_rep (multiple cases)
  - kdc_process_s4u2self_req (multiple cases)
  - process_tgs_req
  - do_tgs_req.c line 701
  - kvno.c line 42 (text output also overflows; previously bad)
  - kvno.c line 94 (can use ANSI C string concatenation)
  - kvno.c line 283 (previously bad)
  - kvno.c line 288, 303
  - t_s4u.c line 53
  - s4u_gss_glue.c line 134, 292
  - g_acquire_cred_imp_cred.c line 190, 191

* Do not put parens around return expressions:
  - g_acquire_cred_imp_cred.c
  - g_acquire_cred_imp_name.c
  (These may be the result of copying existing code.)

* Code documentation was good in many places, but I noted some holes:
  - verify_s4u_x509_user_checksum should explain why it's trying two
ways.
  - kdc_process_s4u2self_rep is a bad name.  "kdc_process_*" was
previously used only for examining request data, and is a bad name
prefix even then.  In this case you are composing reply data.  Also,
this function should have a leading comment explaining what it does,
unless you can come up with a name and parameter names which are super
self-explanatory.
  - krb5int_send_tgs grew a new callback parameter; its meaning should
be documented in a comment before the function definition in send_tgs.c

Part of the issue here is that we do not commonly document internal
function contracts, and would like to change that.  It's a huge job to
fix this for all of our existing code, but we should at least document
new functions we add when they're not trivially obvious.

* Do not cast the return value of malloc; examples:
  - kdc_process_s4u2self_rep
  - kdc_process_s4u2self_req
  - add_pa_data_element
  - kg_compose_deleg_cred
  - g_acquire_cred_imp_cred.c
  - gss_acquire_cred_impersonate_name
  - gss_add_cred_impersonate_name

* Do not void-cast return values which are almost always ignored.  (This
is specified in the coding practices document on the wiki, but is I
believe the general practice in krb5 code, and is also a good
readability practice in my opinion.)  I didn't collect examples here,
but I saw void casts for memset, memcpy, and the gss_release functions.
For the purposes of static analysis, I think Coverity has a good
approach here: it will only complain about the lack of a void cast when
it can statistically determine that the return value is usually used.

* Do not add space between sizeof and the following open paren.  I
didn't collect examples here.

* Do not check for nullity before calling free or krb5_free functions.
(I don't think this is in the coding practices document but is what
we're moving towards.)  Examples:
  - kdc_process_s4u2self_rep cleanup
  - (do_tgs_req.c line 988 but mixed in with others)
  - do_v5_kvno cleanup
  - init_sec_context.c:get_credentials cleanup
  - kg_compose_deleg_cred cleanup
  - gss_add_cred_impersonate_cred cleanup
  - gss_add_cred_impersonate_name cleanup

* In kdc_process_s4u2self_req:
  - Use a cleanup handler, not "free everything and exit" flow control
  - Initialize *s4u_x509_user to NULL at start, use a temporary during
    construction, and ssign the temporary to the output value just
    before returning success

* Trailing blank lines added to some files; examples:
  - kdc_preauth.c
  - krb5_gss_glue.c
  - acquire_cred.c

* Unrelated #if 0s added to ksetpwd.c.

* We are trying to avoid large functions which are hard to see on a
screenful of text or understand within your head.  This means if you're
writing a large function, or adding to a moderately-sized or large
function, you should look for opportunities to naturally break things up
into helper functions.  Some places where I noted opportunities for this
include:
  - kdc_process_s4u2self_req, pa_type == KRB5_PADATA_S4U_X509_USER
    conditional block
  - kdc_process_s4u2self_req, is_local_principal conditional block
  - init_sec_context.c:get_credentials, new proxy_cred conditional

* In krb5_gss_cred_id_rec you introduce some bitfields where none were
present.  I don't know if it is important to contain the size of id_rec
structures, and using bitfields can expand the size of the code which
processes those fields, which at times can be more undesirable than
allocating a bit more memory in an infrequently-used structure.  I am
not sure how the balance comes out in this case, but please be aware of
the tradeoff.