Rework _kdc_find_etype()
#947
Conversation
nicowilliams
force-pushed
the
rework-_kdc_find_etype
branch
from
6880d96
to
b362bcc
Compare
| weak_offered = 1; | ||
|
|
||
| for (i = 0; i < ninner; i++) { | ||
| if (--limit == 0) { |
|
|
nicowilliams
force-pushed
the
rework-_kdc_find_etype
branch
from
b362bcc
to
246b3c4
Compare
| default: | ||
| for (i = 0; i < _krb5_num_etypes; i++) | ||
| if (_krb5_etypes[i]->type == type) | ||
| return _krb5_etypes[i]; |
There was a problem hiding this comment.
Hmm, isn't the compiler going to optimise to this anyway? Is _krb5_etypes local to crypto.c? Seems a bit premature optimisation-ish...
There was a problem hiding this comment.
Why would it optimize this?
The problem is this is an O(N) algorithm invoked from loops elsewhere.
There was a problem hiding this comment.
Can’t it unroll the etype list?
There was a problem hiding this comment.
I guess a compiler could turn the loop into a switch, yeah. I will examine the dissassembly, though I'm not sure I want to assume a compiler this smart.
nicowilliams
force-pushed
the
rework-_kdc_find_etype
branch
from
246b3c4
to
7816ec9
Compare
| * NOTE: This is either all strong enctypes only, or all enctypes, | ||
| * including weak ones, depending on configuration. | ||
| */ | ||
| cached_kerberos_enctypes = krb5_kerberos_enctypes(r->context); |
There was a problem hiding this comment.
Hmm, introduced global shared state when we've been trying to refactor everything to use r. Can you cache these in the context instead?
There was a problem hiding this comment.
It's a global in krb5_kerberos_enctypes()...
There was a problem hiding this comment.
Then why do we need to cache it? And why does it take a context argument? (Not in front of computer :))
There was a problem hiding this comment.
Because it's a pair of static variables inside that function, krb5_kerberos_enctypes()...
There was a problem hiding this comment.
Hmm, OK, so relying on krb5_enctype_valid() being invariant across contexts, which I guess is safe. But, why can't we just call krb5_enctype_valid() each time?
There was a problem hiding this comment.
Ah, that: because it does a lot of work on error that we don't need to do in this context. I'm manually inlining just the piece we need.
nicowilliams
force-pushed
the
rework-_kdc_find_etype
branch
5 times, most recently
from
3a28906
to
c9a4abf
Compare
This is a bug that was introduced by 5447b81 (which made it so that on-store we derive an `etypes` for every HDB entry that had that field absent), but masked by a bug in `_kdc_find_etype()`. The fix is to not just rely on `kadmin del_enctype` for affecting ticket session key negotiation, but to also or instead use `kadmin mod -e`.
_krb5_find_enctype() gets called from loops implementing linear searches, yielding accidentally O(N^2) or worse algorithms. But _krb5_find_enctype() can be O(1) for all numerically-small enctypes. This commit makes it so.
nicowilliams
force-pushed
the
rework-_kdc_find_etype
branch
from
c9a4abf
to
a287236
Compare
_kdc_find_etype()has become a monster. It's time to rewrite it.I'm splitting it into two parts:
_kdc_find_etype(), meant for selecting a ticket's session key enctype (though it's also used in GSS pre-auth to pick the enctype for which to derive a reply key, but the considerations are the same in that case)_kdc_find_key_and_salt(), which is used to pick a key and salt for use inPA-ETYPE-INFOandPA-ETYPE-INFO2The latter is similar in function to
_kdc_get_preferred_key(), but whereas theKDC-REQ-BODY'setypelist does not affect service key selection, it does affect client key selection (I think so anyways), so_kdc_find_key_and_salt()does call_kdc_find_etype().