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. > 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? > 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. > Other than the first point though, anything else looks good to me. Thanks for looking at this! Nathaniel _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
