review of the backupkey protocol implementation

[Andrew Bartlett]

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

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?  

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. 

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. 

Andrew Bartlett


-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.