Fix smartcard offline logon and NTLM authentication

Andrew Bartlett abartlet at samba.org
Mon Jun 27 07:10:21 UTC 2016


On Mon, 2016-06-27 at 08:00 +0200, Stefan Metzmacher wrote:
> 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.

That's up to you.  

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

If you run the test, you will see that it now neither matches the
expected values for Samba nor windows.  I've not confirmed with
attribute is missing, but given the work done here, I think you should
write a similar, specific test, asserting all the replPropertyMetaData
values, just as I don in that test.

Do these branches pass a full autobuild for you?

> > I've put review markers on some of the patches at: 
> > 
> > http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/hea
> > ds/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.

We already have tests for the PAC in smbtoture.  Please just extend
those. 

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

We need this stuff tested, and we have what is needed to start.  We
can't add a change like this to our core authorization layer without
tests that cover it comprehensively, specifically:  
 - NDR tests of saved PAC values
 - runtime tests of expected PAC values from the live KDC in each
situation.

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

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba






More information about the samba-technical mailing list