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. Simo. _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
