Semantics for multiple pkinit_anchors/pkinit_pool lines

[Ken Hornstein]

>  preauth pkinit failed to initialize: PKINIT initialization failed:
>Cannot open file '...': No such file or directory
>
>and we'd lose that if errors were ignored.  The trace log (where you'd
>have to look on the client side) is pretty helpful with or without the
>error being ignored.

Fair enough; I think I was conflating other trace log messages with the
ones in this particular case.

>> Keep track of errors and make it so it won't error out if at least
>> one pkinit_anchors line works
>
>This is probably fine.  We'd still get our KDC log if there's one anchor
>location and it can't be loaded, and the trace log would still note
>which paths failed to load.

Just so I'm clear on the semantics ... obviously, you need at least one
working pkinit_anchors line.  Do you want to have at least one working
pkinit_pool line as well?  I'm fine with either and I can see arguments
for both ways.

This is a good segue into two other PKINIT/preauth related things that
have come up:

- We've found that the KRB5_TRACE functionality doesn't quite capture
  all of the points that the old pkiDebug() macro did, although it is
  a lot better than it was originally.  We have some code that would
  have allowed the user to conditionally turn on the pkiDebug messages
  and that turns out to be invaluable when trying to track down problems
  with PKINIT.  So, fine, seems pretty obvious that more KRB5_TRACE
  points would be helpful for everyone, but this brings up a style
  issue that I've never really understood.  It seems like the current
  style is you never call the TRACE macro directly, but you have to put
  in a macro like TRACE_PKINIT_SOME_MESSAGE() in pkinit_trace.h and then
  call the resulting macro in the code.  I'm curious about why the
  TRACE macro is never called directly (I guess it IS called directly
  once in Leash), but more importantly I am wondering if there are
  any unwritten style issues with the use of TRACE().

- It seems like nearly all preauth errors are flattened down to the
  error KRB5_PREAUTH_FAILED.  And that seems to happen in this code
  segment in preauth2.c:process_pa_data():

    if (must_preauth) {
        /* No real preauth types succeeded and we needed to preauthenticate. */
        if (save.code != 0) {
            ret = k5_restore_ctx_error(context, &save);
            k5_wrapmsg(context, ret, KRB5_PREAUTH_FAILED,
                       _("Pre-authentication failed"));
        }
        ret = KRB5_PREAUTH_FAILED;
    }

  So I understand that the "real" error is being appended to the error
  message, which is fine.  But this presents a problem in practice, as
  many programs don't display the error message string and just the
  output of com_err on the error code.  And the specific issue we're
  running into is if a user is using the SAM2 preauth mechanism and
  they enter the wrong password, instead of saying "Password incorrect",
  they get "Preauth failed", because the correct error IS returned by the
  preauth mechanism but never makes it up to the application.  And even
  if you DID print out the error message string, that's not quite helpful
  because the underlying error code returned is either going to be
  KRB5KRB_AP_ERR_BAD_INTEGRITY or KRB5KDC_ERR_PREAUTH_FAILED, and those
  messages would either be "Decrypt integrity check failed", or
  "Preauthentication failed".

  I have done enough stuff with the preauth code over the years to
  know that it is ... complicated.  So I could believe that returning
  the "correct" error code could cause some unexpected issues; I do
  not believe that it would, but I could be wrong.  I am wondering if
  there is a better way than just always returning KRB5_PREAUTH_FAILED,
  though.

--Ken