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

[Petr Viktorin]

On 06/16/2014 06:23 PM, Nathaniel McCallum wrote:
> On Fri, 2014-06-13 at 18:05 -0400, 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 ?
>
> Argh. Yes. Thanks for this.

lxml.etree.XMLParser has a separate option for this, no_network, which 
is True by default.


>> 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
>
> There is only one PBKDF2 with differing PRFs. This is precisely what is
> implemented. However, I'm still working with jdennis to see if I can
> coax python-nss to do this for me. I'm happy to delete my own
> implementation.
>
>> Btw for the final join, wouldn't it be more compact to use:
>> dk += ''.join(map(chr,u)) ?
>
> My understanding is that comprehensions are now preferred. I know
> reduce() has been removed in Python 3.x. However, map() still appears to
> be there. Does anyone have the official word on the preferred style?

Here's my view, strictly unofficial:

Comprehensions are better in almost all cases.
This (converting each value) is just about the only one where map is 
more concise, and roughly equally readable.
So, for consistency, it's better to use comprehensions everywhere.


>> 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]
>
> Sounds good. Let's see if nss can do this. If not, I'll clean this up.
>
>> 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.
>
> In general, this kind of complexity is reduced somewhat in Python 3.x's
> new bytes/str division. I have a bytes subclass for Py3k that I like to
> use which implements the bitwise operators. I've even had ideas about
> submitting this as an upstream Python patch for the bytes class.

Try the python-ideas list first. There's a recent thread there from Nick 
Coghlan on a similar topic, you might want to read it.



-- 
Petr³