Allow tests to pass in OpenSSL FIPS mode

On 04.10.22 17:45, Peter Eisentraut wrote:

While working on the column encryption patch, I wanted to check that
what is implemented also works in OpenSSL FIPS mode.  I tried running
the normal test suites after switching the OpenSSL installation to FIPS
mode, but that failed all over the place.  So I embarked on fixing that.

Of course, there are some some tests where we do want to test MD5
functionality, such as in the authentication tests or in the tests of
the md5() function itself.  I think we can conditionalize these somehow.

Let's make a small start on this. The attached patch moves the tests of
the md5() function to a separate test file. That would ultimately make
it easier to maintain a variant expected file for FIPS mode where that
function will fail (similar to how we have done it for the pgcrypto tests).

On Tue, Oct 11, 2022 at 01:51:50PM +0200, Peter Eisentraut wrote:

Let's make a small start on this. The attached patch moves the tests of the
md5() function to a separate test file. That would ultimately make it
easier to maintain a variant expected file for FIPS mode where that function
will fail (similar to how we have done it for the pgcrypto tests).

Makes sense to me. This slice looks fine.

I think that the other md5() computations done in the main regression
test suite could just be switched to use one of the sha*() functions
as they just want to put their hands on text values. It looks like a
few of them have some expections with the output size and
generate_series(), though, but this could be tweaked by making the
series shorter, for example.
--
Michael

On 12.10.22 03:18, Michael Paquier wrote:

On Tue, Oct 11, 2022 at 01:51:50PM +0200, Peter Eisentraut wrote:

Let's make a small start on this. The attached patch moves the tests of the
md5() function to a separate test file. That would ultimately make it
easier to maintain a variant expected file for FIPS mode where that function
will fail (similar to how we have done it for the pgcrypto tests).

Makes sense to me. This slice looks fine.

Committed.

I think that the other md5() computations done in the main regression
test suite could just be switched to use one of the sha*() functions
as they just want to put their hands on text values. It looks like a
few of them have some expections with the output size and
generate_series(), though, but this could be tweaked by making the
series shorter, for example.

Right, that's the rest of my original patch. I'll come back with an
updated version of that.

On 2022-Oct-13, Peter Eisentraut wrote:

Right, that's the rest of my original patch. I'll come back with an updated
version of that.

However, there are some changes in brin_multi.out that are quite
surprising and suggest that we might have bugs in brin:

+WARNING:  unexpected number of results 31 for (macaddr8col,>,macaddr8,b1:d1:0e:7b:af:a4:42:12,33)
+WARNING:  unexpected number of results 17 for (macaddr8col,>=,macaddr8,d9:35:91:bd:f7:86:0e:1e,15)
+WARNING:  unexpected number of results 11 for (macaddr8col,<=,macaddr8,23:e8:46:63:86:07:ad:cb,13)
+WARNING:  unexpected number of results 4 for (macaddr8col,<,macaddr8,13:16:8e:6a:2e:6c:84:b4,6)

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La victoria es para quien se atreve a estar solo"

On 13.10.22 12:26, Peter Eisentraut wrote:

I think that the other md5() computations done in the main regression
test suite could just be switched to use one of the sha*() functions
as they just want to put their hands on text values.  It looks like a
few of them have some expections with the output size and
generate_series(), though, but this could be tweaked by making the
series shorter, for example.

Right, that's the rest of my original patch.  I'll come back with an
updated version of that.

Here is the next step. To contain the scope, I focused on just "make
check" for now. This patch removes all incidental calls to md5(),
replacing them with sha256(), so that they'd pass with or without FIPS
mode. (Two tests would need alternative expected files: md5 and
password. I have not included those here.)

Some tests inspect the actual md5 result strings or build statistics
based on them. I have tried to carefully preserve the meaning of the
original tests, to the extent that they could be inferred, in some cases
adjusting example values by matching the md5 outputs to the equivalent
sha256 outputs. Some cases are tricky or mysterious or both and could
use another look.

On Wed, Dec 07, 2022 at 03:14:09PM +0100, Peter Eisentraut wrote:

Here is the next step. To contain the scope, I focused on just "make check"
for now. This patch removes all incidental calls to md5(), replacing them
with sha256(), so that they'd pass with or without FIPS mode. (Two tests
would need alternative expected files: md5 and password. I have not
included those here.)

Yeah, fine by me to do that step-by-step.

Some tests inspect the actual md5 result strings or build statistics based
on them. I have tried to carefully preserve the meaning of the original
tests, to the extent that they could be inferred, in some cases adjusting
example values by matching the md5 outputs to the equivalent sha256 outputs.
Some cases are tricky or mysterious or both and could use another look.

