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

[Petr Viktorin]

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³