On Mon, Dec 07, 2020 at 02:15:58PM +0100, Daniel Gustafsson wrote:
> This is a pretty straightforward patch which given the cryptohash framework
> added previously does what it says on the tin. +1 on rolling MD5 into what we
> already have for SHA2 now. Applies clean and all tests pass.
Thanks.
> One (two-part) comment on the patch though:
>
> The re-arrangement of the code does lead to some attribution confusion however
> IMO. pgcrypto/md5.c and src/common/md5.c are combined with the actual MD5
> implementation from pgcrypto/md5.c and the PG glue code from src/common/md5.c.
> That is in itself a good choice, but the file headers are intact and now claims
> two implementations of which only one remains.
I was pretty sure that at some point I got the attributions messed up.
Thanks for looking at this stuff and ringing the bell.
> Further, bytesToHex was imported in commit 957613be18e6b7 with attribution to
> "Sverre H. Huseby <sverrehu@online.no>" without a PGDG defined copyright,
> which was later added in 2ca65f716aee9ec441dda91d91b88dd7a00bffa1. This patch
> moves bytesToHex from md5.c (where his attribution is) to md5_common.c with no
> maintained attribution. Off the cuff it seems we should either attribute in a
> comment, or leave the function and export it, but the usual "I'm not a lawyer"
> disclaimer applies. Do you have any thoughts?
Hmm. Looking at this area of this history, like d330f155, I think
that what we just need to do is to switch the attribution of Sverre
to md5_common.c for all the sub-functions dedicated to the generation
of the MD5 passwords by fixing the header comment of the file as this
is the remaining code coming from the file where this code was. In
the new md5.c and md5_int.h, the fallback implementation that we get
to use is the KAME one from pgcrypto so we should just mention KAME
there.
I have spent some time double-checking all this stuff, adjusting some
comments, and making the style of the new files more consistent with
the surroundings while minimizing the amount of noise diffs (pgindent
has adjusted some things by itself for the new files). In short, this
seems rather committable to me.
--
Michael