incremental_sort mostly relies on the plan generated, so the change
should be rather straight-forward I guess, though there may be a side
effect depending on costing. Hmm, it does not look like stats_ext
would be an issue as it checks the stats correlation of the attributes
for mcv_lists_arrays.

largeobject_1.out has been forgotten in the set requiring a refresh.
--
Michael

On 09.12.22 05:16, Michael Paquier wrote:

Some tests inspect the actual md5 result strings or build statistics based
on them. I have tried to carefully preserve the meaning of the original
tests, to the extent that they could be inferred, in some cases adjusting
example values by matching the md5 outputs to the equivalent sha256 outputs.
Some cases are tricky or mysterious or both and could use another look.

incremental_sort mostly relies on the plan generated, so the change
should be rather straight-forward I guess, though there may be a side
effect depending on costing. Hmm, it does not look like stats_ext
would be an issue as it checks the stats correlation of the attributes
for mcv_lists_arrays.

largeobject_1.out has been forgotten in the set requiring a refresh.

Here is a refreshed patch with the missing file added.

On Thu, Oct 13, 2022 at 01:16:18PM +0200, Alvaro Herrera wrote:

However, there are some changes in brin_multi.out that are quite
surprising and suggest that we might have bugs in brin:

+WARNING:  unexpected number of results 31 for (macaddr8col,>,macaddr8,b1:d1:0e:7b:af:a4:42:12,33)
+WARNING:  unexpected number of results 17 for (macaddr8col,>=,macaddr8,d9:35:91:bd:f7:86:0e:1e,15)
+WARNING:  unexpected number of results 11 for (macaddr8col,<=,macaddr8,23:e8:46:63:86:07:ad:cb,13)
+WARNING:  unexpected number of results 4 for (macaddr8col,<,macaddr8,13:16:8e:6a:2e:6c:84:b4,6)

This refers to brin_minmax_multi_distance_macaddr8(), no? This is
amazing. I have a hard time imagining how FIPS would interact with
what we do in mac8.c to explain that, so it may be something entirely
different. Is that reproducible?
--
Michael

On 27.02.23 08:16, Michael Paquier wrote:

On Thu, Oct 13, 2022 at 01:16:18PM +0200, Alvaro Herrera wrote:

However, there are some changes in brin_multi.out that are quite
surprising and suggest that we might have bugs in brin:

+WARNING:  unexpected number of results 31 for (macaddr8col,>,macaddr8,b1:d1:0e:7b:af:a4:42:12,33)
+WARNING:  unexpected number of results 17 for (macaddr8col,>=,macaddr8,d9:35:91:bd:f7:86:0e:1e,15)
+WARNING:  unexpected number of results 11 for (macaddr8col,<=,macaddr8,23:e8:46:63:86:07:ad:cb,13)
+WARNING:  unexpected number of results 4 for (macaddr8col,<,macaddr8,13:16:8e:6a:2e:6c:84:b4,6)

This refers to brin_minmax_multi_distance_macaddr8(), no? This is
amazing. I have a hard time imagining how FIPS would interact with
what we do in mac8.c to explain that, so it may be something entirely
different. Is that reproducible?

This is no longer present in the v2 patch.

On Mon, Feb 27, 2023 at 08:23:34AM +0100, Peter Eisentraut wrote:

On 27.02.23 08:16, Michael Paquier wrote:

This refers to brin_minmax_multi_distance_macaddr8(), no? This is
amazing. I have a hard time imagining how FIPS would interact with
what we do in mac8.c to explain that, so it may be something entirely
different. Is that reproducible?

This is no longer present in the v2 patch.

Sure, but why was it happening in the first place? The proposed patch
set only reworks some regression tests. So It seems to me that this
is a sign that we may have issues in some code area that got stressed
in some new way, no?
--
Michael

On 28.02.23 06:01, Michael Paquier wrote:

On Mon, Feb 27, 2023 at 08:23:34AM +0100, Peter Eisentraut wrote:

On 27.02.23 08:16, Michael Paquier wrote:

This refers to brin_minmax_multi_distance_macaddr8(), no? This is
amazing. I have a hard time imagining how FIPS would interact with
what we do in mac8.c to explain that, so it may be something entirely
different. Is that reproducible?

This is no longer present in the v2 patch.

Sure, but why was it happening in the first place?

Because the earlier patch only changed the test input values (which were
generated on the fly using md5()), but did not adjust the expected test
results in all the places.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

[ v2-0001-Remove-incidental-md5-function-uses-from-main-reg.patch ]

I've gone through this and have a modest suggestion: let's invent some
wrapper functions around encode(sha256()) to reduce the cosmetic diffs
and consequent need for closer study of patch changes. In the attached
I called them "notmd5()", but I'm surely not wedded to that name.

