review of the backupkey protocol implementation

[Matthieu Patou]

On 31/01/2011 04:34, Andrew Bartlett wrote:
> On Sun, 2010-11-21 at 17:40 +0300, Matthieu Patou wrote:
>> On 12/10/2010 11:24, Stefan (metze) Metzmacher wrote:
>>> Am 12.10.2010 02:06, schrieb Andrew Bartlett:
>>>> On Tue, 2010-10-05 at 10:43 +0400, Matthieu Patou wrote:
>>>>> Hello,
>>>>>
>>>>> I would like to have a code review of my implementation of the backupkey
>>>>> protocol.
>>>>>
>>>>> The whole stuff is working correctly now and pass make test but I think
>>>>> it would be great to have a review just in case there is some code smell.
>>>>>
>>>>> It's here:
>>>>> http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/backupkey_heimdal_recent
>>> Please fix the indentation in a lot of places, e.g. the backupkey.idl
>>>
>>> And remove unused stuff.
>>>
>>> Why do we need bkrp_BackupKey_debug? If we really need that it should
>>> have the 'noopnum'
>>> property.
>>>
>>> Please create a separate commit that removes the old
>>> protected_storage.idl and related stuff.
>>>
>>> I think we also don't need the ndr_backupkey_print.[ch] files they can
>>> be merged into ndr_backupkey.[ch]
>>>
>>> http://git.samba.org/?p=mat/samba.git;a=commitdiff;h=36269ec9dec917c7500e73ca69fa51149b768d0c
>>> What is BIGNUM ? the '*' of pointers should be next to the variable and
>>> not the type (in a lot of places)
> This is still a problem in the current tree, in function prototypes.
>
>>> The '{' of functions should be in a new line.
>>>
>>> char *addr = "unknown"; should be 'const char *addr = "unknown"'
>>>
>>> You don't need sub_ctx and explicit talloc_free/unlink calls, mem_ctx
>>> is already the temporary memory context, which is cleaned up after the call.
>>>
>>> And please go through each single line and fix the identation and coding
>>> style,
>>> it's really not that hard...
>> This should be fixed in the current version of this branch on my private
>> repo:
>> http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/backupkey_heimdal
>>
>> Can I have your final comment before I push it to autobuild ?
> I have three concerns in this set of patches:
> The first should be easy to fix: I don't like the use of gendb_search()
> etc.  These need to be changed to dsdb_search() or ldb_search() in new
> code (we should remove gendb_search() as it looses errors, referrals and
> controls).

Well I tried with dsdb_search and it didn't work as when I try to get 
the currentVal it's always empty.
I'll give a try to ldb_search to see if it works (I didn't try).
> In generate_bkrp_cert():
>
> The issue I have is with the extension of the exponent in the public key
> computation to 4 bytes.  Why is this done?  What if the exponent is
> already greater than 4 bytes?
So the IDL for this stipulate that the public exponent is 4 bytes no 
less no more so if it's less then you have to pad so that it fills 
nicely in the buffer.
In our code we are pretty sure that it will less than 4 bytes as when we 
create the rsa key just a few lines before we set it to 0x10001 .
I could add an assert in the unlikely case that the length is bigger 
than 4 bytes, I'm pretty confident it won't happen unless some magic bit 
flipping happens or if heimdal starts to badly behave.
> You seem to by trying to use the trick that because you have already
> packed the buffer little endian, that you can extend the integer with
> memcpy() and zero-filling the buffer.

>    If it must be exactly 4, then
> perhaps some comments explaining why this is valid might assist.  I
> would prefer to unpack it into an integer (manually if so required) and
> repack it using our byte order macros, but whatever you do, make it
> clear.
It has to be in little endian what ever your endianess is, I had the impression that just padding at the end.


> Similarly, I'm concerned about the reverse_and_get_blob in general.  It
> seems from inspection that heimdal's BIGNUM integers are big endian, but
> that BACKUP_KEY is little endian.  It would be good to clarify this in
> comments, as all the reversals in this protocol just look very odd.
Noted I'll add some comments on this points before.

Matthieu.
-- 

Matthieu Patou
Samba Team        http://samba.org
Private repo      http://git.samba.org/?p=mat/samba.git;a=summary