Date: Fri, 8 Jan 2021 15:33:44 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Bruce Momjian <bruce@momjian.us>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Subject: Re: Key management with tests
Message-ID: <20210108203344.GV27507@tamriel.snowman.net>
References: <20210107150214.GH32172@momjian.us>
 <20210107190849.GA9852@alvherre.pgsql>
 <20210108003928.GA18394@momjian.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="hRxM9ZOJRrGBCTys"
Content-Disposition: inline
In-Reply-To: <20210108003928.GA18394@momjian.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--hRxM9ZOJRrGBCTys
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:
> On Thu, Jan  7, 2021 at 04:08:49PM -0300, =C1lvaro Herrera wrote:
> > On 2021-Jan-07, Bruce Momjian wrote:
> >=20
> > > All the tests pass now.  The current src/test directory is 19MB, and
> > > adding these tests takes it to 23MB, or a 20% increase.  That seems l=
ike
> > > a lot.  It is testing 128-bit and 256-bit keys --- should we do fewer
> > > tests, or just test 256, or use gzip to compress the tests by 50%?=20
> > > (Does every platform have gzip?)
> >=20
> > So the tests are about 95% of the patch ... do we really need that many
> > tests?
>=20
> No, I don't think so.  Stephen imported the entire NIST test suite.  It
> was so comperhensive, it detected several OpenSSL bugs for zero-length
> strings, which I already reported, but we would never be encrypting
> zero-length strings, so there wasn't a lot of value to it.

I ran the entire test suite locally to ensure everything worked, but I
didn't actually include all of it in the PR which you merged- I had
already reduced it quite a bit by removing all 'additional
authenticated data' test cases (which the tests will automatically skip
and which we haven't implemented support for in the common library
wrappers) and by removing the 192-bit cases.  This reduced the overall
test set by about 2/3rd's or so, as I recall.

> Anyway, I think we need to figure out how to trim.  The first part would
> be to figure out whether we need 128 _and_ 256-bit tests, and then see
> what items are really useful.  Stephen, do you have any ideas on that?
> We currently have 10296 tests, and I think we could get away with 100.

Yeah, it's probably still too much, but I don't have any particularly
justifiable suggestions as to exactly what we should remove or what we
should keep.

Perhaps it'd make sense to try and cover the cases that are more likely
to be issues between our wrapper functions and OpenSSL, and not stress
too much about constantly testing cases that should really be up to
OpenSSL.  As such, I'd propose:

- Add back in some 192-bit tests, so we cover all three bit lengths.
- Add back in some additional authenticated test cases, just to make
  sure that, until/unless we implement support, the test code properly
  skips over those.
- Keep tests for various length plaintext/ciphertext (including 0-byte
  cases, so we make sure those work, since they really should).
- Keep at least one test for each length of tag that's included in the
  test suite.

I'm not sure how many tests we'd end up with from that, but my swag /
gut feeling is that it'd probably be on the order of 100ish and a small
enough set that it won't dwarf the rest of the patch.

Would be nice if we had a way for some buildfarm animal or something to
pull in the entire suite and test it, imv..  If anyone wants to
volunteer, I'd be happy to explain how to make that happen (it's not
hard though- download/unzip the files, drop them in the directory,
update the test script to add all the files into the array).

Thanks,

Stephen

--hRxM9ZOJRrGBCTys
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJf+MGoAAoJEO1sijiDR2RV7+UP/3cMPM3DeaiS3pEVICA6cTYw
rslV1B3C0KK2JD9IaJpXm28qlluXnhQ/CW/iZjj0ObK0oRb7ayZitBEZVO3CnWcr
rsjaf1fJb7LW92YbLQHmulUOHiHaEDLbFapUrd0SNZWZjE3k93yIr4xbGlR3Wu18
/cyUd/vOSwNsT9d/nkzNlxpmSc/7sjijbjuxI5jtSg8aWbXN/VYodEwkOyX/fZi3
SNA846HqNPb4PBr8aNx3xCvaquivjFUB/y6ujO5ec/1fH30qERQki+72pFn5ZDB3
L0qzvI8HRQEp2vWTCEOftI7y3j3CsZAoeF1yuBjXAzOmvk0NA7IvtdVGLqHtwoEA
GE0dEkpaEQgOqMdjKZBzLazFDQFIxmkyU+cGjuHwilHSrD3tMBpzZoy3ANBTUhsY
o81Mojc3ESYM3PzQe6Bf7mjVPj4Gaq5NGGOGok39JWMG67jJmdIqehHyFiDogfCi
Qc5nSnvPrVQBIdsZQXcKpZzItWSJrTo85GRE5ONjOiwA8xwG7cGOkh8mdqWCyZ4k
cuJHVmoB07+aZ/JQxfRjW4F+3KhToy5KYCoCzJxbSM77r4+f92havl8h30q6047t
Kr5GSx5FIcLB+W6sIu0DhHy9k/S25qTAow16p5eEKtkiiUIBZ+491Rv3+OTrXdmp
6xgTz1OJZFxFokvh/cSb
=HnEP
-----END PGP SIGNATURE-----

--hRxM9ZOJRrGBCTys--