Do not loop on principal unknown errors #243
Conversation
|
I pushed a proposed rework which simplifies the code, although it's not quite as minimal of a change. Still needs an automated test and RT headers at a minimum. Tom was concerned that we would be changing the behavior on WRONG_REALM with an empty crealm if retry is true, so we need to examine that path more closely. I don't think there's any need to single out that particular error case and disable retries, but I'm also not certain what the intended purpose that code path is. |
|
Okay, more information. The origin of the "retry" intent can be found in 1.6, back when the AS client code was synchronous and we didn't support client referrals. In this version of the loop, any error other than PREAUTH_REQUIRED, coupled with e_data, is intended to result in another attempt with krb5_do_preauth_tryagain() used to generate potentially better preauthentication information. For instance, PKINIT can try again after decoding the KDC's list of trusted CAs. Release 1.7 added the actual "retry" variable, set by the FAST logic based on the presence of a cookie for a FAST error, and the presence of e-data for a non-FAST error. Release 1.7 also added client referral support. In this version of the loop, a WRONG_REALM with no crealm field bypasses the "retry" logic--but that's really an unimportant detail. A WRONG_REALM reply with no crealm and with e_data (or FAST error padata and a cookie) is not expected in practice, and there's nothing dangerous about passing such an error through the krb5_do_preauth_tryagain() step like we would have done in 1.6, and like we do for any random unhandled error code. So, I'm comfortable with the behavior of my current proposed patch. I will add a test case. |
|
I narrowed the is_referral() contract so that it doesn't take the krb5_init_creds_context "god object" as input, added RT headers and a comment about the retry behavior change, and added a test case. I don't like adding to t_general.py since it really shouldn't exist, but that's where we currently have kinit tests. This should be ready for review by the rest of the team. |
|
I agree with Greg's analysis of the retry case, though I haven't thoroughly analyzed what happens in the retry with ERR_WRONG_REALM case. It's probably not dangerous. The patch seems OK to me, but I think we should avoid long multi-line boolean expressions. Could we separate the err->error checks into an if-statement that returns early from is_referral? |
|
Okay, I changed is_referral() to break up the boolean expression. |
|
Thanks; that looks OK to me. |
If the canonicalize flag is set, the MIT KDC always return the client principal when KRB5_KDC_ERR_C_PRICIPAL_UNKNOWN is returned. Check that this is really a referral by testing that the returned client realm differs from the requested one. [ghudson@mit.edu: simplified and narrowed is_referral() contract. Note that a WRONG_REALM response with e-data or FAST error padata could now be passed through k5_preauth_tryagain() if it has an empty crealm or a crealm equal to the requested client realm. Such a response is unexpected in practice and there is nothing dangerous about handling it this way.] ticket: 8060 target_version: 1.13.1 tags: pullup
ticket: 8060
Simo's commit, not reviewed or edited.