On Fri, 13 Jun 2014, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
I read through the patch and apart from the points Simo made, I don't
have much complaints. One additional thing to do is how to test it --
how to create the payload? Since this code doesn't require anything
specific, it could be well tested through a CI infrastructure.
Additionally, man page is missing.
--
/ Alexander Bokovoy
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
