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

[Alexander Bokovoy]

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