On 06/14/2014 12:05 AM, 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]
If you want to use idiomatic Python,
starmap(lambda a, b: a ^ b, izip(u, [ord(c) for c in l]))
can be rewritten as
[a ^ ord(b) for a, b in zip(u, l)]
(izip is overkill if the list isn't too long)
then you can use
dk.extend(u)
to keep a single list of ints in dk, and at the end,
return ''.join(chr(c) for c in dk[:self.klen])
to turn part you need into a string.
It would be nice to have some tests for this.
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.
Simo.
I see the patch contains lots of classes with no state and a single
method. I believe it would simplify the code greatly if functions were
used instead.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
