Proposed patch for key managment
Attached is a patch for key management, which will eventually be part of
cluster file encryption (CFE), called TDE (Transparent Data Encryption)
by Oracle. It is an update of Masahiko Sawada's patch from July 31:
/messages/by-id/CA+fd4k6RJwNvZTro3q2f5HSDd8HgyUc4CuY9U3e6Ran4C6TO4g@mail.gmail.com
Sawada-san did all the hard work, and I just redirected the patch. The
general outline of this CFE feature can be seen here:
https://wiki.postgresql.org/wiki/Transparent_Data_Encryption
The currently planned progression for this feature is to allow secure
retrieval of key encryption keys (KEK) outside of the database, then use
those to encrypt data keys that encrypt heap/index/tmpfile files.
This patch was changed from Masahiko Sawada's version by removing
references to non-cluster file encryption because having SQL-level keys
stored in this way was considered to have limited usefulness. I have
also remove references to SQL-level KEK rotation since that is best done
as a command-line too.
If most people approve of this general approach, and the design
decisions made, I would like to apply this in the next few weeks, but
this brings complications. The syntax added by this commit might not
provide a useful feature until PG 15, so how do we hide it from users.
I was thinking of not applying the doc changes (or commenting them out)
and commenting out the --help output.
Once this patch is applied, I would like to use the /dev/tty file
descriptor passing feature for the ssl_passphrase_command parameter, so
the ssl passphrase can be prompted from the terminal. (I am attaching
pass_fd.sh as a POC for how file descriptor handling works.) I would
also then write the KEK rotation command-line tool. After that, we can
start working on heap/index/tmpfile encryption using this patch as a
base.
Here is an example of the current patch in action, using a KEK like
'7CE7945EA2F7322536F103E4D7D91C21E52288089EF99D186B9A76D666EBCA66',
which is not echoed to the terminal:
$ initdb -R -c '/u/postgres/tmp/pass_fd.sh "Enter password:" %R'
The files belonging to this database system will be owned by user "postgres".
This user must also own the server process.
The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".
Data page checksums are disabled.
Cluster file encryption is enabled.
fixing permissions on existing directory /u/pgsql/data ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting default time zone ... America/New_York
creating configuration files ... ok
running bootstrap script ...
--> Enter password:ok
performing post-bootstrap initialization ...
--> Enter password:ok
syncing data to disk ... ok
initdb: warning: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.
Success. You can now start the database server using:
pg_ctl -D /u/pgsql/data -l logfile start
$ pg_ctl -R -l /u/pg/server.log start
waiting for server to start...
--> Enter password: done
server started
A non-matching passphrase looks like this:
$ pg_ctl -R -l /u/pg/server.log start
waiting for server to start...
--> Enter password: stopped waiting
pg_ctl: could not start server
Examine the log output.
$ tail -2 /u/pg/server.log
2020-12-02 16:32:34.045 EST [13056] FATAL: cluster passphrase does not match expected passphrase
2020-12-02 16:32:34.047 EST [13056] LOG: database system is shut down
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Dec 2, 2020 at 04:38:14PM -0500, Bruce Momjian wrote:
If most people approve of this general approach, and the design
decisions made, I would like to apply this in the next few weeks, but
this brings complications. The syntax added by this commit might not
provide a useful feature until PG 15, so how do we hide it from users.
I was thinking of not applying the doc changes (or commenting them out)
and commenting out the --help output.
Here is an updated patch to handle the new hash API introduced by
commit 87ae9691d2.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
On Fri, Dec 04, 2020 at 09:08:03PM -0500, Bruce Momjian wrote:
Here is an updated patch to handle the new hash API introduced by
commit 87ae9691d2.
+ if (!ossl_initialized)
+ {
+#ifdef HAVE_OPENSSL_INIT_SSL
+ OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
+#else
+ OPENSSL_config(NULL);
+ SSL_library_init();
+ SSL_load_error_strings();
+#endif
+ ossl_initialized = true;
This is a duplicate of what's done in be-secure-openssl.c, and it does
not strike me as a good idea to do that potentially twice.
git diff --check complains.
+extern bool pg_HMAC_SHA512(const uint8 *key,
+ const uint8 *in, int inlen,
+ uint8 *out);
I think that the split done in this patch makes the HMAC handling in
the core code messier:
- SCRAM makes use of HMAC internally, and we should try to use the
HMAC of OpenSSL if building with it even for SCRAM.
- For the first reason, I think that we should also have a fallback
implementation.
- This API layer should not depend directly on the SHA2 used (SCRAM
uses SHA256 with HMAC).
FWIW, I got plans to work on that once I am done with the business
around MD5 and OpenSSL.
The refactoring done with the ciphers moved from pgcrypto to
src/common/ should be a separate patch. In short, it would be good to
rework this patch and split it into pieces that are independently
useful. This would make the review much easier as well.
--
Michael
On Sat, 5 Dec 2020 at 11:08, Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Dec 2, 2020 at 04:38:14PM -0500, Bruce Momjian wrote:
If most people approve of this general approach, and the design
decisions made, I would like to apply this in the next few weeks, but
this brings complications. The syntax added by this commit might not
provide a useful feature until PG 15, so how do we hide it from users.
I was thinking of not applying the doc changes (or commenting them out)
and commenting out the --help output.Here is an updated patch to handle the new hash API introduced by
commit 87ae9691d2.
Thank you for updating the patch and moving forward!
I've tried to use this patch on my environment (FreeBSD 12.1) but it
doesn't work. I got the following error:
$ bin/initdb -D data -E UTF8 --no-locale -c'echo
"1234567890123456789012345678901234567890123456789012345678901234567890"'
The files belonging to this database system will be owned by user "masahiko".
This user must also own the server process.
The database cluster will be initialized with locale "C".
The default text search configuration will be set to "english".
Data page checksums are disabled.
Cluster file encryption is enabled.
creating directory data ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting default time zone ... Japan
creating configuration files ... ok
running bootstrap script ... child process was terminated by signal
10: Bus error
initdb: removing data directory "data"
The backtrace is:
(lldb) bt
* thread #1, name = 'postgres', stop reason = signal SIGBUS: hardware error
frame #0: 0x0000000000d3b074
postgres`ResourceArrayRemove(resarr=0x7f7f7f7f7f7f80df,
value=34383251232) at resowner.c:308:23
frame #1: 0x0000000000d3c0ef
postgres`ResourceOwnerForgetCryptoHash(owner=0x7f7f7f7f7f7f7f7f,
handle=34383251232) at resowner.c:1421:7
frame #2: 0x0000000000d77c54
postgres`pg_cryptohash_free(ctx=0x000000080166c720) at
cryptohash_openssl.c:228:3
* frame #3: 0x0000000000d6c065
postgres`kmgr_derive_keys(passphrase="1234567890123456789012345678901234567890123456789012345678901234567890\n",
passlen=71, enckey="\x01d
\x17(\x93\xa8id\xae\xa5\x86\x84\xb9Y>\xa84\x16\U00000085\U0000009d'\r\x11123456789012345678901234567890
1234567890123456789012345678901234567890\n",
mackey="0\x1f\xb4_eg\x95\x02\x9e2\x0e\xba\t\b^\x18\x90U\xa0\x8e\xaei\x01oYJL\xa4\xb3E\x97a,\x06h4\x9fX\x9eS\xb2\x81%^d\xa4\x01d
\x17(\x93\xa8id\xae\xa5\x86\x84\xb9Y>\xa84\x16\U00000085\U0000009d'\r\x1112345678901234567890123456789
01234567890123456789012345678901234567890\n") at kmgr_utils.c:239:2
frame #4: 0x0000000000d60810 postgres`BootStrapKmgr at kmgr.c:93:2
frame #5: 0x0000000000647fa1 postgres`BootStrapXLOG at xlog.c:5359:3
frame #6: 0x000000000066f034 postgres`AuxiliaryProcessMain(argc=7,
argv=0x00007fffffffcdb8) at bootstrap.c:450:4
frame #7: 0x00000000008e9cfb postgres`main(argc=8,
argv=0x00007fffffffcdb0) at main.c:201:3
frame #8: 0x000000000051010f postgres`_start(ap=<unavailable>,
cleanup=<unavailable>) at crt1.c:76:7
Investigating the issue, it seems we need to initialize the context
and the state with 0. The following change fixes this issue.
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index e5233daab6..a45c86fa67 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -81,6 +81,8 @@ pg_cryptohash_create(pg_cryptohash_type type)
return NULL;
}
+ memset(ctx, 0, sizeof(pg_cryptohash_ctx));
+ memset(state, 0, sizeof(pg_cryptohash_state));
ctx->data = state;
ctx->type = type;
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Dec 5, 2020 at 11:39:18AM +0900, Michael Paquier wrote:
On Fri, Dec 04, 2020 at 09:08:03PM -0500, Bruce Momjian wrote:
Here is an updated patch to handle the new hash API introduced by
commit 87ae9691d2.+ if (!ossl_initialized) + { +#ifdef HAVE_OPENSSL_INIT_SSL + OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL); +#else + OPENSSL_config(NULL); + SSL_library_init(); + SSL_load_error_strings(); +#endif + ossl_initialized = true; This is a duplicate of what's done in be-secure-openssl.c, and it does not strike me as a good idea to do that potentially twice.
Yeah, I kind of wondered about that. In fact, the code from the
original patch would not compile so I got this init code from somewhere
else. I have now removed it and it works fine. :-)
git diff --check complains.
Uh, can you be more specific? I don't see any output from that command.
+extern bool pg_HMAC_SHA512(const uint8 *key, + const uint8 *in, int inlen, + uint8 *out); I think that the split done in this patch makes the HMAC handling in the core code messier: - SCRAM makes use of HMAC internally, and we should try to use the HMAC of OpenSSL if building with it even for SCRAM. - For the first reason, I think that we should also have a fallback implementation. - This API layer should not depend directly on the SHA2 used (SCRAM uses SHA256 with HMAC). FWIW, I got plans to work on that once I am done with the business around MD5 and OpenSSL.
Uh, I just kind of kept all that code and didn't modify it. It would be
great if you can help me improve it. I will be using the hash code for
the command-line tool that alters the passphrase, so having that in
common/ does help me.
The refactoring done with the ciphers moved from pgcrypto to
src/common/ should be a separate patch. In short, it would be good to
Uh, I am kind of unclear exactly what was done there since I just took
that part of the patch unchanged.
rework this patch and split it into pieces that are independently
useful. This would make the review much easier as well.
I can break out the -R/file descriptor passing part as a separate patch,
and have the ssl_passphrase_command use that, but that's the only part I
know can be useful on its own.
Since the patch is large, I found a way to push the branch to git and
how to make a download link that tracks whatever I push to the 'key'
branch on my github account. Here is the updated patch link:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
--
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 5, 2020 at 12:15:13PM +0900, Masahiko Sawada wrote:
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c index e5233daab6..a45c86fa67 100644 --- a/src/common/cryptohash_openssl.c +++ b/src/common/cryptohash_openssl.c @@ -81,6 +81,8 @@ pg_cryptohash_create(pg_cryptohash_type type) return NULL; }+ memset(ctx, 0, sizeof(pg_cryptohash_ctx)); + memset(state, 0, sizeof(pg_cryptohash_state)); ctx->data = state; ctx->type = type;
OK, I worked with Sawada-san and added the attached patch. The updated
full patch is at the same URL: :-)
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
bzero.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index e5233daab6..02dec1fd1b 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -72,14 +72,15 @@ pg_cryptohash_create(pg_cryptohash_type type)
ctx = ALLOC(sizeof(pg_cryptohash_ctx));
if (ctx == NULL)
return NULL;
+ explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
state = ALLOC(sizeof(pg_cryptohash_state));
if (state == NULL)
{
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
FREE(ctx);
return NULL;
}
+ explicit_bzero(state, sizeof(pg_cryptohash_state));
ctx->data = state;
ctx->type = type;
@@ -97,8 +98,6 @@ pg_cryptohash_create(pg_cryptohash_type type)
if (state->evpctx == NULL)
{
- explicit_bzero(state, sizeof(pg_cryptohash_state));
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote:
OK, I worked with Sawada-san and added the attached patch. The updated
full patch is at the same URL: :-)https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
Oh, I see that you use SHA256 during firstboot, which is why you
bypass the resowner paths in cryptohash_openssl.c. Wouldn't it be
better to use IsBootstrapProcessingMode() then?
@@ -72,14 +72,15 @@ pg_cryptohash_create(pg_cryptohash_type type) ctx = ALLOC(sizeof(pg_cryptohash_ctx)); if (ctx == NULL) return NULL; + explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));state = ALLOC(sizeof(pg_cryptohash_state)); if (state == NULL) { - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); FREE(ctx); return NULL; } + explicit_bzero(state, sizeof(pg_cryptohash_state));
explicit_bzero() is used to give the guarantee that any sensitive data
gets cleaned up after an error or on free. So I think that your use
is incorrect here for an initialization.
if (state->evpctx == NULL)
{
- explicit_bzero(state, sizeof(pg_cryptohash_state));
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
And this original position is IMO more correct.
Anyway, at quick glance, the backtrace of upthread is indicating a
double-free with an attempt to free a resource owner that has been
already free'd.
--
Michael
On Sat, Dec 5, 2020 at 09:54:33PM +0900, Michael Paquier wrote:
On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote:
OK, I worked with Sawada-san and added the attached patch. The updated
full patch is at the same URL: :-)https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
Oh, I see that you use SHA256 during firstboot, which is why you
bypass the resowner paths in cryptohash_openssl.c. Wouldn't it be
better to use IsBootstrapProcessingMode() then?
I tried that, but we also use the resource references before the
resource system is started even in non-bootstrap mode. Maybe we should
be creating a resource owner for this, but it is so early in the boot
process I don't know if that is safe.
@@ -72,14 +72,15 @@ pg_cryptohash_create(pg_cryptohash_type type) ctx = ALLOC(sizeof(pg_cryptohash_ctx)); if (ctx == NULL) return NULL; + explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));state = ALLOC(sizeof(pg_cryptohash_state)); if (state == NULL) { - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); FREE(ctx); return NULL; } + explicit_bzero(state, sizeof(pg_cryptohash_state));explicit_bzero() is used to give the guarantee that any sensitive data
gets cleaned up after an error or on free. So I think that your use
is incorrect here for an initialization.
It turns out what we were missing was a clear of state->resowner in
cases where the resowner was null. I removed the bzero calls completely
and it now runs fine.
if (state->evpctx == NULL)
{
- explicit_bzero(state, sizeof(pg_cryptohash_state));
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),And this original position is IMO more correct.
Do we even need them?
Anyway, at quick glance, the backtrace of upthread is indicating a
double-free with an attempt to free a resource owner that has been
already free'd.
I think that is now fixed too. Updated patch at the same URL:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Sun, 6 Dec 2020 at 00:42, Bruce Momjian <bruce@momjian.us> wrote:
On Sat, Dec 5, 2020 at 09:54:33PM +0900, Michael Paquier wrote:
On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote:
OK, I worked with Sawada-san and added the attached patch. The updated
full patch is at the same URL: :-)https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
Oh, I see that you use SHA256 during firstboot, which is why you
bypass the resowner paths in cryptohash_openssl.c. Wouldn't it be
better to use IsBootstrapProcessingMode() then?I tried that, but we also use the resource references before the
resource system is started even in non-bootstrap mode. Maybe we should
be creating a resource owner for this, but it is so early in the boot
process I don't know if that is safe.@@ -72,14 +72,15 @@ pg_cryptohash_create(pg_cryptohash_type type) ctx = ALLOC(sizeof(pg_cryptohash_ctx)); if (ctx == NULL) return NULL; + explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));state = ALLOC(sizeof(pg_cryptohash_state)); if (state == NULL) { - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); FREE(ctx); return NULL; } + explicit_bzero(state, sizeof(pg_cryptohash_state));explicit_bzero() is used to give the guarantee that any sensitive data
gets cleaned up after an error or on free. So I think that your use
is incorrect here for an initialization.It turns out what we were missing was a clear of state->resowner in
cases where the resowner was null. I removed the bzero calls completely
and it now runs fine.if (state->evpctx == NULL)
{
- explicit_bzero(state, sizeof(pg_cryptohash_state));
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),And this original position is IMO more correct.
Do we even need them?
Anyway, at quick glance, the backtrace of upthread is indicating a
double-free with an attempt to free a resource owner that has been
already free'd.I think that is now fixed too. Updated patch at the same URL:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
Thank you for updating the patch!
I think we need explicit_bzero() also in freeing the keywrap context.
BTW, when we need -R option pg_ctl command to start the server, how
can we start it in the single-user mode?
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Dec 05, 2020 at 10:42:05AM -0500, Bruce Momjian wrote:
On Sat, Dec 5, 2020 at 09:54:33PM +0900, Michael Paquier wrote:
On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote:
if (state->evpctx == NULL)
{
- explicit_bzero(state, sizeof(pg_cryptohash_state));
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),And this original position is IMO more correct.
Do we even need them?
That's the intention to clean up things in a consistent way.
--
Michael
On Mon, Dec 7, 2020 at 09:30:03AM +0900, Masahiko Sawada wrote:
Thank you for updating the patch!
I think we need explicit_bzero() also in freeing the keywrap context.
pg_cryptohash_free() already has this:
explicit_bzero(state, sizeof(pg_cryptohash_state));
explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
Do we need more?
BTW, when we need -R option pg_ctl command to start the server, how
can we start it in the single-user mode?
I added code for that, but I hadn't tested it yet. Now that I tried it,
I realized that it is awkward to supply a file descriptor number (that
will be closed) from the command-line, so I added code and docs to allow
-1 to duplicate standard error, and it worked:
$ postgres --single -R -1 -D /u/pg/data
Enter password:
PostgreSQL stand-alone backend 14devel
backend> select 100;
1: ?column? (typeid = 23, len = 4, typmod = -1, byval = t)
----
1: ?column? = "100" (typeid = 23, len = 4, typmod = -1, byval = t)
----
Updated patch at the same URL:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Mon, Dec 7, 2020 at 11:03:30AM +0900, Michael Paquier wrote:
On Sat, Dec 05, 2020 at 10:42:05AM -0500, Bruce Momjian wrote:
On Sat, Dec 5, 2020 at 09:54:33PM +0900, Michael Paquier wrote:
On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote:
if (state->evpctx == NULL)
{
- explicit_bzero(state, sizeof(pg_cryptohash_state));
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),And this original position is IMO more correct.
Do we even need them?
That's the intention to clean up things in a consistent way.
Ah, I see your point. Added:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On 2 Dec 2020, at 22:38, Bruce Momjian <bruce@momjian.us> wrote:
Attached is a patch for key management, which will eventually be part of
cluster file encryption (CFE), called TDE (Transparent Data Encryption)
by Oracle.
The attackvector protected against seems to be operating systems users
(*without* any legitimate database access) gaining access to the data files.
Such an attacker would also gain access to postgresql.conf and therefore may
have the cluster passphrase command in case it's stored there. That would
imply the attacker is likely (or perhaps better phrased "not entirely
unlikely") to be able to run that command and decrypt the data *iff* there is
no interactive/2fa aspect to the passphrase command. Is that an accurate
interpretation? Do Oracle TDE et.al use a similar setup where an database
restart require manual intervention?
Admittedly I haven't read the other threads on the topic so if it's discussed
somewhere else then I'd appreciate a whacking with a cluestick.
A few other comments from skimming the patch:
+ is data encryption keys, specifically keys zero and one. Key zero is
+ the key uses to encrypt database heap and index files which are stored in
".. is the key used to .."?
+ matches the initdb-supplied passphrase. If this check fails, and the
+ the server will refuse to start.
Seems like something is missing, perhaps just s/and the//?
+ <ulink url="https://tools.ietf.org/html/rfc2315">RFC2315</ulink>.</simpara>
Tiny nitpick: turns out that RFC 2315 (with a space) is the correct spelling.
+ * are read-only. All wrapping and unwrapping key routines depends on
+ * the openssl library for now.
OpenSSL is a name so it should be cased as OpenSSL in text like this.
#include <openssl/ssl.h>
+#include <openssl/conf.h>
Why do we need this header in be-secure-openssl.c? There are no other changes
to that file?
+/* Define to 1 if you have the `OPENSSL_init_crypto' function. */
+#undef HAVE_OPENSSL_INIT_CRYPTO
This seems to be unused?
KmgrSaveCryptoKeys doesn't seem to be explicitly clamping down permissions on
the generated file, is that something we want for belt-and-suspender level
paranoia around keys? The same goes for read_one_keyfile etc.
Also, KmgrSaveCryptoKeys assumes that keys are stored in plain files which is
true for OpenSSL but won't necessarily be true for other crypto libraries.
Would it make sense to have an API in be-kmgr.c with the OpenSSL specifics in
be-kmgr-openssl.c like how the TLS backend support is written? We have spent a
lot effort in making the backend not assume a particular TLS library, it seems
a shame to step away from that here with a tight coupling. (yes, I am biased)
The same goes for src/common/cipher.c which wraps calls in ifdefs, why not just
have an thin wrapper API which cipher-openssl.c implements?
+ case 'K':
+ file_encryption_keylen = atoi(optarg);
+ break;
atoi() will return zero on failing to parse the keylen, where 0 implies
"disabled". Wouldn't it make sense to parse this with code which can't
silently fall back on disabling in case of users mistyping?
+ * Skip cryptographic keys. It's generally not good idea to copy the
".. not *a* good idea .."
+ pg_log_fatal("cluser passphrase referenced %%R, but -R not specified");
s/cluser/cluster/ (there are few copy-pasteos of that elsewhere too)
+ elog(ERROR, "too many cryptographic kes");
s/kes/keys/
+#ifndef CIPHER_H
+#define CIPHER_H
The risk for headerguard collision with non-PG headers seem quite high, maybe
PG_CIPHER_H would be better?
I will try to dive in a bit deeper over the holidays.
cheers ./daniel
Greetings,
* Daniel Gustafsson (daniel@yesql.se) wrote:
On 2 Dec 2020, at 22:38, Bruce Momjian <bruce@momjian.us> wrote:
Attached is a patch for key management, which will eventually be part of
cluster file encryption (CFE), called TDE (Transparent Data Encryption)
by Oracle.The attackvector protected against seems to be operating systems users
(*without* any legitimate database access) gaining access to the data files.
That isn't the only attack vector that this addresses (though it is one
that this is envisioned to help with- to wit, someone rsync'ing the DB
directory). TDE also helps with traditional data at rest requirements,
in environments where you don't trust the storage layer to handle that
(for whatever reason), and it's at least imagined that backups with
pg_basebackup would also be encrypted, helping protect against backup
theft.
There's, further, certainly no shortage of folks asking for this.
Such an attacker would also gain access to postgresql.conf and therefore may
have the cluster passphrase command in case it's stored there. That would
imply the attacker is likely (or perhaps better phrased "not entirely
unlikely") to be able to run that command and decrypt the data *iff* there is
no interactive/2fa aspect to the passphrase command. Is that an accurate
interpretation? Do Oracle TDE et.al use a similar setup where an database
restart require manual intervention?
This is very much an 'it depends' as other products have different
capabilities in this area. The most similar implementation to what is
being contemplated for PG today would be the big O's "tablespace" TDE,
which offers options of either "Password-based software keystores" (you
have to provide a password when you want to bring that tablespace
online), or "Auto-login software keystores" (there's a "system generated
password" that's used to encrypt the keystore, which seems to be
more-or-less the username and hostname of the system), or HSM based
options. As such, a DB restart might require manual intervention (if
the keystore is password based, or an HSM that requires manual
involvement) or it might not (auto-login keystore of some kind).
Admittedly I haven't read the other threads on the topic so if it's discussed
somewhere else then I'd appreciate a whacking with a cluestick.
I'm sure it was, but hopefully the above helps you avoid having to dig
through the very (very (very)) long threads on this topic.
Thanks,
Stephen
On Fri, Dec 4, 2020 at 10:32:45PM -0500, Bruce Momjian wrote:
I can break out the -R/file descriptor passing part as a separate patch,
and have the ssl_passphrase_command use that, but that's the only part I
know can be useful on its own.Since the patch is large, I found a way to push the branch to git and
how to make a download link that tracks whatever I push to the 'key'
branch on my github account. Here is the updated patch link:https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
I have made some good progress on the patch. I realized that pg_upgrade
can't just copy the keys from the old cluster --- they encrypt the user
heap/index files that are copied/linked by pg_upgrade, but also encrypt
the system tables, which initdb creates, so the keys have to be copied
at initdb bootstrap time --- I have added an option to do that. I also
realized that pg_upgrade will be starting/stopping the server, so I need
to add an option to pg_upgrade to allow that prompting. I can now
successfully pg_upgrade a cluster that uses cluster file encryption, and
keep the same keys. All at the same URL.
In addition I have completed the command-line tool to allow changing of
the cluster passphrase, which applies over the first diff; diff at:
https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff
My next task is to write a script for Yubikey authentication.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Hi, everyone
I have read the patch and did some simple tests. I'm not entirely sure
about some code segments; e.g.:In the BootStrapKmgr() we generate a data encryption key by:
key = generate_crypto_key(file_encryption_keylen);However, I found that the file_encryption_keylen is always 0 in bootstrap
mode because there exitst another variable bootstrap_file_encryption_keylen
in xlog.c and bootstrap.c.We get the REL/WAL key by KmgrGetKey() call and it works like:
return (const CryptoKey *) &(KmgrShmem->intlKeys[id]);But in bootstrap mode, the KmgrShmem are not assigned. So, if we want to
use it to encrypt something in bootstrap mode, I suggest we make the
following changes:
if ( in bootstrap mode)
return intlKeys[id]; // a static variable which contains key
else
reutrn (const CryptoKey *) &(KmgrShmem->intlKeys[id]);
--
There is no royal road to learning.
Highgo Software Co.
On Wed, Dec 9, 2020 at 10:34:59PM +0100, Daniel Gustafsson wrote:
On 2 Dec 2020, at 22:38, Bruce Momjian <bruce@momjian.us> wrote:
Attached is a patch for key management, which will eventually be part of
cluster file encryption (CFE), called TDE (Transparent Data Encryption)
by Oracle.The attackvector protected against seems to be operating systems users
(*without* any legitimate database access) gaining access to the data files.
Such an attacker would also gain access to postgresql.conf and therefore may
have the cluster passphrase command in case it's stored there. That would
imply the attacker is likely (or perhaps better phrased "not entirely
unlikely") to be able to run that command and decrypt the data *iff* there is
no interactive/2fa aspect to the passphrase command. Is that an accurate
interpretation? Do Oracle TDE et.al use a similar setup where an database
restart require manual intervention?
I have updated the docs to say "read" access more clearly:
The purpose of cluster file encryption is to prevent users with read
access to the directories used to store database files from being able to
access the data stored in those files. For example, when using cluster
file encryption, users who have read access to the cluster directories
for backup purposes will not be able to read the data stored in the
these files.
I already said "read access" in the later part of the paragraph, but
obviously it needs to be mentioned early too. If we need more text
here, please let me know.
As far as someone hijacking the passphrase command, that is a serious
risk. No matter how you do the authentication, even TFA, you are going
to have someone able to grab that passphrase as it comes out of the
script and passes to the server. It might help to store the script and
postgres binaries in a more secure location than the data, but I am not
sure how realistic that is. I can if you are using a NAS for data store
and have local binaries and passphrase script, that would be more secure
than putting the script on the NAS. Is that something we should explain?
Should we explicity say that there is no protection against write
access? What can someone with write access to PGDATA do if
postgresql.conf is not stored there? I don't know.
Admittedly I haven't read the other threads on the topic so if it's discussed
somewhere else then I'd appreciate a whacking with a cluestick.A few other comments from skimming the patch:
+ is data encryption keys, specifically keys zero and one. Key zero is + the key uses to encrypt database heap and index files which are stored in ".. is the key used to .."?
Fixed, thanks.
+ matches the initdb-supplied passphrase. If this check fails, and the + the server will refuse to start. Seems like something is missing, perhaps just s/and the//?
Fixed.
+ <ulink url="https://tools.ietf.org/html/rfc2315">RFC2315</ulink>.</simpara>
Tiny nitpick: turns out that RFC 2315 (with a space) is the correct spelling.
Fixed.
+ * are read-only. All wrapping and unwrapping key routines depends on + * the openssl library for now. OpenSSL is a name so it should be cased as OpenSSL in text like this.
Fixed, and fixed "depends".
#include <openssl/ssl.h>
+#include <openssl/conf.h>
Why do we need this header in be-secure-openssl.c? There are no other changes
to that file?
Not sure, removed, since it compiles fine without it.
+/* Define to 1 if you have the `OPENSSL_init_crypto' function. */ +#undef HAVE_OPENSSL_INIT_CRYPTO This seems to be unused?
Agreed, removed.
KmgrSaveCryptoKeys doesn't seem to be explicitly clamping down permissions on
the generated file, is that something we want for belt-and-suspender level
paranoia around keys? The same goes for read_one_keyfile etc.
Well, KmgrSaveCryptoKeys is calling BasicOpenFile, which is calling
BasicOpenFilePerm(fileName, fileFlags, pg_file_create_mode). The
question is whether we should honor the pg_file_create_mode specified on
the data directory, or make it tighter for these keys. The file is
already owner-rw-only by default:
$ ls -l
drwx------ 2 postgres postgres 4096 Dec 10 13:13 live/
$ cd live/
$ ls -l
total 8
-rw------- 1 postgres postgres 356 Dec 10 13:13 0
-rw------- 1 postgres postgres 356 Dec 10 13:13 1
If the key files were mixed in with a bunch of other files of lesser
value, like with Apache, I could see not honoring it, but for this case,
since all the files are of equal value, I think just honoring it makes
sense.
Also, KmgrSaveCryptoKeys assumes that keys are stored in plain files which is
true for OpenSSL but won't necessarily be true for other crypto libraries.
Would it make sense to have an API in be-kmgr.c with the OpenSSL specifics in
be-kmgr-openssl.c like how the TLS backend support is written? We have spent a
lot effort in making the backend not assume a particular TLS library, it seems
a shame to step away from that here with a tight coupling. (yes, I am biased)
I have no idea, sorry. I am mostly using the excellent routines
Sawada-san used in the original patch.
The same goes for src/common/cipher.c which wraps calls in ifdefs, why not just
have an thin wrapper API which cipher-openssl.c implements?
Same. I am open to it, sure, but I would need a patch.
+ case 'K': + file_encryption_keylen = atoi(optarg); + break; atoi() will return zero on failing to parse the keylen, where 0 implies "disabled". Wouldn't it make sense to parse this with code which can't silently fall back on disabling in case of users mistyping?
Good question, I found 43 references to atoi(optarg) in our code, so it
seems they all should be fixed, no? Not sure I want to be different here.
+ * Skip cryptographic keys. It's generally not good idea to copy the
".. not *a* good idea .."
Thank, fixed
+ pg_log_fatal("cluser passphrase referenced %%R, but -R not specified");
s/cluser/cluster/ (there are few copy-pasteos of that elsewhere too)
All fixed, thanks.
+ elog(ERROR, "too many cryptographic kes");
s/kes/keys/
Fixed.
+#ifndef CIPHER_H +#define CIPHER_H The risk for headerguard collision with non-PG headers seem quite high, maybe PG_CIPHER_H would be better?
Agreed, fixed.
I will try to dive in a bit deeper over the holidays.
Thanks. The diff URL has all the updates.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Dec 9, 2020 at 05:18:37PM -0500, Stephen Frost wrote:
Greetings,
* Daniel Gustafsson (daniel@yesql.se) wrote:
On 2 Dec 2020, at 22:38, Bruce Momjian <bruce@momjian.us> wrote:
Attached is a patch for key management, which will eventually be part of
cluster file encryption (CFE), called TDE (Transparent Data Encryption)
by Oracle.The attackvector protected against seems to be operating systems users
(*without* any legitimate database access) gaining access to the data files.That isn't the only attack vector that this addresses (though it is one
that this is envisioned to help with- to wit, someone rsync'ing the DB
directory). TDE also helps with traditional data at rest requirements,
in environments where you don't trust the storage layer to handle that
(for whatever reason), and it's at least imagined that backups with
pg_basebackup would also be encrypted, helping protect against backup
theft.There's, further, certainly no shortage of folks asking for this.
Can we flesh this out more in the docs? Any idea on wording compared to
what I have?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Thu, Dec 10, 2020 at 07:26:48PM +0800, Neil Chen wrote:
Hi, everyone
I have read the patch and did some simple tests. I'm not entirely sure
about some code segments; e.g.:In the BootStrapKmgr() we generate a data encryption key by:
key = generate_crypto_key(file_encryption_keylen);However, I found that the file_encryption_keylen is always 0 in bootstrap
mode because there exitst another variable bootstrap_file_encryption_keylen
in xlog.c and bootstrap.c.
Oh, good point; that is very helpful. I was relying on SetConfigOption
to set file_encryption_keylen, but that happens _after_ we create the
keys, so they were zero length. I have fixed this by passing
bootstrap_file_encryption_keylen to the boot routines. The diff URL has
the fix:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
We get the REL/WAL key by KmgrGetKey() call and it works like:
return (const CryptoKey *) &(KmgrShmem->intlKeys[id]);But in bootstrap mode, the KmgrShmem are not assigned. So, if we want to
use it to encrypt something in bootstrap mode, I suggest we make the
following changes:
if ( in bootstrap mode)
return intlKeys[id]; // a static variable which contains key
else
reutrn (const CryptoKey *) &(KmgrShmem->intlKeys[id]);
Yes, you are also correct here. I had not gotten to using KmgrGetKey
yet, but it clearly needs your suggestion, so have done that.
Thanks for your help.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Dec 9, 2020 at 08:40:50PM -0500, Bruce Momjian wrote:
My next task is to write a script for Yubikey authentication.
I know Craig Ringer wanted Yubikey support, which allows two-factor
authentication, so I have added it to the most recent patch by adding a
cluster_passphrase_command %d/directory parameter:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
You can also store the PIN in a file, so you don't need a PIN to be
entered by the user for each server start. Attached is the script I
with a PIN required. Here is a session:
$ initdb -K 256 -R -c '/u/postgres/tmp/pass_yubi.sh %R "%d"'
The files belonging to this database system will be owned by user "postgres".
This user must also own the server process.
The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".
Data page checksums are disabled.
Cluster file encryption is enabled.
fixing permissions on existing directory /u/pgsql/data ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting default time zone ... America/New_York
creating configuration files ... ok
running bootstrap script ...
Enter Yubikey PIN:
WARNING: The Yubikey can be locked and require a reset if too many pin
attempts fail. It is recommended to run this command manually and save
the passphrase in a secure location for possible recovery.
--> Enter Yubikey PIN:
ok
performing post-bootstrap initialization ...
--> Enter Yubikey PIN:
ok
syncing data to disk ... ok
initdb: warning: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.
Success. You can now start the database server using:
pg_ctl -D /u/pgsql/data -l logfile start
$ pg_ctl -R -l /u/pg/server.log start
waiting for server to start...
--> Enter Yubikey PIN:
done
server started
It even allows for passphrase rotation using my pg_altercpass tool with
this patch:
https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff
The Yubikey-encrypted passphrase is stored in the key directory, so the
encrypted passphrase stays with the data/WAL keys during passphrase
rotation:
$ pg_altercpass -R '/u/postgres/tmp/pass_yubi.sh %R "%d"' '/u/postgres/tmp/pass_yubi.sh %R "%d"'
--> Enter Yubikey PIN:
--> Enter Yubikey PIN:
WARNING: The Yubikey can be locked and require a reset if too many pin
attempts fail. It is recommended to run this command manually and save
the passphrase in a secure location for possible recovery.
--> Enter Yubikey PIN:
Yubikey PIN rotation has to be done using the Yubikey tool, and data/WAL
key rotation has to be done via switching to a standby, which hasn't
been implemented yet.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
On Fri, Dec 11, 2020 at 01:01:14PM -0500, Bruce Momjian wrote:
On Wed, Dec 9, 2020 at 08:40:50PM -0500, Bruce Momjian wrote:
My next task is to write a script for Yubikey authentication.
I know Craig Ringer wanted Yubikey support, which allows two-factor
authentication, so I have added it to the most recent patch by adding a
cluster_passphrase_command %d/directory parameter:https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
You can also store the PIN in a file, so you don't need a PIN to be
entered by the user for each server start.
Here is the output without requiring a PIN; attached is the script used:
++ initdb -K 256 -R -c '/u/postgres/tmp/pass_yubi_nopin.sh "%d"'
The files belonging to this database system will be owned by user "postgres".
This user must also own the server process.
The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".
Data page checksums are disabled.
Cluster file encryption is enabled.
fixing permissions on existing directory /u/pgsql/data ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting default time zone ... America/New_York
creating configuration files ... ok
running bootstrap script ... engine "pkcs11" set.
WARNING: The Yubikey can be locked and require a reset if too many pin
attempts fail. It is recommended to run this command manually and save
the passphrase in a secure location for possible recovery.
engine "pkcs11" set.
ok
performing post-bootstrap initialization ... engine "pkcs11" set.
ok
syncing data to disk ... ok
initdb: warning: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.
Success. You can now start the database server using:
pg_ctl -D /u/pgsql/data -l logfile start
++ pg_ctl -R -l /u/pg/server.log start
waiting for server to start... done
server started
++ pg_altercpass -R '/u/postgres/tmp/pass_yubi_nopin.sh "%d"' '/u/postgres/tmp/pass_yubi_nopin.sh "%d"'
engine "pkcs11" set.
engine "pkcs11" set.
WARNING: The Yubikey can be locked and require a reset if too many pin
attempts fail. It is recommended to run this command manually and save
the passphrase in a secure location for possible recovery.
engine "pkcs11" set.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
On Wed, Dec 2, 2020 at 04:38:14PM -0500, Bruce Momjian wrote:
Attached is a patch for key management, which will eventually be part of
cluster file encryption (CFE), called TDE (Transparent Data Encryption)
by Oracle. It is an update of Masahiko Sawada's patch from July 31:/messages/by-id/CA+fd4k6RJwNvZTro3q2f5HSDd8HgyUc4CuY9U3e6Ran4C6TO4g@mail.gmail.com
Sawada-san did all the hard work, and I just redirected the patch. The
general outline of this CFE feature can be seen here:https://wiki.postgresql.org/wiki/Transparent_Data_Encryption
The currently planned progression for this feature is to allow secure
retrieval of key encryption keys (KEK) outside of the database, then use
those to encrypt data keys that encrypt heap/index/tmpfile files.
...
If most people approve of this general approach, and the design
decisions made, I would like to apply this in the next few weeks, but
this brings complications. The syntax added by this commit might not
provide a useful feature until PG 15, so how do we hide it from users.
I was thinking of not applying the doc changes (or commenting them out)
and commenting out the --help output.
I am getting close to applying these patches, probably this week. The
patches are cumulative:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff
I do have a few questions:
Why is KmgrShmemData a struct, when it only has a single member? Are
all shared memory areas structs?
Should pg_altercpass be using fsync's for directory renames?
Can anyone test this on Windows, particularly -R handling?
What testing infrastructure should this have?
There are a few shell script I should include to show how to create
commands. Where should they be stored? /contrib module?
Are people okay with having the feature enabled, but invisible
since the docs and --help output are missing? When we enable
ssl_passphrase_command to prompt from the terminal, some of the
command-line options will be useful.
Do people like the command-letter choices?
I called the alter passphrase utility pg_altercpass. I could
have called it pg_clusterpass, but I wanted to highlight it is
only for changing the passphrase, not for creating them.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Mon, Dec 14, 2020 at 06:06:15PM -0500, Bruce Momjian wrote:
I am getting close to applying these patches, probably this week. The
patches are cumulative:https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff
I strongly object to a commit based on the current state of the patch.
Based on my lookup of the patches you are referring to, I see a couple
of things that should be splitted up and refactored properly before
thinking about the core part of the patch (FWIW, I don't have much
thoughts to offer about the core part because I haven't really thought
about it, but it does not prevent to do a correct refactoring). Here
are some notes:
- The code lacks a lot of comments IMO. Why is retrieve_passphrase()
doing what it does? It seems to me that pg_altercpass needs a large
brushup.
- There are no changes to src/tools/msvc/. Seeing the diffs in
src/common/, this stuff breaks Windows builds.
- The HMAC split is terrible, as mentioned upthread. I think that you
would need to extract this stuff as a separate patch, and not add more
mess to the existing mess (SCRAM uses its own HMAC with SHA256, which
is already wrong). We can and should have a fallback implementation,
because that's easy to do. And we need to have the fallback
implementation depend on the contents of cryptohash.c (in a
src/common/hmac.c), while the OpenSSL portion requires a
hmac_openssl.c where you can choose the hash type based on
pg_cryptohash_type. So ossl_HMAC_SHA512() does not do the right
thing. APIs flexible enough to allow a new SSL library to plug into
this stuff are required.
- Not much a fan of the changes done in cryptohash.c for the resource
owners as well. It also feels like this could be thought as something
directly for resowner.c.
- The cipher split also should be done in its own patch, and reviewed
on its own. There is a mix of dependencies between non-OpenSSL and
OpenSSL code paths, making the pluggin of a new SSL library harder to
do. In short, this requires proper API design, which is not something
we have here. There are also no changes in pgcrypto for that stuff.
I do have a few questions:
That looks like a lot of things to sort out as well.
Can anyone test this on Windows, particularly -R handling?
What testing infrastructure should this have?
Using TAP tests would be adapted for those two points.
There are a few shell script I should include to show how to create
commands. Where should they be stored? /contrib module?
Why are these needed. Is it a matter of documentation?
Are people okay with having the feature enabled, but invisible
since the docs and --help output are missing? When we enable
ssl_passphrase_command to prompt from the terminal, some of the
command-line options will be useful.
You are suggesting to enable the feature by default, make it invisible
to the users and leave it undocumented? Is there something I missing
here?
Do people like the command-letter choices?
I called the alter passphrase utility pg_altercpass. I could
have called it pg_clusterpass, but I wanted to highlight it is
only for changing the passphrase, not for creating them.
I think that you should attach such patches directly to the emails
sent to pgsql-hackers, if those github links disappear for some
reason, it would become impossible to refer to see what has been
discussed here.
+/*
+ * We have to use postgres.h not postgres_fe.h here, because there's so much
+ * backend-only stuff in the kmgr include files we need. But we need a
+ * frontend-ish environment otherwise. Hence this ugly hack.
+ */
+#define FRONTEND 1
+
+#include "postgres.h"
I would advise to really understand why this happens and split up the
backend and frontend parts cleanly. This style ought to be avoided as
much as possible.
@@ -95,9 +101,9 @@ pg_cryptohash_create(pg_cryptohash_type type)
if (state->evpctx == NULL)
{
+#ifndef FRONTEND
explicit_bzero(state, sizeof(pg_cryptohash_state));
explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-#ifndef FRONTEND
I think that this change is incorrect. Any clean up of memory should
be done for the frontend *and* the backend.
+#ifdef USE_OPENSSL
+ ctx = (PgCipherCtx *) palloc0(sizeof(PgCipherCtx));
+
+ ctx->encctx = ossl_cipher_ctx_create(cipher, key, klen, true);
+ ctx->decctx = ossl_cipher_ctx_create(cipher, key, klen, false);
+#endif
There is a cipher_openssl.c and a cipher.c that includes USE_OPENSSL.
This requires a cleaner split IMO. We should avoid as much as
possible OpenSSL-specific code paths in files part of src/common/ when
not building with OpenSSL. So this is now a mixed bag of
dependencies.
--
Michael
Hi, Bruce
I read your question and here are some of my thoughts.
Why is KmgrShmemData a struct, when it only has a single member?
Are
all shared memory areas structs?
Yes, all areas created by ShmemInitStruct() are structs. I think the
significance of using struct is that it delimits an "area" to store the
KMS related things, which makes our memory management more clear and
unified.
What testing infrastructure should this have?
There are a few shell script I should include to show how to create
commands. Where should they be stored? /contrib module?
Are people okay with having the feature enabled, but invisible
since the docs and --help output are missing? When we enable
ssl_passphrase_command to prompt from the terminal, some of the
command-line options will be useful.
Since our implementation is not in contrib, I don't think we should put the
script there. Maybe we can refer to postgresql.conf.sample?
Actually, I wonder whether we can add the UDK(user data encryption key) to
the first version of KMS, which can be provided to plug-ins such as
pgsodium. In this way, users can at least use it to a certain extent.
Also, I have tested some KMS functionalities by adding very simple TDE
logic. In the meantime, I found some confusion in the following places:
1. Previously, we added a variable bootstrap_keys_wrap that is used for
encryption during initdb. However, since we save the "wrapped" key, we need
to use a global KEK that can be accessed in boot mode to unwrap it before
use... I don't know if that's good. To make it simple, I modified the
bootstrap_keys_wrap to store the "unwrapped" key so that the encryption
function can get it correctly. (The variable name should be changed
accordingly).
2. I tried to add support for AES_CTR mode, and the code for encrypting
buffer blocks. In the process I found that in pg_cipher_ctx_create, the key
length is declared as "byte". However, in the CryptoKey structure, the
length is stored as "bit", which leads me to use a form similar to
Key->klen / 8 when I call this function. Maybe we should unify the two to
avoid unnecessary confusion.
3. This one is not a problem/bug. I have some doubts about the length of
data encryption blocks. For the page, we do not encrypt the PageHeaderData,
which is 192 bit long. By default, the page size is 8K(65536 bits), so the
length of the data we want to encrypt is 65344 bits. This number can't be
divisible by 128 and 192 keys and 256 bits long keys. Will this cause a
problem?
Here is a simple patch that I wrote with references to Sawada's previous
TDE works, hope it can help you.
Thanks.
--
There is no royal road to learning.
HighGo Software Co.
Attachments:
encrypt_buffer.diffapplication/octet-stream; name=encrypt_buffer.diffDownload
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index a664ecf494..2197bd3a14 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -1025,6 +1025,7 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
true);
RelationOpenSmgr(rel);
+ PageEncryptInplace(page, MAIN_FORKNUM, lastblock);
PageSetChecksumInplace(page, lastblock);
smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf.data, false);
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 39e33763df..498a18dd33 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -326,6 +326,7 @@ end_heap_rewrite(RewriteState state)
true);
RelationOpenSmgr(state->rs_new_rel);
+ PageEncryptInplace(state->rs_buffer, MAIN_FORKNUM, state->rs_blockno);
PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
@@ -690,6 +691,7 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
*/
RelationOpenSmgr(state->rs_new_rel);
+ PageEncryptInplace(page, MAIN_FORKNUM, state->rs_blockno);
PageSetChecksumInplace(page, state->rs_blockno);
smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index c4f22f1c69..f55f66110e 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -175,6 +175,7 @@ btbuildempty(Relation index)
* XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record. Therefore, we need
* this even when wal_level=minimal.
*/
+ PageEncryptInplace(metapage, INIT_FORKNUM, BTREE_METAPAGE);
PageSetChecksumInplace(metapage, BTREE_METAPAGE);
smgrwrite(index->rd_smgr, INIT_FORKNUM, BTREE_METAPAGE,
(char *) metapage, true);
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 8730de25ed..729551d67b 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -665,6 +665,7 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
true);
}
+ PageEncryptInplace(page, MAIN_FORKNUM, blkno);
PageSetChecksumInplace(page, blkno);
/*
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index e4508a2b92..4a0a24a5a7 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -168,6 +168,7 @@ spgbuildempty(Relation index)
* of their existing content when the corresponding create records are
* replayed.
*/
+ PageEncryptInplace(page, INIT_FORKNUM, SPGIST_METAPAGE_BLKNO);
PageSetChecksumInplace(page, SPGIST_METAPAGE_BLKNO);
smgrwrite(index->rd_smgr, INIT_FORKNUM, SPGIST_METAPAGE_BLKNO,
(char *) page, true);
@@ -177,6 +178,7 @@ spgbuildempty(Relation index)
/* Likewise for the root page. */
SpGistInitPage(page, SPGIST_LEAF);
+ PageEncryptInplace(page, INIT_FORKNUM, SPGIST_ROOT_BLKNO);
PageSetChecksumInplace(page, SPGIST_ROOT_BLKNO);
smgrwrite(index->rd_smgr, INIT_FORKNUM, SPGIST_ROOT_BLKNO,
(char *) page, true);
@@ -186,6 +188,7 @@ spgbuildempty(Relation index)
/* Likewise for the null-tuples root page. */
SpGistInitPage(page, SPGIST_LEAF | SPGIST_NULLS);
+ PageEncryptInplace(page, INIT_FORKNUM, SPGIST_NULL_BLKNO);
PageSetChecksumInplace(page, SPGIST_NULL_BLKNO);
smgrwrite(index->rd_smgr, INIT_FORKNUM, SPGIST_NULL_BLKNO,
(char *) page, true);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a074dea53e..b95ed8eb0b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -61,6 +61,7 @@
#include "replication/walreceiver.h"
#include "replication/walsender.h"
#include "storage/bufmgr.h"
+#include "storage/encryption.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/large_object.h"
@@ -939,6 +940,7 @@ static void ReadControlFile(void);
static char *str_time(pg_time_t tnow);
static void SetPromoteIsTriggered(void);
static bool CheckForStandbyTrigger(void);
+static void XLogWritePages(char *from, Size nbytes, uint32 startoffset);
#ifdef WAL_DEBUG
static void xlog_outrec(StringInfo buf, XLogReaderState *record);
@@ -2529,41 +2531,33 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
{
char *from;
Size nbytes;
- Size nleft;
- int written;
/* OK to write the page(s) */
from = XLogCtl->pages + startidx * (Size) XLOG_BLCKSZ;
nbytes = npages * (Size) XLOG_BLCKSZ;
- nleft = nbytes;
- do
- {
- errno = 0;
- pgstat_report_wait_start(WAIT_EVENT_WAL_WRITE);
- written = pg_pwrite(openLogFile, from, nleft, startoffset);
- pgstat_report_wait_end();
- if (written <= 0)
- {
- char xlogfname[MAXFNAMELEN];
- int save_errno;
- if (errno == EINTR)
- continue;
+ if (DataEncryptionEnabled())
+ {
+ int i;
- save_errno = errno;
- XLogFileName(xlogfname, ThisTimeLineID, openLogSegNo,
- wal_segment_size);
- errno = save_errno;
- ereport(PANIC,
- (errcode_for_file_access(),
- errmsg("could not write to log file %s "
- "at offset %u, length %zu: %m",
- xlogfname, startoffset, nleft)));
+ /* Encrypt xlog pages one by one */
+ for (i = 0; i < npages; i++)
+ {
+ char *buftowrite;
+ Size nwrite = Min(nbytes, XLOG_BLCKSZ);
+
+ buftowrite = EncryptXLog(from, nwrite, openLogSegNo,
+ startoffset);
+ XLogWritePages(buftowrite, nwrite, startoffset);
+ startoffset += nwrite;
+ from += XLOG_BLCKSZ;
}
- nleft -= written;
- from += written;
- startoffset += written;
- } while (nleft > 0);
+ }
+ else
+ {
+ XLogWritePages(from, npages * (Size) XLOG_BLCKSZ, startoffset);
+ startoffset += npages * (Size) XLOG_BLCKSZ;
+ }
npages = 0;
@@ -2680,6 +2674,46 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
}
}
+/*
+ * Write XLOG pages starting from 'startoffset'.
+ */
+static void
+XLogWritePages(char *from, Size nbytes, uint32 startoffset)
+{
+ Size nleft;
+ int written;
+
+ nleft = nbytes;
+ do
+ {
+ errno = 0;
+ pgstat_report_wait_start(WAIT_EVENT_WAL_WRITE);
+ written = pg_pwrite(openLogFile, from, nleft, startoffset);
+ pgstat_report_wait_end();
+ if (written <= 0)
+ {
+ char xlogfname[MAXFNAMELEN];
+ int save_errno;
+
+ if (errno == EINTR)
+ continue;
+
+ save_errno = errno;
+ XLogFileName(xlogfname, ThisTimeLineID, openLogSegNo,
+ wal_segment_size);
+ errno = save_errno;
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not write to log file %s "
+ "at offset %u, length %zu: %m",
+ xlogfname, startoffset, nleft)));
+ }
+ nleft -= written;
+ from += written;
+ startoffset += written;
+ } while (nleft > 0);
+}
+
/*
* Record the LSN for an asynchronous transaction commit/abort
* and nudge the WALWriter if there is work for it to do.
@@ -12049,6 +12083,16 @@ retry:
xlogreader->seg.ws_tli = curFileTLI;
+ /*
+ * Decrypt read record so that we can validate page both short header
+ * and possibly long header.
+ */
+ if (DataEncryptionEnabled())
+ {
+ DecryptXLog(readBuf, XLOG_BLCKSZ, readSegNo, readOff);
+ xlogreader->encrypted = false;
+ }
+
/*
* Check the page header immediately, so that we can retry immediately if
* it's not valid. This may seem unnecessary, because XLogReadRecord()
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index a63ad8cfd0..3ad57e1a85 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -29,6 +29,7 @@
#ifndef FRONTEND
#include "miscadmin.h"
+#include "storage/encryption.h"
#include "pgstat.h"
#include "utils/memutils.h"
#endif
@@ -615,6 +616,16 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
/* we can be sure to have enough WAL available, we scrolled back */
Assert(readLen == XLOG_BLCKSZ);
+#ifndef FRONTEND
+ if (state->encrypted)
+ {
+ uint32 off = XLogSegmentOffset(targetSegmentPtr, state->segcxt.ws_segsize);
+
+ DecryptXLog(state->readBuf, XLOG_BLCKSZ, targetSegNo, off);
+ state->encrypted = false;
+ }
+#endif
+
if (!XLogReaderValidatePageHeader(state, targetSegmentPtr,
state->readBuf))
goto err;
@@ -650,6 +661,14 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
goto err;
}
+#ifndef FRONTEND
+ if (state->encrypted)
+ {
+ DecryptXLog(state->readBuf, XLOG_BLCKSZ, targetSegNo, targetPageOff);
+ state->encrypted = false;
+ }
+#endif
+
/*
* Now that we know we have the full header, validate it.
*/
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 32a3099c1f..463ec31445 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -25,6 +25,7 @@
#include "access/xlogutils.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "storage/encryption.h"
#include "storage/smgr.h"
#include "utils/guc.h"
#include "utils/hsearch.h"
@@ -946,6 +947,9 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
&errinfo))
WALReadRaiseError(&errinfo);
+ if (DataEncryptionEnabled())
+ state->encrypted = true;
+
/* number of valid bytes in the buffer */
return count;
}
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index d538f25726..e8f605030f 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -443,7 +443,7 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
smgrread(src, forkNum, blkno, buf.data);
- if (!PageIsVerifiedExtended(page, blkno,
+ if (!PageIsVerifiedExtended(page, forkNum, blkno,
PIV_LOG_WARNING | PIV_REPORT_STAT))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
diff --git a/src/backend/crypto/kmgr.c b/src/backend/crypto/kmgr.c
index 9b3d026fc4..4990b42f4a 100644
--- a/src/backend/crypto/kmgr.c
+++ b/src/backend/crypto/kmgr.c
@@ -51,7 +51,7 @@ static KmgrShmemData *KmgrShmem;
char *cluster_passphrase_command = NULL;
int file_encryption_keylen = 0;
-CryptoKey boostrap_keys_wrap[KMGR_MAX_INTERNAL_KEYS];
+CryptoKey bootstrap_keys_wrap[KMGR_MAX_INTERNAL_KEYS];
extern char *bootstrap_old_key_datadir;
extern int bootstrap_file_encryption_keylen;
@@ -92,6 +92,7 @@ BootStrapKmgr(void)
else
{
char live_path[MAXPGPATH];
+ CryptoKey keys[KMGR_MAX_INTERNAL_KEYS];
if (mkdir(LIVE_KMGR_DIR, pg_dir_create_mode) < 0)
ereport(ERROR,
@@ -99,7 +100,7 @@ BootStrapKmgr(void)
errmsg("could not create cluster file encryption directory \"%s\": %m",
LIVE_KMGR_DIR)));
- memset(boostrap_keys_wrap, 0, sizeof(boostrap_keys_wrap));
+ memset(bootstrap_keys_wrap, 0, sizeof(bootstrap_keys_wrap));
/* bzero keys on exit */
on_proc_exit(bzeroKmgrKeys, 0);
@@ -124,12 +125,10 @@ BootStrapKmgr(void)
/* Wrap all data encryption keys by key encryption key */
for (int id = 0; id < KMGR_MAX_INTERNAL_KEYS; id++)
{
- CryptoKey *key;
-
/* generate a data encryption key */
- key = generate_crypto_key(bootstrap_file_encryption_keylen);
+ memcpy(&bootstrap_keys_wrap[id], generate_crypto_key(bootstrap_file_encryption_keylen), sizeof(CryptoKey));
- if (!kmgr_wrap_key(ctx, key, &(boostrap_keys_wrap[id])))
+ if (!kmgr_wrap_key(ctx, &bootstrap_keys_wrap[id], &(keys[id])))
{
pg_free_keywrap_ctx(ctx);
elog(ERROR, "failed to wrap data encryption key");
@@ -137,7 +136,7 @@ BootStrapKmgr(void)
}
/* Save data encryption keys to the disk */
- KmgrSaveCryptoKeys(LIVE_KMGR_DIR, boostrap_keys_wrap);
+ KmgrSaveCryptoKeys(LIVE_KMGR_DIR, keys);
pg_free_keywrap_ctx(ctx);
}
@@ -256,7 +255,7 @@ KmgrGetKey(int id)
Assert(id < KMGR_MAX_INTERNAL_KEYS);
return (const CryptoKey *) (IsBootstrapProcessingMode() ?
- &(boostrap_keys_wrap[id]) : &(KmgrShmem->intlKeys[id]));
+ &(bootstrap_keys_wrap[id]) : &(KmgrShmem->intlKeys[id]));
}
/* Generate an empty CryptoKey */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 2eb19ad293..84c4bdee09 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -77,6 +77,7 @@
#include "replication/walsender.h"
#include "replication/walsender_private.h"
#include "storage/condition_variable.h"
+#include "storage/encryption.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/pmsignal.h"
@@ -858,6 +859,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
XLByteToSeg(targetPagePtr, segno, state->segcxt.ws_segsize);
CheckXLogRemoved(segno, state->seg.ws_tli);
+ /* Decrypt xlog page if needed */
+ if (DataEncryptionEnabled())
+ state->encrypted = true;
+
return count;
}
diff --git a/src/backend/storage/Makefile b/src/backend/storage/Makefile
index 8376cdfca2..3f99e317e1 100644
--- a/src/backend/storage/Makefile
+++ b/src/backend/storage/Makefile
@@ -8,6 +8,6 @@ subdir = src/backend/storage
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-SUBDIRS = buffer file freespace ipc large_object lmgr page smgr sync
+SUBDIRS = buffer encryption file freespace ipc large_object lmgr page smgr sync
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index ad0d1a9abc..3003090058 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -918,7 +918,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
}
/* check for garbage data */
- if (!PageIsVerifiedExtended((Page) bufBlock, blockNum,
+ if (!PageIsVerifiedExtended((Page) bufBlock, forkNum, blockNum,
PIV_LOG_WARNING | PIV_REPORT_STAT))
{
if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)
@@ -2793,12 +2793,15 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
*/
bufBlock = BufHdrGetBlock(buf);
+ bufToWrite = PageEncryptCopy((Page) bufBlock, buf->tag.forkNum,
+ buf->tag.blockNum);
+
/*
* Update page checksum if desired. Since we have only shared lock on the
* buffer, other processes might be updating hint bits in it, so we must
* copy the page to private storage if we do checksumming.
*/
- bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum);
+ bufToWrite = PageSetChecksumCopy((Page) bufToWrite, buf->tag.blockNum);
if (track_io_timing)
INSTR_TIME_SET_CURRENT(io_start);
@@ -3275,6 +3278,9 @@ FlushRelationBuffers(Relation rel)
localpage = (char *) LocalBufHdrGetBlock(bufHdr);
+ PageEncryptInplace(localpage, bufHdr->tag.forkNum,
+ bufHdr->tag.blockNum);
+
/* Setup error traceback support for ereport() */
errcallback.callback = local_buffer_write_error_callback;
errcallback.arg = (void *) bufHdr;
diff --git a/src/backend/storage/encryption/Makefile b/src/backend/storage/encryption/Makefile
new file mode 100644
index 0000000000..b9c722104c
--- /dev/null
+++ b/src/backend/storage/encryption/Makefile
@@ -0,0 +1,19 @@
+#-------------------------------------------------------------------------
+#
+# Makefile--
+# Makefile for storage/encryption
+#
+# IDENTIFICATION
+# src/backend/storage/encryption/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/backend/storage/encryption/
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+ bufenc.o \
+ walenc.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/storage/encryption/bufenc.c b/src/backend/storage/encryption/bufenc.c
new file mode 100644
index 0000000000..ed0242d857
--- /dev/null
+++ b/src/backend/storage/encryption/bufenc.c
@@ -0,0 +1,93 @@
+/*-------------------------------------------------------------------------
+ *
+ * bufenc.c
+ *
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/storage/encryption/bufenc.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "storage/bufpage.h"
+#include "storage/encryption.h"
+#include "storage/fd.h"
+#include "crypto/kmgr.h"
+
+static uint8 buf_encryption_iv[PG_AES_IV_SIZE];
+static void set_buffer_encryption_iv(Page page, BlockNumber blocknum);
+
+void
+EncryptBufferBlock(BlockNumber blocknum, Page page)
+{
+ PgCipherCtx *relkeyctx;
+ const CryptoKey *relkey;
+ uint8 *key;
+ int outlen;
+
+ relkey = KmgrGetKey(KMGR_REL_KEY_ID);
+ key = (uint8 *) pstrdup((const char *) relkey->key);
+ relkeyctx = pg_cipher_ctx_create(PG_CIPHER_AES_CTR, key, relkey->klen / 8);
+
+ set_buffer_encryption_iv(page, blocknum);
+ pg_cipher_encrypt(relkeyctx,
+ (uint8*)(page + PageEncryptOffset),
+ SizeOfPageEncryption,
+ (uint8*)(page + PageEncryptOffset),
+ &outlen,
+ buf_encryption_iv);
+
+ Assert(SizeOfPageEncryption == outlen);
+}
+
+void
+DecryptBufferBlock(BlockNumber blocknum, Page page)
+{
+ PgCipherCtx *relkeyctx;
+ const CryptoKey *relkey;
+ uint8 *key;
+ int outlen;
+
+ relkey = KmgrGetKey(KMGR_REL_KEY_ID);
+ key = (uint8 *) pstrdup((const char *) relkey->key);
+ relkeyctx = pg_cipher_ctx_create(PG_CIPHER_AES_CTR, key, relkey->klen / 8);
+
+ set_buffer_encryption_iv(page, blocknum);
+ pg_cipher_decrypt(relkeyctx,
+ (uint8*)(page + PageEncryptOffset),
+ SizeOfPageEncryption,
+ (uint8*)(page + PageEncryptOffset),
+ &outlen,
+ buf_encryption_iv);
+
+ Assert(SizeOfPageEncryption == outlen);
+}
+
+/*
+ * Nonce for buffer encryption consists of page lsn, block number
+ * and counter. The counter is a counter value for CTR cipher mode.
+ */
+static void
+set_buffer_encryption_iv(Page page, BlockNumber blocknum)
+{
+ uint8 *p = buf_encryption_iv;
+
+ MemSet(buf_encryption_iv, 0, PG_AES_IV_SIZE);
+
+ /* page lsn (8 byte) */
+ memcpy(p, &((PageHeader) page)->pd_lsn, sizeof(PageXLogRecPtr));
+ p += sizeof(PageXLogRecPtr);
+
+ /* block number (4 byte) */
+ memcpy(p, &blocknum, sizeof(BlockNumber));
+ p += sizeof(BlockNumber);
+
+ /* Space for counter (4 byte) */
+ memset(p, 0, ENC_BUFFER_AES_COUNTER_SIZE);
+ p += ENC_BUFFER_AES_COUNTER_SIZE;
+}
+
diff --git a/src/backend/storage/encryption/walenc.c b/src/backend/storage/encryption/walenc.c
new file mode 100644
index 0000000000..97cecab558
--- /dev/null
+++ b/src/backend/storage/encryption/walenc.c
@@ -0,0 +1,110 @@
+/*-------------------------------------------------------------------------
+ *
+ * walenc.c
+ *
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/storage/encryption/walenc.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "access/xlog.h"
+#include "access/xlog_internal.h"
+#include "storage/encryption.h"
+#include "crypto/kmgr.h"
+
+static uint8 wal_encryption_iv[PG_AES_IV_SIZE];
+static char wal_encryption_buf[XLOG_BLCKSZ];
+
+static void
+set_wal_encryption_iv(XLogSegNo segment, uint32 offset)
+{
+ uint8 *p = wal_encryption_iv;
+ uint32 pageno = offset / XLOG_BLCKSZ;
+
+ /* Space for counter (4 byte) */
+ memset(p, 0, ENC_WAL_AES_COUNTER_SIZE);
+ p += ENC_WAL_AES_COUNTER_SIZE;
+
+ /* Segment number (8 byte) */
+ memcpy(p, &segment, sizeof(XLogSegNo));
+ p += sizeof(XLogSegNo);
+
+ /* Page number within a WAL segment (4 byte) */
+ memcpy(p, &pageno, sizeof(uint32));
+}
+
+/*
+ * Copy the contents of WAL page and encrypt it. Returns the copied and
+ * encrypted WAL page.
+ */
+char *
+EncryptXLog(char *page, Size nbytes, XLogSegNo segno, uint32 offset)
+{
+#if 0
+ const CryptoKey *walkey;
+ PgCipherCtx *walkeyctx = NULL;
+ uint8 *key;
+ int outlen;
+
+ Assert(nbytes <= XLOG_BLCKSZ);
+
+ walkey = KmgrGetKey(KMGR_WAL_KEY_ID);
+ key = (uint8 *) pstrdup((const char *) walkey->key);
+ walkeyctx = pg_cipher_ctx_create(PG_CIPHER_AES_CTR, key, walkey->klen / 8);
+
+ set_wal_encryption_iv(segno, offset);
+
+ /* Copy to work buffer */
+ memcpy(wal_encryption_buf, page, XLOG_BLCKSZ);
+
+ pg_cipher_encrypt(walkeyctx,
+ (uint8*)(wal_encryption_buf + XLogEncryptionOffset),
+ nbytes - XLogEncryptionOffset,
+ (uint8*)(wal_encryption_buf + XLogEncryptionOffset),
+ &outlen,
+ wal_encryption_iv);
+
+ Assert(outlen == (nbytes - XLogEncryptionOffset));
+ return wal_encryption_buf;
+#endif
+ return page;
+}
+
+/*
+ * Decrypt a WAL page and return. Unlike EncryptXLog, this function encrypt
+ * the given buffer directly.
+ */
+void
+DecryptXLog(char *page, Size nbytes, XLogSegNo segno, uint32 offset)
+{
+#if 0
+ const CryptoKey *walkey;
+ PgCipherCtx *walkeyctx = NULL;
+ uint8 *key;
+ int outlen;
+
+ Assert(nbytes <= XLOG_BLCKSZ);
+
+ walkey = KmgrGetKey(KMGR_WAL_KEY_ID);
+ key = (uint8 *) pstrdup((const char *) walkey->key);
+ walkeyctx = pg_cipher_ctx_create(PG_CIPHER_AES_CTR, key, walkey->klen / 8);
+
+ set_wal_encryption_iv(segno, offset);
+
+ pg_cipher_decrypt(walkeyctx,
+ (uint8*)(page + XLogEncryptionOffset),
+ nbytes - XLogEncryptionOffset,
+ (uint8*)(page + XLogEncryptionOffset),
+ &outlen,
+ wal_encryption_iv);
+
+ Assert(outlen == (nbytes - XLogEncryptionOffset));
+#endif
+}
+
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index ddf18079e2..5ad4d35c44 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -19,6 +19,7 @@
#include "access/xlog.h"
#include "pgstat.h"
#include "storage/checksum.h"
+#include "storage/encryption.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -26,7 +27,6 @@
/* GUC variable */
bool ignore_checksum_failure = false;
-
/* ----------------------------------------------------------------
* Page support functions
* ----------------------------------------------------------------
@@ -85,7 +85,7 @@ PageInit(Page page, Size pageSize, Size specialSize)
* to pgstat.
*/
bool
-PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
+PageIsVerifiedExtended(Page page, ForkNumber forknum, BlockNumber blkno, int flags)
{
PageHeader p = (PageHeader) page;
size_t *pagebytes;
@@ -108,6 +108,8 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
checksum_failure = true;
}
+ PageDecryptInplace(page, forknum, blkno);
+
/*
* The following checks don't prove the header is correct, only that
* it looks sane enough to allow into the buffer pool. Later usage of
@@ -1427,3 +1429,47 @@ PageSetChecksumInplace(Page page, BlockNumber blkno)
((PageHeader) page)->pd_checksum = pg_checksum_page((char *) page, blkno);
}
+
+char*
+PageEncryptCopy(Page page, ForkNumber forknum, BlockNumber blkno)
+{
+ static char *pageCopy = NULL;
+
+ if (PageIsNew(page) || !DataEncryptionEnabled() || !EncryptForkNum(forknum))
+ return (char *) page;
+
+ /*
+ * We allocate the copy space once and use it over on each subsequent
+ * call. The point of palloc'ing here, rather than having a static char
+ * array, is first to ensure adequate alignment for the checksumming code
+ * and second to avoid wasting space in processes that never call this.
+ */
+ if (pageCopy == NULL)
+ pageCopy = MemoryContextAlloc(TopMemoryContext, BLCKSZ);
+
+ memcpy(pageCopy, (char *) page, BLCKSZ);
+ EncryptBufferBlock(blkno, pageCopy);
+ return pageCopy;
+}
+
+void
+PageEncryptInplace(Page page, ForkNumber forknum, BlockNumber blkno)
+{
+ Assert(forknum <= MAX_FORKNUM && blkno != InvalidBlockNumber);
+
+ if (PageIsNew(page) || !DataEncryptionEnabled() || !EncryptForkNum(forknum))
+ return;
+
+ EncryptBufferBlock(blkno, page);
+}
+
+void
+PageDecryptInplace(Page page, ForkNumber forknum, BlockNumber blkno)
+{
+ Assert(forknum <= MAX_FORKNUM && blkno != InvalidBlockNumber);
+
+ if (PageIsNew(page) || !DataEncryptionEnabled() || !EncryptForkNum(forknum))
+ return;
+
+ DecryptBufferBlock(blkno, page);
+}
diff --git a/src/common/cipher_openssl.c b/src/common/cipher_openssl.c
index 1ab5a390e7..8a822c3901 100644
--- a/src/common/cipher_openssl.c
+++ b/src/common/cipher_openssl.c
@@ -34,6 +34,7 @@
typedef const EVP_CIPHER *(*ossl_EVP_cipher_func) (void);
static ossl_EVP_cipher_func get_evp_aes_cbc(int klen);
+static ossl_EVP_cipher_func get_evp_aes_ctr(int klen);
static ossl_EVP_cipher_func
get_evp_aes_cbc(int klen)
@@ -51,6 +52,22 @@ get_evp_aes_cbc(int klen)
}
}
+static ossl_EVP_cipher_func
+get_evp_aes_ctr(int klen)
+{
+ switch (klen)
+ {
+ case PG_AES128_KEY_LEN:
+ return EVP_aes_128_ctr;
+ case PG_AES192_KEY_LEN:
+ return EVP_aes_192_ctr;
+ case PG_AES256_KEY_LEN:
+ return EVP_aes_256_ctr;
+ default:
+ return NULL;
+ }
+}
+
/*
* Initialize and return an EVP_CIPHER_CTX. Return NULL if the given
* cipher algorithm is not supported or on failure..
@@ -68,13 +85,16 @@ ossl_cipher_ctx_create(int cipher, uint8 *key, int klen, bool enc)
{
case PG_CIPHER_AES_CBC:
func = get_evp_aes_cbc(klen);
- if (!func)
- goto failed;
+ break;
+ case PG_CIPHER_AES_CTR:
+ func = get_evp_aes_ctr(klen);
break;
default:
goto failed;
}
+ if (!func)
+ goto failed;
if (enc)
ret = EVP_EncryptInit_ex(ctx, (const EVP_CIPHER *) func(), NULL, key, NULL);
@@ -84,7 +104,7 @@ ossl_cipher_ctx_create(int cipher, uint8 *key, int klen, bool enc)
if (!ret)
goto failed;
- if (!EVP_CIPHER_CTX_set_key_length(ctx, PG_AES256_KEY_LEN))
+ if (!EVP_CIPHER_CTX_set_key_length(ctx, klen))
goto failed;
/*
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 4146753d47..488ae2987f 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -50,6 +50,7 @@ typedef struct XLogPageHeaderData
} XLogPageHeaderData;
#define SizeOfXLogShortPHD MAXALIGN(sizeof(XLogPageHeaderData))
+#define XLogEncryptionOffset SizeOfXLogShortPHD
typedef XLogPageHeaderData *XLogPageHeader;
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 0b6d00dd7d..f268e396d2 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -209,6 +209,7 @@ struct XLogReaderState
*/
char *readBuf;
uint32 readLen;
+ bool encrypted; /* readBuf is encrypted? */
/* last read XLOG position for data currently in readBuf */
WALSegmentContext segcxt;
diff --git a/src/include/common/cipher.h b/src/include/common/cipher.h
index e3fb1d04b5..c8148714fd 100644
--- a/src/include/common/cipher.h
+++ b/src/include/common/cipher.h
@@ -25,7 +25,8 @@
* algorithm.
*/
#define PG_CIPHER_AES_CBC 0
-#define PG_MAX_CIPHER_ID 1
+#define PG_CIPHER_AES_CTR 1
+#define PG_MAX_CIPHER_ID 2
/* AES128/192/256 various length definitions */
#define PG_AES128_KEY_LEN (128 / 8)
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index d0a52f8e08..67be7e50af 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -15,6 +15,7 @@
#define BUFPAGE_H
#include "access/xlogdefs.h"
+#include "common/relpath.h"
#include "storage/block.h"
#include "storage/item.h"
#include "storage/off.h"
@@ -165,6 +166,9 @@ typedef struct PageHeaderData
typedef PageHeaderData *PageHeader;
+#define PageEncryptOffset offsetof(PageHeaderData, pd_linp)
+#define SizeOfPageEncryption (BLCKSZ - PageEncryptOffset)
+
/*
* pd_flags contains the following flag bits. Undefined bits are initialized
* to zero and may be used in the future.
@@ -418,8 +422,8 @@ do { \
((overwrite) ? PAI_OVERWRITE : 0) | \
((is_heap) ? PAI_IS_HEAP : 0))
-#define PageIsVerified(page, blkno) \
- PageIsVerifiedExtended(page, blkno, \
+#define PageIsVerified(page, forknum, blkno) \
+ PageIsVerifiedExtended(page, forknum, blkno, \
PIV_LOG_WARNING | PIV_REPORT_STAT)
/*
@@ -433,7 +437,7 @@ StaticAssertDecl(BLCKSZ == ((BLCKSZ / sizeof(size_t)) * sizeof(size_t)),
"BLCKSZ has to be a multiple of sizeof(size_t)");
extern void PageInit(Page page, Size pageSize, Size specialSize);
-extern bool PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags);
+extern bool PageIsVerifiedExtended(Page page, ForkNumber forknum, BlockNumber blkno, int flags);
extern OffsetNumber PageAddItemExtended(Page page, Item item, Size size,
OffsetNumber offsetNumber, int flags);
extern Page PageGetTempPage(Page page);
@@ -452,5 +456,8 @@ extern bool PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
Item newtup, Size newsize);
extern char *PageSetChecksumCopy(Page page, BlockNumber blkno);
extern void PageSetChecksumInplace(Page page, BlockNumber blkno);
+extern char *PageEncryptCopy(Page page, ForkNumber forknum, BlockNumber blkno);
+extern void PageEncryptInplace(Page page, ForkNumber forknum, BlockNumber blkno);
+extern void PageDecryptInplace(Page page, ForkNumber forknum, BlockNumber blkno);
#endif /* BUFPAGE_H */
diff --git a/src/include/storage/encryption.h b/src/include/storage/encryption.h
new file mode 100644
index 0000000000..a61c47ea83
--- /dev/null
+++ b/src/include/storage/encryption.h
@@ -0,0 +1,42 @@
+/*-------------------------------------------------------------------------
+ *
+ * encryption.h
+ * Cluster encryption functions.
+ *
+ * Portions Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ * src/include/storage/encryption.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef ENCRYPTION_H
+#define ENCRYPTION_H
+
+#include "access/xlogdefs.h"
+#include "common/cipher.h"
+#include "crypto/kmgr.h"
+#include "storage/bufpage.h"
+
+#define DataEncryptionEnabled() true
+
+/* Cluster encryption encrypts only main fork */
+#define EncryptForkNum(forknum) \
+ ((forknum) == MAIN_FORKNUM || (forknum) == INIT_FORKNUM)
+
+/*
+ * The size in byte for counter of AES-CTR mode in nonce.
+ */
+#define ENC_BUFFER_AES_COUNTER_SIZE 4
+#define ENC_WAL_AES_COUNTER_SIZE 4
+
+/* bufenc.c */
+extern void DecryptBufferBlock(BlockNumber blocknum, Page page);
+extern void EncryptBufferBlock(BlockNumber blocknum, Page page);
+
+/* walenc.c */
+extern char *EncryptXLog(char *page, Size nbytes, XLogSegNo segno,
+ uint32 offset);
+extern void DecryptXLog(char *page, Size nbytes, XLogSegNo segno,
+ uint32 offset);
+
+#endif /* ENCRYPTION_H */
On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
On Mon, Dec 14, 2020 at 06:06:15PM -0500, Bruce Momjian wrote:
I am getting close to applying these patches, probably this week. The
patches are cumulative:https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diffI strongly object to a commit based on the current state of the patch.
Based on my lookup of the patches you are referring to, I see a couple
of things that should be splitted up and refactored properly before
thinking about the core part of the patch (FWIW, I don't have much
thoughts to offer about the core part because I haven't really thought
about it, but it does not prevent to do a correct refactoring). Here
are some notes:
- The code lacks a lot of comments IMO. Why is retrieve_passphrase()
doing what it does? It seems to me that pg_altercpass needs a large
brushup.
Uh, pg_altercpass is a new file I wrote and almost every block has a
comment. I added a few more, but can you be more specific?
- There are no changes to src/tools/msvc/. Seeing the diffs in
src/common/, this stuff breaks Windows builds.
OK, done in patch at URL.
- The HMAC split is terrible, as mentioned upthread. I think that you
would need to extract this stuff as a separate patch, and not add more
mess to the existing mess (SCRAM uses its own HMAC with SHA256, which
is already wrong). We can and should have a fallback implementation,
because that's easy to do. And we need to have the fallback
implementation depend on the contents of cryptohash.c (in a
src/common/hmac.c), while the OpenSSL portion requires a
hmac_openssl.c where you can choose the hash type based on
pg_cryptohash_type. So ossl_HMAC_SHA512() does not do the right
thing. APIs flexible enough to allow a new SSL library to plug into
this stuff are required.
- Not much a fan of the changes done in cryptohash.c for the resource
owners as well. It also feels like this could be thought as something
directly for resowner.c.
- The cipher split also should be done in its own patch, and reviewed
on its own. There is a mix of dependencies between non-OpenSSL and
OpenSSL code paths, making the pluggin of a new SSL library harder to
do. In short, this requires proper API design, which is not something
we have here. There are also no changes in pgcrypto for that stuff.
I am going to need someone to help me make these changes. I don't feel
I know enough about the crypto API to do it, and it will take me 1+ week
to learn it.
I do have a few questions:
That looks like a lot of things to sort out as well.
Can anyone test this on Windows, particularly -R handling?
What testing infrastructure should this have?
Using TAP tests would be adapted for those two points.
OK, I will try that.
There are a few shell script I should include to show how to create
commands. Where should they be stored? /contrib module?Why are these needed. Is it a matter of documentation?
I have posted some of the scripts. They involved complex shell
scripting that I doubt the average user can do. The scripts are for
prompting for a passphrase from the user's terminal, or using the
Yubikey, with our without typing a pin. I can put them in the docs and
then users can copy them into a file. Is that the preferred method?
Are people okay with having the feature enabled, but invisible
since the docs and --help output are missing? When we enable
ssl_passphrase_command to prompt from the terminal, some of the
command-line options will be useful.You are suggesting to enable the feature by default, make it invisible
to the users and leave it undocumented? Is there something I missing
here?
The point is that the command-line options will be active, but will not
be documented. It will not do anything on its own unless you specify
that command-line option. I can comment-out the command-line options
from being active but the code it going to look very messy.
Do people like the command-letter choices?
I called the alter passphrase utility pg_altercpass. I could
have called it pg_clusterpass, but I wanted to highlight it is
only for changing the passphrase, not for creating them.I think that you should attach such patches directly to the emails
sent to pgsql-hackers, if those github links disappear for some
reason, it would become impossible to refer to see what has been
discussed here.
Well, the patches are changing frequently. I am attaching a version to
this email.
+/* + * We have to use postgres.h not postgres_fe.h here, because there's so much + * backend-only stuff in the kmgr include files we need. But we need a + * frontend-ish environment otherwise. Hence this ugly hack. + */ +#define FRONTEND 1 + +#include "postgres.h" I would advise to really understand why this happens and split up the backend and frontend parts cleanly. This style ought to be avoided as much as possible.
Uh, I got this code from pg_resetwal.c, which does something similar to
pg_altercpass.
@@ -95,9 +101,9 @@ pg_cryptohash_create(pg_cryptohash_type type)
if (state->evpctx == NULL)
{
+#ifndef FRONTEND
explicit_bzero(state, sizeof(pg_cryptohash_state));
explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-#ifndef FRONTEND
I think that this change is incorrect. Any clean up of memory should
be done for the frontend *and* the backend.
Oh, good point. Fixed.
+#ifdef USE_OPENSSL + ctx = (PgCipherCtx *) palloc0(sizeof(PgCipherCtx)); + + ctx->encctx = ossl_cipher_ctx_create(cipher, key, klen, true); + ctx->decctx = ossl_cipher_ctx_create(cipher, key, klen, false); +#endif There is a cipher_openssl.c and a cipher.c that includes USE_OPENSSL. This requires a cleaner split IMO. We should avoid as much as possible OpenSSL-specific code paths in files part of src/common/ when not building with OpenSSL. So this is now a mixed bag of dependencies.
Again, I need help here.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
On Tue, Dec 15, 2020 at 10:36:56AM +0800, Neil Chen wrote:
Hi, Bruce��
I read your question and here are some of my thoughts.
� � � � Why is KmgrShmemData a struct, when it only has a single member?�
Are
� � � � all shared memory areas structs?�
Yes, all areas created by ShmemInitStruct() are structs. I think the
significance of using struct is that it delimits an "area" �to store the KMS
related things, which makes our memory management more clear and unified.
OK, thanks, that helps.
� � � � What testing infrastructure should this have?
� � � � There are a few shell script I should include to show how to create
� � � � commands.� Where should they be stored?� /contrib module?�
� � � � Are people okay with having the feature enabled, but invisible
� � � � since the docs and --help output are missing? When we enable
� � � � ssl_passphrase_command to prompt from the terminal, some of the
� � � � command-line options will be useful.�
Since our implementation is not in contrib, I don't think we should put the
script there. Maybe we can refer to postgresql.conf.sample?��
Uh, the script are 20-60 lines long --- I am attaching them to this
email. Plus, when we allow user prompting for the SSL passphrase, we
will have another script, or maybe three mor if people want to use a
Yubikey to unlock the SSL passphrase.
Actually, I wonder whether we can add the UDK(user data encryption key) to the
first version of KMS, which can be provided to plug-ins such as pgsodium. In
this way, users can at least use it to a certain extent.
I don't thinK I want to get into that at this point. It can be done
later.
Also, I have tested some KMS functionalities by adding very simple TDE logic.
In the meantime, I found some confusion in the following places:
OK, I know Cybertec has a TDE patch too.
1. Previously, we added a variable bootstrap_keys_wrap that is used for
encryption during initdb. However, since we save the "wrapped" key, we need to
use a global KEK that can be accessed in boot mode to unwrap it before use... I
don't know if that's good. To make it simple, I modified the
bootstrap_keys_wrap to store the "unwrapped" key so that the encryption
function can get it correctly. (The variable name should be changed
accordingly).
I see what you are saying. We store the wrapped in bootstrap mode, but
the unwrapped in normal mode. There is also the case of when we copy
the keys from an old cluster. I will work on a patch tomorrow and
report back here.
2. I tried to add support for AES_CTR mode, and the code for encrypting buffer
blocks. In the process I found that in pg_cipher_ctx_create, the key length is
declared as "byte". However, in the CryptoKey structure, the length is stored
as "bit", which leads me to use a form similar to Key->klen / 8 when I call
this function. Maybe we should unify the two to avoid unnecessary confusion.
Agreed. I will look at that too tomorrow.
3. This one is not a problem/bug. I have some doubts about the length of data
encryption blocks. For the page, we do not encrypt the PageHeaderData, which is
192 bit long. By default, the page size is 8K(65536 bits), so the length of the
data we want to encrypt is 65344 bits. This number can't be divisible by 128
and 192 keys and 256 bits long keys. Will this cause a problem?
Since we are using CTR mode for everything, it should be fine. We
encrypt WAL as it is written.
Here is a simple patch that I wrote with references to Sawada's previous TDE
works, hope it can help you.
Thanks. I will work on the items you mentioned. Can you look at the
Cybertec patch and then our wiki to see what needs to be done next?
https://wiki.postgresql.org/wiki/Transparent_Data_Encryption
Thanks for getting a proof-of-concept patch out there. :-)
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Mon, Dec 14, 2020 at 10:19:02PM -0500, Bruce Momjian wrote:
On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
- The HMAC split is terrible, as mentioned upthread. I think that you
would need to extract this stuff as a separate patch, and not add more
mess to the existing mess (SCRAM uses its own HMAC with SHA256, which
is already wrong). We can and should have a fallback implementation,
because that's easy to do. And we need to have the fallback
implementation depend on the contents of cryptohash.c (in a
src/common/hmac.c), while the OpenSSL portion requires a
hmac_openssl.c where you can choose the hash type based on
pg_cryptohash_type. So ossl_HMAC_SHA512() does not do the right
thing. APIs flexible enough to allow a new SSL library to plug into
this stuff are required.
- Not much a fan of the changes done in cryptohash.c for the resource
owners as well. It also feels like this could be thought as something
directly for resowner.c.
- The cipher split also should be done in its own patch, and reviewed
on its own. There is a mix of dependencies between non-OpenSSL and
OpenSSL code paths, making the pluggin of a new SSL library harder to
do. In short, this requires proper API design, which is not something
we have here. There are also no changes in pgcrypto for that stuff.I am going to need someone to help me make these changes. I don't feel
I know enough about the crypto API to do it, and it will take me 1+ week
to learn it.
I think that designing a correct set of APIs that can be plugged with
any SSL library is the correct move in the long term. I have on my
agenda to clean up HMAC as SCRAM uses that with SHA256 and you would
use that with SHA512. Daniel has mentioned that he has been touching
this area, and I also got a patch halfly done though pgcrypto needs
some extra thoughts. So this is still WIP but you could reuse that
here.
Uh, I got this code from pg_resetwal.c, which does something similar to
pg_altercpass.
Yeah, that's actually the part where it is a bad idea to just copy
this pattern. pg_resetwal should not do that in the long term in my
opinion, but nobody has come to clean up this stuff. (- -;)
+#ifdef USE_OPENSSL + ctx = (PgCipherCtx *) palloc0(sizeof(PgCipherCtx)); + + ctx->encctx = ossl_cipher_ctx_create(cipher, key, klen, true); + ctx->decctx = ossl_cipher_ctx_create(cipher, key, klen, false); +#endif There is a cipher_openssl.c and a cipher.c that includes USE_OPENSSL. This requires a cleaner split IMO. We should avoid as much as possible OpenSSL-specific code paths in files part of src/common/ when not building with OpenSSL. So this is now a mixed bag of dependencies.Again, I need help here.
My take would be to try to sort out the HMAC part first, then look at
the ciphers. One step at a time.
--
Michael
On Tue, Dec 15, 2020 at 02:20:33PM +0900, Michael Paquier wrote:
Uh, I got this code from pg_resetwal.c, which does something similar to
pg_altercpass.Yeah, that's actually the part where it is a bad idea to just copy
this pattern. pg_resetwal should not do that in the long term in my
opinion, but nobody has come to clean up this stuff. (- -;)
Glad you asked about this. It turns out pg_altercpass works fine with
postgres_fe.h, so I will now use that. pg_resetwal still can't use it
though.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Mon, Dec 14, 2020 at 11:16:18PM -0500, Bruce Momjian wrote:
1. Previously, we added a variable bootstrap_keys_wrap that is used for
encryption during initdb. However, since we save the "wrapped" key, we need to
use a global KEK that can be accessed in boot mode to unwrap it before use... I
don't know if that's good. To make it simple, I modified the
bootstrap_keys_wrap to store the "unwrapped" key so that the encryption
function can get it correctly. (The variable name should be changed
accordingly).I see what you are saying. We store the wrapped in bootstrap mode, but
the unwrapped in normal mode. There is also the case of when we copy
the keys from an old cluster. I will work on a patch tomorrow and
report back here.
I had not considered that we need the date keys available in bootstrap
mode, even if we copied them from another cluster during pg_upgrade. I
have updated the diff URLs and attaching a patch showing the changes I
made. Basically, I had to separate BootStrapKmgr() into sections:
1. copy or create an empty live key directory
2. get the pass phrase
3. populate the live key directory if we didn't copy it
4. decrypt they keys into a file-scoped variable
Thanks for showing me this missing feature.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
bootstrap_key.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/crypto/kmgr.c b/src/backend/crypto/kmgr.c
new file mode 100644
index 9143e72..24c5d7f
*** a/src/backend/crypto/kmgr.c
--- b/src/backend/crypto/kmgr.c
*************** static KmgrShmemData *KmgrShmem;
*** 50,56 ****
char *cluster_passphrase_command = NULL;
int file_encryption_keylen = 0;
! CryptoKey bootstrap_keys_wrap[KMGR_MAX_INTERNAL_KEYS];
extern char *bootstrap_old_key_datadir;
extern int bootstrap_file_encryption_keylen;
--- 50,56 ----
char *cluster_passphrase_command = NULL;
int file_encryption_keylen = 0;
! CryptoKey bootstrap_keys[KMGR_MAX_INTERNAL_KEYS];
extern char *bootstrap_old_key_datadir;
extern int bootstrap_file_encryption_keylen;
*************** static CryptoKey *generate_crypto_key(in
*** 65,74 ****
void
BootStrapKmgr(void)
{
! PgKeyWrapCtx *ctx;
char passphrase[KMGR_MAX_PASSPHRASE_LEN];
- uint8 KEK_enc[KMGR_ENC_KEY_LEN];
- uint8 KEK_hmac[KMGR_MAC_KEY_LEN];
int passlen;
#ifndef USE_OPENSSL
--- 65,74 ----
void
BootStrapKmgr(void)
{
! char live_path[MAXPGPATH];
! CryptoKey *keys_wrap;
! int nkeys;
char passphrase[KMGR_MAX_PASSPHRASE_LEN];
int passlen;
#ifndef USE_OPENSSL
*************** BootStrapKmgr(void)
*** 78,83 ****
--- 78,85 ----
errhint("Compile with --with-openssl to use cluster encryption."))));
#endif
+ snprintf(live_path, sizeof(live_path), "%s/%s", DataDir, LIVE_KMGR_DIR);
+
/* copy cluster file encryption keys from an old cluster? */
if (bootstrap_old_key_datadir != NULL)
{
*************** BootStrapKmgr(void)
*** 87,122 ****
bootstrap_old_key_datadir, LIVE_KMGR_DIR);
copydir(old_key_dir, LIVE_KMGR_DIR, true);
}
! /* generate new cluster file encryption keys */
else
{
! char live_path[MAXPGPATH];
!
! if (mkdir(LIVE_KMGR_DIR, pg_dir_create_mode) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create cluster file encryption directory \"%s\": %m",
LIVE_KMGR_DIR)));
- memset(bootstrap_keys_wrap, 0, sizeof(bootstrap_keys_wrap));
- /* bzero keys on exit */
- on_proc_exit(bzeroKmgrKeys, 0);
-
- /* Get key encryption key from the passphrase command */
- snprintf(live_path, sizeof(live_path), "%s/%s", DataDir, LIVE_KMGR_DIR);
- passlen = kmgr_run_cluster_passphrase_command(cluster_passphrase_command,
- passphrase, KMGR_MAX_PASSPHRASE_LEN,
- live_path);
- if (passlen < KMGR_MIN_PASSPHRASE_LEN)
- ereport(ERROR,
- (errmsg("passphrase must be at least %d bytes",
- KMGR_MIN_PASSPHRASE_LEN)));
-
/* Get key encryption key and HMAC key from passphrase */
kmgr_derive_keys(passphrase, passlen, KEK_enc, KEK_hmac);
- explicit_bzero(passphrase, passlen);
-
/* Create temporary key wrap context */
ctx = pg_create_keywrap_ctx(KEK_enc, KEK_hmac);
if (!ctx)
--- 89,128 ----
bootstrap_old_key_datadir, LIVE_KMGR_DIR);
copydir(old_key_dir, LIVE_KMGR_DIR, true);
}
! /* create empty directory */
else
{
! if (mkdir(LIVE_KMGR_DIR, pg_dir_create_mode) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create cluster file encryption directory \"%s\": %m",
LIVE_KMGR_DIR)));
+ }
+
+ /*
+ * Get key encryption key from the passphrase command. The passphrase
+ * command might want to check for the existance of files in the
+ * live directory, so run this _after_ copying the directory in place.
+ */
+ passlen = kmgr_run_cluster_passphrase_command(cluster_passphrase_command,
+ passphrase, KMGR_MAX_PASSPHRASE_LEN,
+ live_path);
+ if (passlen < KMGR_MIN_PASSPHRASE_LEN)
+ ereport(ERROR,
+ (errmsg("passphrase must be at least %d bytes",
+ KMGR_MIN_PASSPHRASE_LEN)));
+
+ /* generate new cluster file encryption keys */
+ if (bootstrap_old_key_datadir == NULL)
+ {
+ CryptoKey bootstrap_keys_wrap[KMGR_MAX_INTERNAL_KEYS];
+ PgKeyWrapCtx *ctx;
+ uint8 KEK_enc[KMGR_ENC_KEY_LEN];
+ uint8 KEK_hmac[KMGR_MAC_KEY_LEN];
/* Get key encryption key and HMAC key from passphrase */
kmgr_derive_keys(passphrase, passlen, KEK_enc, KEK_hmac);
/* Create temporary key wrap context */
ctx = pg_create_keywrap_ctx(KEK_enc, KEK_hmac);
if (!ctx)
*************** BootStrapKmgr(void)
*** 142,149 ****
--- 148,177 ----
/* Save data encryption keys to the disk */
KmgrSaveCryptoKeys(LIVE_KMGR_DIR, bootstrap_keys_wrap);
+ explicit_bzero(bootstrap_keys_wrap, sizeof(bootstrap_keys_wrap));
pg_free_keywrap_ctx(ctx);
}
+
+ /*
+ * We are either decrypting keys we copied from an old cluster, or
+ * decrypting keys we just wrote above --- either way, we decrypt
+ * them here and store them in a file-scoped variable for use in
+ * later encrypting during bootstrap mode.
+ */
+ /* Get the crypto keys from the file */
+ keys_wrap = kmgr_get_cryptokeys(LIVE_KMGR_DIR, &nkeys);
+ Assert(nkeys == KMGR_MAX_INTERNAL_KEYS);
+
+ if (!kmgr_verify_passphrase(passphrase, passlen, keys_wrap, bootstrap_keys,
+ KMGR_MAX_INTERNAL_KEYS))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cluster passphrase does not match expected passphrase")));
+
+ /* bzero keys on exit */
+ on_proc_exit(bzeroKmgrKeys, 0);
+
+ explicit_bzero(passphrase, passlen);
}
/* Report shared-memory space needed by KmgrShmem */
*************** void
*** 179,187 ****
InitializeKmgr(void)
{
CryptoKey *keys_wrap;
char passphrase[KMGR_MAX_PASSPHRASE_LEN];
int passlen;
- int nkeys;
struct stat buffer;
char live_path[MAXPGPATH];
--- 207,215 ----
InitializeKmgr(void)
{
CryptoKey *keys_wrap;
+ int nkeys;
char passphrase[KMGR_MAX_PASSPHRASE_LEN];
int passlen;
struct stat buffer;
char live_path[MAXPGPATH];
*************** InitializeKmgr(void)
*** 219,234 ****
(errcode(ERRCODE_INTERNAL_ERROR),
(errmsg("cluster has no data encryption keys"))));
- /* Get the crypto keys from the file */
- keys_wrap = kmgr_get_cryptokeys(LIVE_KMGR_DIR, &nkeys);
- Assert(nkeys == KMGR_MAX_INTERNAL_KEYS);
-
/* Get cluster passphrase */
snprintf(live_path, sizeof(live_path), "%s/%s", DataDir, LIVE_KMGR_DIR);
passlen = kmgr_run_cluster_passphrase_command(cluster_passphrase_command,
passphrase, KMGR_MAX_PASSPHRASE_LEN,
live_path);
/*
* Verify passphrase and prepare a data encryption key in plaintext in shared memory.
*/
--- 247,262 ----
(errcode(ERRCODE_INTERNAL_ERROR),
(errmsg("cluster has no data encryption keys"))));
/* Get cluster passphrase */
snprintf(live_path, sizeof(live_path), "%s/%s", DataDir, LIVE_KMGR_DIR);
passlen = kmgr_run_cluster_passphrase_command(cluster_passphrase_command,
passphrase, KMGR_MAX_PASSPHRASE_LEN,
live_path);
+ /* Get the crypto keys from the file */
+ keys_wrap = kmgr_get_cryptokeys(LIVE_KMGR_DIR, &nkeys);
+ Assert(nkeys == KMGR_MAX_INTERNAL_KEYS);
+
/*
* Verify passphrase and prepare a data encryption key in plaintext in shared memory.
*/
*************** static void
*** 245,251 ****
bzeroKmgrKeys(int status, Datum arg)
{
if (IsBootstrapProcessingMode())
! explicit_bzero(bootstrap_keys_wrap, sizeof(bootstrap_keys_wrap));
else
explicit_bzero(KmgrShmem->intlKeys, sizeof(KmgrShmem->intlKeys));
}
--- 273,279 ----
bzeroKmgrKeys(int status, Datum arg)
{
if (IsBootstrapProcessingMode())
! explicit_bzero(bootstrap_keys, sizeof(bootstrap_keys));
else
explicit_bzero(KmgrShmem->intlKeys, sizeof(KmgrShmem->intlKeys));
}
*************** KmgrGetKey(int id)
*** 256,262 ****
Assert(id < KMGR_MAX_INTERNAL_KEYS);
return (const CryptoKey *) (IsBootstrapProcessingMode() ?
! &(bootstrap_keys_wrap[id]) : &(KmgrShmem->intlKeys[id]));
}
/* Generate an empty CryptoKey */
--- 284,290 ----
Assert(id < KMGR_MAX_INTERNAL_KEYS);
return (const CryptoKey *) (IsBootstrapProcessingMode() ?
! &(bootstrap_keys[id]) : &(KmgrShmem->intlKeys[id]));
}
/* Generate an empty CryptoKey */
On Tue, Dec 15, 2020 at 10:36:56AM +0800, Neil Chen wrote:
2. I tried to add support for AES_CTR mode, and the code for encrypting buffer
blocks. In the process I found that in pg_cipher_ctx_create, the key length is
declared as "byte". However, in the CryptoKey structure, the length is stored
as "bit", which leads me to use a form similar to Key->klen / 8 when I call
this function. Maybe we should unify the two to avoid unnecessary confusion.
Yes, I would also like to get opinions on this. We certainly have to
have the key length be in _bit_ units when visible by users, but I see a
lot of cases where we allocate arrays based on bytes. I am unclear
where the proper units should be. At a minimum, we should specify the
units in the function parameter names.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Greetings,
* Bruce Momjian (bruce@momjian.us) wrote:
On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
On Mon, Dec 14, 2020 at 06:06:15PM -0500, Bruce Momjian wrote:
I am getting close to applying these patches, probably this week. The
patches are cumulative:https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diffI strongly object to a commit based on the current state of the patch.
Yeah, I agree that this could use some work though I don't think it's
all that far away from being something we can get in, to allow us to
move forward with the other work around supporting TDE.
There are a few shell script I should include to show how to create
commands. Where should they be stored? /contrib module?Why are these needed. Is it a matter of documentation?
I have posted some of the scripts. They involved complex shell
scripting that I doubt the average user can do. The scripts are for
prompting for a passphrase from the user's terminal, or using the
Yubikey, with our without typing a pin. I can put them in the docs and
then users can copy them into a file. Is that the preferred method?
If we are going to include these in core anywhere, I would think a new
directory under contrib would make the most sense. I'd hope that we
could find a way to automate the testing of them though, so that we have
some idea when they break because otherwise I'd be concerned that
they'll break somewhere down the line and we won't notice for quite a
while.
This doesn't seem like something that would make sense to only have in
the documentation, which isn't a terribly convenient way to make use of
them.
Are people okay with having the feature enabled, but invisible
since the docs and --help output are missing? When we enable
ssl_passphrase_command to prompt from the terminal, some of the
command-line options will be useful.You are suggesting to enable the feature by default, make it invisible
to the users and leave it undocumented? Is there something I missing
here?The point is that the command-line options will be active, but will not
be documented. It will not do anything on its own unless you specify
that command-line option. I can comment-out the command-line options
from being active but the code it going to look very messy.
I'm a bit concerned with what's being contemplated here.. Ideally,
we'll actually get everything committed for v14 but if we don't and this
doesn't serve any use-case then I'm not sure that it should be
included in the release. I also don't like committing and reverting
things either, but having command line options that aren't documented
but which exist in the code isn't great either, nor is having random
code commented out..
Do people like the command-letter choices?
I called the alter passphrase utility pg_altercpass. I could
have called it pg_clusterpass, but I wanted to highlight it is
only for changing the passphrase, not for creating them.I think that you should attach such patches directly to the emails
sent to pgsql-hackers, if those github links disappear for some
reason, it would become impossible to refer to see what has been
discussed here.Well, the patches are changing frequently. I am attaching a version to
this email.
I agree with having the patches posted to the list. I don't really
agree with the argument of "they change frequently".
* Michael Paquier (michael@paquier.xyz) wrote:
On Mon, Dec 14, 2020 at 10:19:02PM -0500, Bruce Momjian wrote:
On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
- The cipher split also should be done in its own patch, and reviewed
on its own. There is a mix of dependencies between non-OpenSSL and
OpenSSL code paths, making the pluggin of a new SSL library harder to
do. In short, this requires proper API design, which is not something
we have here. There are also no changes in pgcrypto for that stuff.I am going to need someone to help me make these changes. I don't feel
I know enough about the crypto API to do it, and it will take me 1+ week
to learn it.I think that designing a correct set of APIs that can be plugged with
any SSL library is the correct move in the long term. I have on my
agenda to clean up HMAC as SCRAM uses that with SHA256 and you would
use that with SHA512. Daniel has mentioned that he has been touching
this area, and I also got a patch halfly done though pgcrypto needs
some extra thoughts. So this is still WIP but you could reuse that
here.
Yeah, looking at what's been done there seems like the right approach to
me as well, ideally we'd have one set of APIs that'll support all these
use cases (not unlike what curl does..).
The other thought I had here off-hand was that we might want to randomly
generate a key during startup and have that available and use it for
anything that's temporary and which is going to get blown away on a
restart, I've been thinking that doing that might allow us to have a
relatively simpler API for transient/temporary files too.
Thanks,
Stephen
Import Notes
Reply to msg id not found: X9hHoQcZ11pBtgnz@paquier.xyz20201215031902.GB14596@momjian.us | Resolved by subject fallback
On Tue, Dec 15, 2020 at 02:20:33PM +0900, Michael Paquier wrote:
On Mon, Dec 14, 2020 at 10:19:02PM -0500, Bruce Momjian wrote:
I am going to need someone to help me make these changes. I don't feel
I know enough about the crypto API to do it, and it will take me 1+ week
to learn it.I think that designing a correct set of APIs that can be plugged with
any SSL library is the correct move in the long term. I have on my
agenda to clean up HMAC as SCRAM uses that with SHA256 and you would
use that with SHA512. Daniel has mentioned that he has been touching
this area, and I also got a patch halfly done though pgcrypto needs
some extra thoughts. So this is still WIP but you could reuse that
here.
I thought this was going to be a huge job, but once I looked at it, it
was clear exactly what you were saying. Comparing cryptohash.c and
cryptohash_openssl.c I saw exactly what you wanted, and I think I have
completed it in the attached patch. cryptohash.c implemented the hash
in C code if OpenSSL is not present --- I assume you didn't want me to
do that, but rather to split the API so it was easy to add another
implementation.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
key-alter.diff.gzapplication/gzipDownload
�| �_ key-alter.diff ��}�6�(���)���J��%'N�c+�O�Gr���}t)����D�(�����?�� Rr�vw����D$�!0f�2��cQ,^�K�����n�������a0�W%�-����[1�'��#OT��f������/�>h�k�j��
���x������<*�y_z�#��/~� \^-��? �����{�tw��Lv���zw�N��^��t�Z���w�S�Vs�_,>���������������#y3���
^=zN7�Z�����j���^�{Z�f���|���W�V�\�����h�
��B�Adut�������<�.��f��}�j5,hq����z������������7��or�-���f}�L ��-���;3w���~�pDmD���n��
�vC���"������������{~���Z��41wp^z���E_��|��F\�K/�3�j\��k?M�����^(�� �3w"d?�2�`��3���D4�8X9R^0���
�'BoqO�������7+�7��6��],KF�w��������`:_R��}p���L��=B��0Z�?/1��Ew��fh/w�M�[������)�p<��_^`��3�%!N����w���&�r'��t_���b��a��J
X�>�u����?_�0f�� �so��$���w���F @���$���+�!cB�a�7HL���po\������H`W����
hvDH,�g����[�u�0!� ���_�o|�"�9@����5�R�����gIY�, G1�$AbG��l �V?�.D:>����/p����si�;��p��K&���PCr�W�*9H!|���X�`�{.���G�`���]�n�bv�yy�;�j��4Q�LT<1P���
yi����vXg@"��c��gn
`�5���5� ��aG��9����������T����� k��t���`���D#��{�y���� �OD�Z�WK��pK���m�8S����&��������dz���� ��%�#�� �\b�|>���t_!
^������|��6(%��uR�����~B$� W
f����A�S�j����m�Z,���E��B�*S�N|���J�DJ����(���E�������,C-��|���Fw�����l�}����xz���2&�/q$������n�
)��lU��^�fJir�9"9*��N�!�a���$)l����e�L)��>-A
�;��u�!O o��\-�W������
$�����T��v�����u�7o s�!m��lz�I���&,���T�aQy�� ���R�����a7����m� ���JL#w��VD�(FI�������A�q2�4N��?[zW(Ke�vn���&���e����J�����jyC��� ���e�M��N9b��h3�x.^>�;�
�X�<�����#��\�� ��k�^�]�������vz�����;���VD����W��.�,M�K�`��A^2�B
K =�.��^*aZ�$*F$�j!J�MJ\$<�NH�]1h���!Q-��2��7���/�t�[���7��!,9������pv���Wh0����sw�y5WH#Q���v�y���s�f$1=Y�|�_^��[o�I�Sg�-O��_�Kn@�`�;qNb��a��D ���E G���
f���>�%jf%�hn�=wN�������?�$
+�hh4���:u�
��v b�6��#��O��0���O�D �����%�dWoQ�.��8Qh#*|<<�1fD\r�Sz�x�
v���K�r��HAQ��@`).D������!,=�*wq�;r+�C���o�n�J}������+��M��"X��+�B��J9��e�3��>�!
a��x��J�������%�� #�����H���Z�>�}���j�[���P?�.
�
��` �e�����%*�t��`v7}���duy���K�/��n���`����W�<y������J��?b����,�r��
��N��@�t���IW"K��9�gqV����2�2�O���CE��������H�p��=�W��Yp+����A}��*��I4�c��F<�XC�3s?���H�0��m�7���,i'5��,� XV��2�S:Z
�$:]�����S�K����`�O�2J��5�b��hU
������2�����H%��(Uj�3IOn
�I��&_iQ��v��F<�L��k@2�gW���|��ad��Qa��i�d���Y�4u2�#�P��7��q�s�K��<) ����\-�$d�T��A$Y���� �@��+���s�2�($���;%�dv�����c1 D��la��`�h�H��/��?��LB|Q�Hh�����0o3��f������F>I!����2sP��k
56fz ��$)��x'D��~��&����hB����������,Co�25��113�9�I���E�9�bF���;��� $�Px����; ���h��&�,C'��z`������6l����
�d1�
���������Gs��
'�=���=�;����w��,�S�'`>�/�G�+�p#�+���H��
�Y|��O�lcr���N�!�p�����Z$>��5������Bb(���g�zt��K�G��ceOn�p%I 0J�]�=9[�_�TH� �����vfY�U��y�59�F� ���W��a0�Id?��c�{�9<6DXvO��S���D�X����������~-n�;yG�g�#4aq0��8��.�Hru�Z�
#@<<or�i�����lqa0���F��b��R���&�"P�����H�<,X� t����n��
��B|(��^��4QK�����(^O��g9g��)z�������J���X�� [�SYvCk� u�� �P��?�H��#�Q����+/�&����h^6U�u������>�:MdN�Z)Y��E�iry�����c���dB�5p%$4�; ���$��H'����Rm9����dR4��<��T+��W�a�}%m�8��d�H��8f;�(���J�F��Hk� @@�����U�N�@�\^O���$��F�q6���`:��:�<��Ip��D�F���uNl�b9d��Xs1X@��j�L��p_A Yt1�@��-��qB�O�[o�I>c�%%��d{=�����q8�v���)�'���}Y����wG�?�I�Ad�CX+T��j19��^.����.��|o9.�����t����Jc�U����_/w��o�\�4���k)��9M2,�dn�[�w
�6�o���~����$@����A���~u{
H|�_v���;Y��!����
zzWrA�%4P����U���Z�z^{�wHUky-�-�J�vs\w�GUq`���x��T��4���^6�9���������@����O���e�������u�Ip���"z�(9�x����/�U�%�T��G�,�d����my*Jgn��0�OCq�����~�n��g ��/���M�$��$crvR
����+�A�a8l��u=�Zh"�(��f�Q1R)���a���[5�M���(�e����F��Q�
��m��S���B�y�82�~a�-�R�j����P�� l�?�H�� �B
)��N�<:=S%���
s���'{4�s���6���K�.�����G���4���C�4�]`5���� MZe�"���Zb���z�a��3�����^�<�P6�S�<������l����=�����Zy��J��������5��Y�R�A�%-<�3O�g00e����MO�hU�*Ne�i�[�
���;��^�/��$�����d��,��l������kCR�2oSXx�]�,Yq�x�/ 4�a�Q*�]�lXO���=�$�����>*+T���@�n��J�e`o�[q�������oiW����,��s���f������|*�hp��]6�_i����������?~����O��?~�������������7�P����X��h]��d�/��P�Y��m��{�
�j��V
����{��Kn�p����p�)���e�� D���mBOH�j���{�wV��=������C$pMU`J�K��I �#�W|oS���t?2l5�j���U[N���7�������tb�f��6��o��3��#Z%��iY��I��|3I��1s�f�"�<N4�r��UL�i������e�b;E` ��T
I�?�<�|�M(�(�#�RN��wuwF����mu1�>��@��U5�Z��b��#|7��/�����%)4�.�9���a�(�ah$Z37�f ��;��C\����t�5�6�f�6*g�',�2�����K������f�
�� �������N�1i#JF�$j2�E�4����g�@��t��#(�L��#$�=�[�M0��U��nt�?4��!���:�@$���l�P�iu��D����Cu[7������=<��� ���^�R�m$��~uv8����P9~&��n�VW�`5�������o��+�����������$��r�:���yK0���M��@`�����G�8:��u
bYg���V�jo���2=@Gq��_K���I`�|g�5}������~��M|�(\Q3�(Z�RL�s�J���XZ��q!qM�f�#������3�SP#�� �I��
3���D���]�b��1nE����������$��O��b��9~��~��� �zwhIa��^�8��Y�r���v��2�-������jm@��
��(���������'�Z�-��;�'��%�z������n��m������?�!y�/�g�t�Sr����P�T�h�����������i��9���i�]cp_ �P��'