pgsql: Add key management system

On 2020-12-25 16:19, Bruce Momjian wrote:

Add key management system
doc/src/sgml/database-encryption.sgml | 97 +++++

Attached are a few typos.

I also noticed that this option does not occur in the initdb --help:

-u --copy-encryption-keys

Was that deliberate?

Thanks,

Erik Rijkers

On Fri, Dec 25, 2020 at 07:30:01PM +0100, Erik Rijkers wrote:

On 2020-12-25 16:19, Bruce Momjian wrote:

Add key management system
doc/src/sgml/database-encryption.sgml | 97 +++++

Attached are a few typos.

I also noticed that this option does not occur in the initdb --help:

-u --copy-encryption-keys

Was that deliberate?

No. :-( Attached patch applied. Thanks.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

I think cipher_failure() should be marked pg_attribute_noreturn().

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2 -I./src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o src/common/cipher.o src/common/cipher.c
src/common/cipher.c: In function ‘pg_cipher_ctx_create’:
src/common/cipher.c:28:1: warning: control reaches end of non-void function [-Wreturn-type]
28 | }

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

I think cipher_failure() should be marked pg_attribute_noreturn().

Perhaps more to the point, it still doesn't build at all without
--with-openssl.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&amp;dt=2020-12-25%2019%3A13%3A19

regards, tom lane

I wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

I think cipher_failure() should be marked pg_attribute_noreturn().

Perhaps more to the point, it still doesn't build at all without
--with-openssl.

[ looks closer ... ] Oh, we're on about the same thing -- the difference
is that sifaka is using -Werror.

pg_attribute_noreturn() seems like a good idea, but we're also going to
need dummy return statements in the callers, to satisfy compilers that
don't understand that.

regards, tom lane

On Fri, Dec 25, 2020 at 02:37:06PM -0500, Tom Lane wrote:

I wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

I think cipher_failure() should be marked pg_attribute_noreturn().

Perhaps more to the point, it still doesn't build at all without
--with-openssl.

[ looks closer ... ] Oh, we're on about the same thing -- the difference
is that sifaka is using -Werror.

pg_attribute_noreturn() seems like a good idea, but we're also going to
need dummy return statements in the callers, to satisfy compilers that
don't understand that.

Yes, done. I tested it with a non-OpenSSL configure run now and it
worked. Thanks for the report.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Dec 25, 2020 at 02:37:06PM -0500, Tom Lane wrote:

pg_attribute_noreturn() seems like a good idea, but we're also going to
need dummy return statements in the callers, to satisfy compilers that
don't understand that.

Yes, done. I tested it with a non-OpenSSL configure run now and it
worked. Thanks for the report.

Now that it compiles cleanly, what about test cases? It looks
to me like you broke src/test/Makefile:

@@ -30,7 +30,7 @@ endif
endif
ifeq ($(with_openssl),yes)
ifneq (,$(filter ssl,$(PG_TEST_EXTRA)))
-SUBDIRS += ssl
+SUBDIRS += ssl crypto
endif
endif

because there is no such subdirectory. Assuming that that's a
case of forgetting to "git add" the whole subdirectory, I still
don't much care for this implementation: the user should be able
to decide which subdirectories get run. Maybe more like

 ifeq ($(with_openssl),yes)
 ifneq (,$(filter ssl,$(PG_TEST_EXTRA)))
 SUBDIRS += ssl
 endif
+ifneq (,$(filter crypto,$(PG_TEST_EXTRA)))
+SUBDIRS += crypto
+endif
 endif

regards, tom lane

On Fri, Dec 25, 2020 at 02:53:10PM -0500, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Dec 25, 2020 at 02:37:06PM -0500, Tom Lane wrote:

pg_attribute_noreturn() seems like a good idea, but we're also going to
need dummy return statements in the callers, to satisfy compilers that
don't understand that.

Yes, done. I tested it with a non-OpenSSL configure run now and it
worked. Thanks for the report.

Now that it compiles cleanly, what about test cases? It looks
to me like you broke src/test/Makefile:

@@ -30,7 +30,7 @@ endif
endif
ifeq ($(with_openssl),yes)
ifneq (,$(filter ssl,$(PG_TEST_EXTRA)))
-SUBDIRS += ssl
+SUBDIRS += ssl crypto
endif
endif

Odd I am running the regression tests.

because there is no such subdirectory. Assuming that that's a
case of forgetting to "git add" the whole subdirectory, I still
don't much care for this implementation: the user should be able
to decide which subdirectories get run. Maybe more like

ifeq ($(with_openssl),yes)
ifneq (,$(filter ssl,$(PG_TEST_EXTRA)))
SUBDIRS += ssl
endif
+ifneq (,$(filter crypto,$(PG_TEST_EXTRA)))
+SUBDIRS += crypto
+endif
endif

There are no tests yet. I need to write those in TAP, and I am going to
wait until I have something more substantial to test. I do have a test
framework here I am using.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Hi,

On 2020-12-25 15:12:44 -0500, Bruce Momjian wrote:

There are no tests yet. I need to write those in TAP, and I am going to
wait until I have something more substantial to test. I do have a test
framework here I am using.

That's not great. It's not a tiny amount of code, and not having
anything to exercise it until some unclear future time means that other
people will have a hard time working anywhere in the vicinity. And that
might quite plausibly stay that way for v14?

Greetings,

Andres Freund

On Fri, Dec 25, 2020 at 03:12:08PM -0800, Andres Freund wrote:

Hi,

On 2020-12-25 15:12:44 -0500, Bruce Momjian wrote:

There are no tests yet. I need to write those in TAP, and I am going to
wait until I have something more substantial to test. I do have a test
framework here I am using.

That's not great. It's not a tiny amount of code, and not having
anything to exercise it until some unclear future time means that other
people will have a hard time working anywhere in the vicinity. And that
might quite plausibly stay that way for v14?

It might. It depends on who else wants to work on it, and who wants to
write tests. I did test Stephen's GCM changes.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Hi,

Thank you for developing a great feature. I tested it immediately.
The attached patch changes the value of the --file-encryption-keylen option of the initdb command to mandatory. No value is currently required.
I also changed the help message and the manual.

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Bruce Momjian [mailto:bruce@momjian.us]
Sent: Saturday, December 26, 2020 4:01 AM
To: Erik Rijkers <er@xs4all.nl>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Add key management system

On Fri, Dec 25, 2020 at 07:30:01PM +0100, Erik Rijkers wrote:

On 2020-12-25 16:19, Bruce Momjian wrote:

Add key management system
doc/src/sgml/database-encryption.sgml | 97 +++++

Attached are a few typos.

I also noticed that this option does not occur in the initdb --help:

-u --copy-encryption-keys

Was that deliberate?

No. :-( Attached patch applied. Thanks.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

On Sat, Dec 26, 2020 at 02:38:40PM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:

Hi,

Thank you for developing a great feature. I tested it immediately.
The attached patch changes the value of the --file-encryption-keylen option of the initdb command to mandatory. No value is currently required.
I also changed the help message and the manual.

Thank you, applied.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee