[Freeipa-devel] [PATCH 0053] Implement OTP token importing

[Simo Sorce]

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.

Simo.