This also accounts for some relatively recent additions to stats_ext.sql
that introduced yet more uses of md5(). This passes for me on a
FIPS-enabled Fedora system, with the exception of md5.sql and
password.sql. I agree that the right thing for md5.sql is just to add
a variant expected-file. password.sql could perhaps use some refactoring
so that we don't have two large expected-files to manage.

The only other place that perhaps needs discussion is rowsecurity.sql,
which has some surprisingly large changes: not only do the random
strings change, but there are rowcount differences in some results.
I believe this is because there are RLS policy checks and view conditions
that actually examine the contents of the "md5" strings, eg

CREATE POLICY p1 ON s1 USING (a in (select x from s2 where y like '%2f%'));

My recommendation is to just accept those changes as OK and move on.
I doubt that anybody checked the existing results line-by-line either.

So, once we've done something about md5.sql and password.sql, I think
this is committable.

regards, tom lane

On 5 Mar 2023, at 00:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

[ v2-0001-Remove-incidental-md5-function-uses-from-main-reg.patch ]

I've gone through this and have a modest suggestion: let's invent some
wrapper functions around encode(sha256()) to reduce the cosmetic diffs
and consequent need for closer study of patch changes. In the attached
I called them "notmd5()", but I'm surely not wedded to that name.

For readers without all context, wouldn't it be better to encode in the
function name why we're not just calling a hash like md5? Something like
fips_allowed_hash() or similar?

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

On 5 Mar 2023, at 00:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I've gone through this and have a modest suggestion: let's invent some
wrapper functions around encode(sha256()) to reduce the cosmetic diffs
and consequent need for closer study of patch changes. In the attached
I called them "notmd5()", but I'm surely not wedded to that name.

For readers without all context, wouldn't it be better to encode in the
function name why we're not just calling a hash like md5? Something like
fips_allowed_hash() or similar?

I'd prefer shorter than that --- all these queries are laid out on the
expectation of a very short function name. Maybe "fipshash()"?

We could make the comment introducing the function declarations more
elaborate, too.

regards, tom lane

On 6 Mar 2023, at 15:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:

For readers without all context, wouldn't it be better to encode in the
function name why we're not just calling a hash like md5? Something like
fips_allowed_hash() or similar?

I'd prefer shorter than that --- all these queries are laid out on the
expectation of a very short function name. Maybe "fipshash()"?

We could make the comment introducing the function declarations more
elaborate, too.

fipshash() with an explanatory comments sounds like a good idea.

--
Daniel Gustafsson

On 05.03.23 00:04, Tom Lane wrote:

I've gone through this and have a modest suggestion: let's invent some
wrapper functions around encode(sha256()) to reduce the cosmetic diffs
and consequent need for closer study of patch changes. In the attached
I called them "notmd5()", but I'm surely not wedded to that name.

Do you mean create this on the fly in the test suite, or make it a new
built-in function?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 05.03.23 00:04, Tom Lane wrote:

I've gone through this and have a modest suggestion: let's invent some
wrapper functions around encode(sha256()) to reduce the cosmetic diffs
and consequent need for closer study of patch changes. In the attached
I called them "notmd5()", but I'm surely not wedded to that name.

Do you mean create this on the fly in the test suite, or make it a new
built-in function?

The former --- please read my version of the patch.

regards, tom lane

On 09.12.22 05:16, Michael Paquier wrote:

On Wed, Dec 07, 2022 at 03:14:09PM +0100, Peter Eisentraut wrote:

Here is the next step. To contain the scope, I focused on just "make check"
for now. This patch removes all incidental calls to md5(), replacing them
with sha256(), so that they'd pass with or without FIPS mode. (Two tests
would need alternative expected files: md5 and password. I have not
included those here.)

Yeah, fine by me to do that step-by-step.

It occurred to me that it would be easier to maintain this in the long
run if we could enable a "fake FIPS" mode that would have the same
effect but didn't require fiddling with the OpenSSL configuration or
installation.

The attached patch shows how this could work. Thoughts?

On 8 Mar 2023, at 09:49, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

It occurred to me that it would be easier to maintain this in the long run if we could enable a "fake FIPS" mode that would have the same effect but didn't require fiddling with the OpenSSL configuration or installation.

The attached patch shows how this could work. Thoughts?

- * Initialize a hash context.  Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * Initialize a hash context.
Regardless of which, we wan't this hunk since the code clearly can return -1.

+#ifdef FAKE_FIPS_MODE
I'm not enthusiastic about this. If we use this rather than OpenSSL with FIPS
enabled we might end up missing bugs or weird behavior due to changes in
OpenSSL that we didn't test.

--
Daniel Gustafsson