Fix smartcard offline logon and NTLM authentication

[Stefan Metzmacher]

Am 27.06.2016 um 06:57 schrieb Andrew Bartlett:
> On Mon, 2016-06-27 at 15:02 +1200, Andrew Bartlett wrote:
>> On Mon, 2016-06-20 at 22:55 +0200, Stefan Metzmacher wrote:
>>>
>>> Hi,
>>>
>>> here're some patches to fix smartcard offline logons
>>> and related bugs.
>>>
>>> The key part is adding PAC_CREDENTIAL with the NTHASH.
>>>
>>> In order to avoid an NTHASH based on a password,
>>> I also implemented the UF_SMARTCARD_REQUIRED feature,
>>> that generates a random NTHASH value, that is only
>>> known to the KDC and the private key of the smartcard.
>>>
>>> I may need to add some more BUG: markers, but you can start
>>> with the review now:-)
>>>
>>> See
>>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/head
>>> s/
>>> master4-smart-ok
>>> it's based on
>>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/head
>>> s/
>>> master4-smart-base
>> G'Day metze,
>>
>> I can't see any tests for the critical components of this task, that
>> is
>> the changed PAC.  Can you add a test that confirms the returned PAC
>> has
>> the correct password, nor that these elements are present?
>>
>> I'll keep looking over the rest of the changes.  I know you mention
>> adding more BUG: markers, which is OK, but please don't backport
>> these.
>>  Samba 4.5 is coming soon enough, and I would really prefer not to
>> see
>> big backports made for pwdLastSet nor smart card login features.  

Ok, I'll keep them within the samba.plus packages only for 4.4.

>> Finally, please ensure that you fix the code to pass the repl_move
>> test.  This is sensitive to the exact repl_meta_data behaviour, in
>> particular the number of password attributes with metadata, but it
>> seems we still don't match Windows even with your changes. 

What difference do you see compared to Windows?

> I've put review markers on some of the patches at: 
> 
> http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/m
> etze-logoncount-smart
> 
> However, to review the others, I would like to see:
>  - example values for supplementalCredentials put in
> source4/torture/ndr/drsblobs.c to test the changed IDL and the new
> manual parser. 
>  - similar tests for the PAC changes.  In particular, I'm nervous
> because of the issues last time we introduced PAC_UPN_DNS_INFO last
> time, and because there are probably other more sloppy PAC parsers out
> there in other products.

Yes, I'm working with Günther to add those tests.

>  - Tests to show we fill in the PAC correctly, both in the PKINIT and
> not-PKINIT case (given we are adding the UPN stuff).
>  - Confirmation that the password in the PAC is correct (as above).  
>  - You could pull one password with GetNCChanges REPL_OBJECT if you
> want to test the randomly generated case. 

This would require a lot of additional work, as it's currently not possible
to get the required replykey out of existing krb5 libraries
in order to decrypt the PAC_CREDENTIALS blob.

I agree it would be nice to have tests for all this, but if they are
required to
get this in, it would mean these fixes for real world problems won't
make it into 4.5,
sorry.

> I would also like to thank you for your incredible patience in this
> area.  I have neglected your requests for review for too long, and I
> hope this downpayment is some small way towards reducing the backlog.
> 
> I'm really quite excited by what Samba 4.5 looks like having for the AD
> DC.  My team here at Catalyst are working on a lot of performance and
> correctness issues right now around linked attributes and LDB, and we
> are gaining some big improvements!

Yes, 4.5 will contain quite some improvements, if we can get everything
upstream
we already have ready.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160627/4debc524/signature.sig>