kdc: cross realm s4u2self handling

[Isaac Boukris]

On Wed, Sep 19, 2018 at 2:57 AM, Greg Hudson <ghudson at mit.edu> wrote:
> On 09/18/2018 12:12 PM, Isaac Boukris wrote:
>>
>> I'd like to start with the last commit in the MIT patch, which
>> partially reverts 8a9909ff9ef6b51c5ed09ead6713888fbb34072f commit,
>> that I don't fully understand.
>
>
> That was part of a series of commits made between 1.11 and 1.12, with the
> goal of treating the client request as an immutable object (commit
> 9e37d01a0122904776fada43ec65425c375414d8).  Prior to those commits,
> process_tgs_req() would set the request server to the result of the server
> search, which could be a referral.  For the commit you are trying to
> understand, there were two checks against request->server that I was
> concerned with:
>
> * In kdc_util.c:kdc_process_s4u2self_req(), we compare request->server
> against the client principal in the header ticket and error out if it
> doesn't match.
>
> * In tgs_policy.c:check_tgs_nontgt(), we compare the header ticket server
> against the request server and error out if it doesn't match.
>
> I wanted to make sure that leaving request->server alone would not allow
> requests through which would previously have been disallowed.  I was only
> considering within-realm S4U2Self requests, but I believe that in the 1.11
> code, the check in kdc_process_s4u2self_req() would have prevented
> cross-realm S4U2Self requests from succeeding.  It no longer interferes
> bequest request->server is now the original request server.
>
> I may have unwittingly been preserving a bug rather than an intentional
> restriction, so we can probably relax it.


Ok, it makes sense now, I can see it'd have failed the comparison in
kdc_process_s4u2self_req() (thanks for the detailed clarification).

Note that in my heimdal work, I have assumed that we can skip this
comparison check when the service isn't local since we are not issuing a
ticket to self but a referral, so let the KDC of the service do the check
when it issues the service ticket.
The reason it came up was, because heimdal has a more relaxed check
implemented via hdb plugin which allows matching different names, like an
SPN to a regular principal name, by looking up both name in db and calling
the plugin callback with both entries, then samba plugin just compares the
SID to confirm it's the same principal.
However, when the service is not local, we don't have these db handles so
we can't do that.
I think we should implement something similar in MIT too (in a later
stage), do you think the above assumption is correct?


Secondly, I'm wondering if we need to change the sign_authdata() hdb
callback to also supply the tgt-client principal name (see the first commit
in samba patch).

The reason is that in s4u2self, when the impersonated principal is from
local realm, then the PAC is of the tgt-client and we may need to verify it
(heimdal does, though I'm unsure why).
For in-realm request, we could use server->princ, since they are the same
principal, but when the service is foreign then it will be the cross realm
trust principal and we cannot verify the PAC.
I guess my main question is whether we need to verify this PAC at, or we
can just discard it, which is more of a Windows question.


Other than that, what do you think of the pac_verify/sign_ex() routines,
does it look ok?


Thanks!