[MS-BKRP] backupkey server and GnuTLS

[Garming Sam]

Hi Andreas,

I've looked through all the patches and I'm fairly happy with them. 
There's a few things I noticed though, but apart from those you can 
effectively consider me signed off.


s4-torture: Migrate get_cert_guid() from backupkey to GnuTLS
In this patch, I noticed that there's a predefined size for the issuer 
unique id. I was wondering if it would be more appropriate to avoid this 
assumption (calling the function twice to get the correct length). The 
same goes for the additional torture tests that you've added. Assuming 
that all these checks pass on Windows, then they're definitely helpful 
additions.


s4-rpc-bkrp: Use GnuTLS API for hash functions
I'm well aware that GNUTLS_MAC_SHA1 refers to the same constant as its 
digest counterpart, but if it is doing a plain digest, then the 
appropriate constant should probably be used (especially when skimming, 
it's one of the more obvious things to notice).


s4-rpc-bkrp: Self sign the certificate using GnuTLS
In the function, generate_bkrp_cert, it looks like you may have missed 
'gnutls_privkey_deinit(issuer_privkey)' on the first return of WERR_NOMEM.


I also noticed you removed the CA status, which was the other thing I 
was going to comment on.


Cheers,

Garming


On 27/11/15 11:44, Andrew Bartlett wrote:
> On Thu, 2015-11-26 at 16:55 +0100, Andreas Schneider wrote:
>> On Friday 20 November 2015 10:44:04 Andrew Bartlett wrote:
>>>> So I hope you can explain the testing procedures you used ...
>>> This is what we did (we used libvirt snapshot VMs):
>>>
>>> - Take a Dec 2014 patched Windows 8.1 machine that has never, ever
>>> been
>>> joined to the domain
>>>
>>> - Join to the domain
>>>
>>> - Log in as administrator
>>>
>>> - Open Credentials Manager (part of control panel, can be searched
>>> for)
>>>
>>> - If it gives an error, then there is an issue, if it opens
>>> correctly
>>> you are OK.
>> Hi Andrew,
>>
>> I've tested with Windows 8.1 and fixed the remaining bugs. After I
>> identified
>> the remaining issues, I've improved the torture test. It doesn't only
>> validate
>> the RSA key bits but also the rest of the cert. I run the test
>> against Windows
>> 2012 to fine tune it.
>>
>> You can find the patchset for review here:
>>
>> https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master
>> -rpc-bkrp
>>
>> That we are completely using GnuTLS you need version 3.4.7. This
>> version
>> offers gnutls_x509_crt_set_issuer_unique_id(). I check for this
>> symbol and if
>> found we only build the backupkey server using GnuTLS. If not the
>> certificate
>> self signing part still uses Heimdal.
>>
>> The top commit is an additional torture test which only works against
>> Windows
>> and backupkey built with GnuTLS 3.4.7.
>>
>> Attached shows that Windows 8.1 Credentials Manager is working.
>>
>>
>> Please review and push!
> Garming was looking carefully over the code yesterday when he was in
> the office, so I'll ask him to finish that and get you his
> reviews/comments on Monday.
> Thanks for all your hard efforts here.  Removing the dependency on
> Heimdal is a critical step for Samba as an AD DC, and this is really
> important work.  It is also really complex, sensitive code that has
> caused issues in the past, so while I know it is frustrating I would
> kindly ask you to wait for Garming's review.
> I really appreciate that you not only fixed the issues against Windows,
> but extended the testsuite to match.
> Thanks!
> Andrew Bartlett