Re: Refactor MD5 implementations and switch to EVP for OpenSSL

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Refactor MD5 implementations and switch to EVP for OpenSSL
Date: 2020-12-09 05:47:25
Message-ID: X9Bk7fYENyM2vwjo@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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(at)online(dot)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

Attachment Content-Type Size
v4-0001-Refactor-MD5-implementations-in-the-tree.patch text/x-diff 46.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2020-12-09 06:13:48 Re: [PATCH] Keeps tracking the uniqueness with UniqueKey
Previous Message Dilip Kumar 2020-12-09 04:46:41 Re: Parallel Inserts in CREATE TABLE AS