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

[Nathaniel McCallum]

On Mon, 2014-06-16 at 11:53 +0300, Alexander Bokovoy wrote:
> 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.

I don't see why we can't use the samples that jcholast dug up as test
data since they come from the RFC. The noted fix for Figure 7 should be
included so that the test works.

> Additionally, man page is missing.

The man page should be in 0053.2.