Semantics for multiple pkinit_anchors/pkinit_pool lines

[Greg Hudson]

On 1/28/21 10:50 AM, Ken Hornstein wrote:
> 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?

Yes, if pkinit_pool is specified.

>   So, fine, seems pretty obvious that more KRB5_TRACE
>   points would be helpful for everyone

Sure.

>   I'm curious about why the
>   TRACE macro is never called directly (I guess it IS called directly
>   once in Leash)

When the trace logging system was designed, there was some theoretical
interest in integrating it with DTrace.  Centralizing the trace points
could facilitate this; an OS packager could perhaps change all of the
macros in k5-trace.h and pkinit-trace.h to call DTrace functions.  I
don't think anyone has taken advantage of that, though.

> I am wondering if there are
>   any unwritten style issues with the use of TRACE().

See the comments in include/k5-trace.h.  The note about not tracing
before an unrecoverable errors is less applicable to PKINIT, because
errors from PKINIT are often obscured by the preauth framework.

> - It seems like nearly all preauth errors are flattened down to the
>   error KRB5_PREAUTH_FAILED.

Or KRB5KDC_ERR_PREAUTH_FAILED if they come from the KDC.  SAM-2 is a
little unusual among preauth mechanisms because the password being wrong
is detected on the client side (it has this in common with the
no-preauth scenario, where a wrong password leads to a
KRB5KRB_AP_ERR_BAD_INTEGRITY error when trying to decrypt the KDC reply).

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

Our programs or out-of-tree programs?

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

kinit specifically looks for KRB5KRB_AP_ERR_BAD_INTEGRITY and
KRB5KDC_ERR_PREAUTH_FAILED after prompting for a password (see kinit.c
line 810).  The second check could probably be extended to cover
KRB5_PREAUTH_FAILED as well.

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

There's a bit of history here:

* In commit 632260bd1fccfb420f0827b59c85c329203eafc9 (ticket 7517) we
started doing what you proposed, returning the error code from the first
failed non-informational preauth mech.

* In commit 560e11dabb63b141df29c54aaa2e120309a1e021 (ticket 8457) we
switched to wrapping with KRB5_PREAUTH_FAILED, because the S4U2Self code
expected to see that error code during realm identification.

* In commit 9d6847c8f0187a0dd6466420335b5460de5c449e we added a better
internal API for realm identification, removing the consideration that
led to the wrapping.

So we could perhaps return to the ticket 7517 behavior.  But for the
specific SAM-2 problem you describe, I think it would be better for
krb5_get_init_creds_password() to recognize password-incorrect errors
using the logic currently in kinit (extended to include client-side
preauth errors).  There is currently no com_err code for "password
incorrect", so we'd have to add one to k5e1_err.et.