Re: WIP: Data at rest encryption

[Michael Paquier]

On Wed, Sep 04, 2019 at 06:56:18AM +0200, Antonin Houska wrote:
> This thread started later than our effort but important design questions are
> being discussed there. So far there seems to be no consensus whether
> full-instance encryption should be implemented first, so any effort spent on
> this patch might get wasted. When/if there will be an agreement on the design,
> we'll see how much of this patch can be used.

I see.  I have marked the patch as returned with feedback in CF
2019-11.  Let's see how the other one finishes, before perhaps coming
back to this one.
--
Michael


signature.asc
Description: PGP signature

Re: WIP: Data at rest encryption

[Shawn Wang]

 On Wed, 04 Sep 2019 00:56:15 +0800 Alvaro Herrera 
 wrote 



On 2019-Aug-02, Shawn Wang wrote: 



> Hi Antonin, 

> It is very glad to see the new patch. I used the public patches a long time 
> ago. 

> I did some tests like the stream replication, much data running, temporary 
> files encryption. 

> I found that there is an issue in the src/backend/storage/file/encryption.c. 
> You should put block_size = EVP_CIPHER_CTX_block_size(ctx); under the #ifdef 
> USE_ASSERT_CHECKING. 

> There is some problem to merge your patches to the latest kernel in the 
> pg_ctl.c. 



Is a new, fixed version going to be posted soon?  It's been a while. 






Also, apologies if this has been asked before, but: how does this patch 

relate to the stuff being discussed in 

https://postgr.es/m/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp ? 








Hi Álvaro,



Thank you for a reply.


I mainly said that the issues in the src/backend/storage/file/encryption.c. If 
somebody want to use these patches, I think Antonin need to fix it.

It does not relate to the stuff being discussed in TDE. As I know, some company 
use these patches to encrypt data, even if these issues don't matter.



Regards, 

--

Shawn Wang

Re: WIP: Data at rest encryption

[Antonin Houska]

Alvaro Herrera  wrote:

> On 2019-Aug-02, Shawn Wang wrote:
> 
> > Hi Antonin,
> > It is very glad to see the new patch. I used the public patches a long time 
> > ago.
> > I did some tests like the stream replication, much data running, temporary 
> > files encryption.
> > I found that there is an issue in the 
> > src/backend/storage/file/encryption.c. You should put block_size = 
> > EVP_CIPHER_CTX_block_size(ctx); under the #ifdef USE_ASSERT_CHECKING.
> > There is some problem to merge your patches to the latest kernel in the 
> > pg_ctl.c.
> 
> Is a new, fixed version going to be posted soon?  It's been a while.
> 
> Also, apologies if this has been asked before, but: how does this patch
> relate to the stuff being discussed in
> https://postgr.es/m/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp ?

This thread started later than our effort but important design questions are
being discussed there. So far there seems to be no consensus whether
full-instance encryption should be implemented first, so any effort spent on
this patch might get wasted. When/if there will be an agreement on the design,
we'll see how much of this patch can be used.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: WIP: Data at rest encryption

[Ibrar Ahmed]

On Tue, Sep 3, 2019 at 10:21 PM Alvaro Herrera 
wrote:

> On 2019-Aug-02, Shawn Wang wrote:
>
> > Hi Antonin,
> > It is very glad to see the new patch. I used the public patches a long
> time ago.
> > I did some tests like the stream replication, much data running,
> temporary files encryption.
> > I found that there is an issue in the
> src/backend/storage/file/encryption.c. You should put block_size =
> EVP_CIPHER_CTX_block_size(ctx); under the #ifdef USE_ASSERT_CHECKING.
> > There is some problem to merge your patches to the latest kernel in the
> pg_ctl.c.
>
> Is a new, fixed version going to be posted soon?  It's been a while.
>
> Also, apologies if this has been asked before, but: how does this patch
> relate to the stuff being discussed in
> https://postgr.es/m/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp ?
>
> Yes, it is related to that, but we don't have that on our agenda in a
weekly meeting.  It has
many conflicting points about what we are doing. Swada / Bruce can comment
on that.


> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>

-- 
Ibrar Ahmed

Re: WIP: Data at rest encryption

[Alvaro Herrera]

On 2019-Aug-02, Shawn Wang wrote:

> Hi Antonin,
> It is very glad to see the new patch. I used the public patches a long time 
> ago.
> I did some tests like the stream replication, much data running, temporary 
> files encryption.
> I found that there is an issue in the src/backend/storage/file/encryption.c. 
> You should put block_size = EVP_CIPHER_CTX_block_size(ctx); under the #ifdef 
> USE_ASSERT_CHECKING.
> There is some problem to merge your patches to the latest kernel in the 
> pg_ctl.c.

Is a new, fixed version going to be posted soon?  It's been a while.

Also, apologies if this has been asked before, but: how does this patch
relate to the stuff being discussed in
https://postgr.es/m/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp ?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: WIP: Data at rest encryption

[Shawn Wang]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, failed
Spec compliant:   tested, failed
Documentation:not tested

Hi Antonin,
It is very glad to see the new patch. I used the public patches a long time ago.
I did some tests like the stream replication, much data running, temporary 
files encryption.
I found that there is an issue in the src/backend/storage/file/encryption.c. 
You should put block_size = EVP_CIPHER_CTX_block_size(ctx); under the #ifdef 
USE_ASSERT_CHECKING.
There is some problem to merge your patches to the latest kernel in the 
pg_ctl.c.

Regards,

--
Shawn Wang

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Thomas Munro]

On Sat, Jul 6, 2019 at 2:42 AM Antonin Houska  wrote:
> I've reworked the way key is passed to postmaster (v04-0003-...) so it's
> easier to call the encryption key command interactively, adjusted pg_upgrade
> so that it preserves database, tablespace and relfilenode OIDs (v04-0014-...),
> reorganized the code a bit and split the code into more diffs.

Hi Antonin,

Some robotic feedback:

1.  On Linux, an assertion failed:

ExceptionalCondition (conditionName=conditionName@entry=0x973891
"!(decrypt_p == p)", errorType=errorType@entry=0x928d7d
"FailedAssertion", fileName=fileName@entry=0x973827 "xlogutils.c",
lineNumber=lineNumber@entry=815) at assert.c:54

See full stack here:
https://travis-ci.org/postgresql-cfbot/postgresql/builds/50833

2.  On Windows the build failed:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.46469

-- 
Thomas Munro
https://enterprisedb.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Bruce Momjian]

On Tue, Jun 25, 2019 at 02:28:00PM +0200, Peter Eisentraut wrote:
> On 2019-06-17 11:23, Antonin Houska wrote:
> > I'm thinking how to teach postmaster to accept FEBE protocol connections
> > temporarily, just to receive the key. The user applications like pg_ctl,
> > initdb or pg_upgrade would retrieve the key / password from the DBA, then
> > start postmaster and send it the key.
> > 
> > Perhaps the message format should be a bit generic so that extensions like
> > this can use it to receive their keys too.
> > 
> > (The idea of an unix socket or named pipe I proposed upthread is not good
> > because it's harder to implement in a portable way.)
> 
> How are the requirements here different from ssl_passphrase_command?
> Why do we need a new mechanism?

Agreed.  My pgcryptokey prompting shell script was mostly a
proof-of-concept.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Peter Eisentraut]

On 2019-06-25 15:39, Robert Haas wrote:
> On Tue, Jun 25, 2019 at 8:28 AM Peter Eisentraut
>  wrote:
>> How are the requirements here different from ssl_passphrase_command?
>> Why do we need a new mechanism?
> 
> I don't think that the requirements are different, and I don't think
> we need a new mechanism.
> 
> I am curious exactly how you would set up ssl_passphrase_command to
> prompt interactively.

For example like this:
https://www.2ndquadrant.com/en/blog/postgresql-passphrase-protected-ssl-keys-systemd/

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Tue, Jun 25, 2019 at 8:28 AM Peter Eisentraut
 wrote:
> How are the requirements here different from ssl_passphrase_command?
> Why do we need a new mechanism?

I don't think that the requirements are different, and I don't think
we need a new mechanism.

I am curious exactly how you would set up ssl_passphrase_command to
prompt interactively.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Peter Eisentraut]

On 2019-06-17 11:23, Antonin Houska wrote:
> I'm thinking how to teach postmaster to accept FEBE protocol connections
> temporarily, just to receive the key. The user applications like pg_ctl,
> initdb or pg_upgrade would retrieve the key / password from the DBA, then
> start postmaster and send it the key.
> 
> Perhaps the message format should be a bit generic so that extensions like
> this can use it to receive their keys too.
> 
> (The idea of an unix socket or named pipe I proposed upthread is not good
> because it's harder to implement in a portable way.)

How are the requirements here different from ssl_passphrase_command?
Why do we need a new mechanism?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Bruce Momjian  wrote:

> On Mon, Jun  3, 2019 at 12:04:54PM -0400, Robert Haas wrote:
> > 
> > What I'm talking about here is that it also has to be reasonably
> > possible to write an encryption key command that does something
> > useful.  I don't have a really clear vision for how that's going to
> > work.  Nobody wants the server, which is probably being launched by
> > pg_ctl or systemd or both, to prompt using its own stdin/stderr, but
> > the we need to think about how the prompting is actually going to
> > work.  One idea is to do it via the FEBE protocol: connect to the
> > server using libpq, issue a new command that causes the server to
> > enter COPY mode, and then send the encryption key as COPY data.
> > However, that idea doesn't really work, because we've got to be able
> > to get the key before we run recovery or reach consistency, so the
> > server won't be listening for connections at that point.  Also, we've
> > got to have a way for this to work in single-user mode, which also
> > can't listen for connections.  It's probably all fine if your
> > encryption key command is something like 'curl
> > https://my.secret.keyserver.me/sekcret.key' because then there's an
> > external server which you can just go access - but I don't quite
> > understand how you'd do interactive prompting from here.  Sorry to get
> > hung up on what may seem like a minor point, but I think it's actually
> > fairly fundamental: we've got to have a clear vision of how end-users
> > will really be able to make use of this.
> 
> pgcryptoey has an example of prompting from /dev/tty:
> 
>   http://momjian.us/download/pgcryptokey/
> 
> Look at pgcryptokey_default.sample.

It's a nice exercise of shell features but does not seem to be easily
portable, and it has some other restrictions. One is mentioned in a comment:

# Use 'pg_ctl -s' so starting dots do not interfere with password entry

I think that besides the dots, log messages can also be disturbing if logging
collector is not enabled. Another corner case might be there when user runs
pg_ctl with --no-wait option and wants to do run some other command
immediately.

I'm thinking how to teach postmaster to accept FEBE protocol connections
temporarily, just to receive the key. The user applications like pg_ctl,
initdb or pg_upgrade would retrieve the key / password from the DBA, then
start postmaster and send it the key.

Perhaps the message format should be a bit generic so that extensions like
this can use it to receive their keys too.

(The idea of an unix socket or named pipe I proposed upthread is not good
because it's harder to implement in a portable way.)

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Bruce Momjian]

On Thu, Jun 13, 2019 at 09:14:13PM -0400, Bruce Momjian wrote:
> On Mon, Jun  3, 2019 at 12:04:54PM -0400, Robert Haas wrote:
> > 
> > What I'm talking about here is that it also has to be reasonably
> > possible to write an encryption key command that does something
> > useful.  I don't have a really clear vision for how that's going to
> > work.  Nobody wants the server, which is probably being launched by
> > pg_ctl or systemd or both, to prompt using its own stdin/stderr, but
> > the we need to think about how the prompting is actually going to
> > work.  One idea is to do it via the FEBE protocol: connect to the
> > server using libpq, issue a new command that causes the server to
> > enter COPY mode, and then send the encryption key as COPY data.
> > However, that idea doesn't really work, because we've got to be able
> > to get the key before we run recovery or reach consistency, so the
> > server won't be listening for connections at that point.  Also, we've
> > got to have a way for this to work in single-user mode, which also
> > can't listen for connections.  It's probably all fine if your
> > encryption key command is something like 'curl
> > https://my.secret.keyserver.me/sekcret.key' because then there's an
> > external server which you can just go access - but I don't quite
> > understand how you'd do interactive prompting from here.  Sorry to get
> > hung up on what may seem like a minor point, but I think it's actually
> > fairly fundamental: we've got to have a clear vision of how end-users
> > will really be able to make use of this.
> 
> pgcryptoey has an example of prompting from /dev/tty:
> 
>   http://momjian.us/download/pgcryptokey/
> 
> Look at pgcryptokey_default.sample.

Also, look at pgcryptokey_default.c and its use with
shared_preload_libraries for a full example of prompting on server boot.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Bruce Momjian]

On Mon, Jun  3, 2019 at 12:04:54PM -0400, Robert Haas wrote:
> 
> What I'm talking about here is that it also has to be reasonably
> possible to write an encryption key command that does something
> useful.  I don't have a really clear vision for how that's going to
> work.  Nobody wants the server, which is probably being launched by
> pg_ctl or systemd or both, to prompt using its own stdin/stderr, but
> the we need to think about how the prompting is actually going to
> work.  One idea is to do it via the FEBE protocol: connect to the
> server using libpq, issue a new command that causes the server to
> enter COPY mode, and then send the encryption key as COPY data.
> However, that idea doesn't really work, because we've got to be able
> to get the key before we run recovery or reach consistency, so the
> server won't be listening for connections at that point.  Also, we've
> got to have a way for this to work in single-user mode, which also
> can't listen for connections.  It's probably all fine if your
> encryption key command is something like 'curl
> https://my.secret.keyserver.me/sekcret.key' because then there's an
> external server which you can just go access - but I don't quite
> understand how you'd do interactive prompting from here.  Sorry to get
> hung up on what may seem like a minor point, but I think it's actually
> fairly fundamental: we've got to have a clear vision of how end-users
> will really be able to make use of this.

pgcryptoey has an example of prompting from /dev/tty:

http://momjian.us/download/pgcryptokey/

Look at pgcryptokey_default.sample.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Pavlo Golub]

Greetings, Antonin.

You wrote 2019-06-05, 15:32:

> Robert Haas  wrote:

>> On Fri, May 31, 2019 at 2:59 AM Antonin Houska  wrote:
>> > > Sounds good.  I'm not quite sure how this is going to work.  Ideally
>> > > you'd like the encryption key command to fetch the key from something
>> > > like ssh-agent, or maybe pop up a window on the user's terminal with a
>> > > key prompt.  Just reading from stdin and writing to stdout is not
>> > > going to be very convenient...
>> >
>> > When I mentioned writing to stdout in my previous email, I viewed it from 
>> > the
>> > perspective of postmaster: whichever way the command gets the password from
>> > the DBA, it'll always write it to stdout and postmaster will read it from
>> > there. This was to address your objection that the executables do not 
>> > actually
>> > "return" strings.
>> 
>> So the part about returning strings is really just a wording issue:
>> the documentation needs to talk about data sent to stdout or wherever,
>> because that's what really happens, and somebody writing such a
>> command from scratch needs to know what it must do.
>> 
>> What I'm talking about here is that it also has to be reasonably
>> possible to write an encryption key command that does something
>> useful.  I don't have a really clear vision for how that's going to
>> work.  Nobody wants the server, which is probably being launched by
>> pg_ctl or systemd or both, to prompt using its own stdin/stderr, but
>> the we need to think about how the prompting is actually going to
>> work.

> Since you mentioned ssh-agent above, I think that postmaster can, instead of
> running a command, create an unix socket and read the key from there. (I refer
> only to the socket as a communication channel, not to the protocol - ssh-agent
> does not seem to actually send key over the socket.) Unlike the socket for
> backend connections, this one would only be readable by the user that runs
> postmaster, and would only exist during the encryption initialization.

> The simplest approach from the perspective of the DBA is that pg_ctl can write
> the key to the socket. Besides that we can also implement a separate utility
> to send the key, to be used in other special cases such as starting postgres
> via systemd.

> (If the unix socket is a problem on windows, we might need to use named pipe
> instead.)

Yes, that definitely a problem on Windows. Pipes seems reasonable. Are
mapped files are appropriate from the security point of view?

>> > The header comment in pg_upgrade.c indicates that this is because of 
>> > TOAST. So
>> > I think that dbnode and relfilenode are not preserved just because there 
>> > was
>> > no strong reason to do so by now.
>> 
>> Maybe you want to propose an independent patch making that change?

> I think of a separate diff in the encryption patch series. As the encryption
> is the only reason for this enhancement, I'm not sure if it deserves a
> separate CF entry. (Although it might be considered refactoring because
> eventually pg_upgrade won't need to handle the different relnodes, and thus it
> might become a little bit simpler.)




-- 
Kind regards,
 Pavlo  mailto:pavlo.go...@cybertec.at

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Robert Haas  wrote:

> On Fri, May 31, 2019 at 2:59 AM Antonin Houska  wrote:
> > > Sounds good.  I'm not quite sure how this is going to work.  Ideally
> > > you'd like the encryption key command to fetch the key from something
> > > like ssh-agent, or maybe pop up a window on the user's terminal with a
> > > key prompt.  Just reading from stdin and writing to stdout is not
> > > going to be very convenient...
> >
> > When I mentioned writing to stdout in my previous email, I viewed it from 
> > the
> > perspective of postmaster: whichever way the command gets the password from
> > the DBA, it'll always write it to stdout and postmaster will read it from
> > there. This was to address your objection that the executables do not 
> > actually
> > "return" strings.
> 
> So the part about returning strings is really just a wording issue:
> the documentation needs to talk about data sent to stdout or wherever,
> because that's what really happens, and somebody writing such a
> command from scratch needs to know what it must do.
> 
> What I'm talking about here is that it also has to be reasonably
> possible to write an encryption key command that does something
> useful.  I don't have a really clear vision for how that's going to
> work.  Nobody wants the server, which is probably being launched by
> pg_ctl or systemd or both, to prompt using its own stdin/stderr, but
> the we need to think about how the prompting is actually going to
> work.

Since you mentioned ssh-agent above, I think that postmaster can, instead of
running a command, create an unix socket and read the key from there. (I refer
only to the socket as a communication channel, not to the protocol - ssh-agent
does not seem to actually send key over the socket.) Unlike the socket for
backend connections, this one would only be readable by the user that runs
postmaster, and would only exist during the encryption initialization.

The simplest approach from the perspective of the DBA is that pg_ctl can write
the key to the socket. Besides that we can also implement a separate utility
to send the key, to be used in other special cases such as starting postgres
via systemd.

(If the unix socket is a problem on windows, we might need to use named pipe
instead.)

> > The header comment in pg_upgrade.c indicates that this is because of TOAST. 
> > So
> > I think that dbnode and relfilenode are not preserved just because there was
> > no strong reason to do so by now.
> 
> Maybe you want to propose an independent patch making that change?

I think of a separate diff in the encryption patch series. As the encryption
is the only reason for this enhancement, I'm not sure if it deserves a
separate CF entry. (Although it might be considered refactoring because
eventually pg_upgrade won't need to handle the different relnodes, and thus it
might become a little bit simpler.)

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Fri, May 31, 2019 at 2:59 AM Antonin Houska  wrote:
> > Sounds good.  I'm not quite sure how this is going to work.  Ideally
> > you'd like the encryption key command to fetch the key from something
> > like ssh-agent, or maybe pop up a window on the user's terminal with a
> > key prompt.  Just reading from stdin and writing to stdout is not
> > going to be very convenient...
>
> When I mentioned writing to stdout in my previous email, I viewed it from the
> perspective of postmaster: whichever way the command gets the password from
> the DBA, it'll always write it to stdout and postmaster will read it from
> there. This was to address your objection that the executables do not actually
> "return" strings.

So the part about returning strings is really just a wording issue:
the documentation needs to talk about data sent to stdout or wherever,
because that's what really happens, and somebody writing such a
command from scratch needs to know what it must do.

What I'm talking about here is that it also has to be reasonably
possible to write an encryption key command that does something
useful.  I don't have a really clear vision for how that's going to
work.  Nobody wants the server, which is probably being launched by
pg_ctl or systemd or both, to prompt using its own stdin/stderr, but
the we need to think about how the prompting is actually going to
work.  One idea is to do it via the FEBE protocol: connect to the
server using libpq, issue a new command that causes the server to
enter COPY mode, and then send the encryption key as COPY data.
However, that idea doesn't really work, because we've got to be able
to get the key before we run recovery or reach consistency, so the
server won't be listening for connections at that point.  Also, we've
got to have a way for this to work in single-user mode, which also
can't listen for connections.  It's probably all fine if your
encryption key command is something like 'curl
https://my.secret.keyserver.me/sekcret.key' because then there's an
external server which you can just go access - but I don't quite
understand how you'd do interactive prompting from here.  Sorry to get
hung up on what may seem like a minor point, but I think it's actually
fairly fundamental: we've got to have a clear vision of how end-users
will really be able to make use of this.

> The header comment in pg_upgrade.c indicates that this is because of TOAST. So
> I think that dbnode and relfilenode are not preserved just because there was
> no strong reason to do so by now.

Maybe you want to propose an independent patch making that change?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Robert Haas  wrote:

> On Tue, May 28, 2019 at 11:27 AM Antonin Houska  wrote:
> > We thought that it's more convenient for administrator to enter password. To
> > achieve that, we can instead tell the admin how to use the key derivation 
> > tool
> > (pg_keysetup) and send its output to postgres. So yes, it's possible to 
> > always
> > receive the key from the "encryption key command". I'll change this.
> 
> Sounds good.  I'm not quite sure how this is going to work.  Ideally
> you'd like the encryption key command to fetch the key from something
> like ssh-agent, or maybe pop up a window on the user's terminal with a
> key prompt.  Just reading from stdin and writing to stdout is not
> going to be very convenient...

When I mentioned writing to stdout in my previous email, I viewed it from the
perspective of postmaster: whichever way the command gets the password from
the DBA, it'll always write it to stdout and postmaster will read it from
there. This was to address your objection that the executables do not actually
"return" strings.

> > > +pg_upgrade. (Therefore the encrypted cluster does not support pg_upgrade 
> > > with
> > > +the --link option.)
> > >
> > > That seems pretty unfortunate.  Any way to do better?
> >
> >
> > Currently I'm thinking of making the IV less dependent on RelFileNode
> > (e.g. generate a random number for which RelFileNode serves as the seed) and
> > storing it in pg_class as a storage parameter.
> 
> I don't think it's a very good idea.  For one thing, you can't store
> the IV for pg_class in pg_class; that has a circularity problem.  For
> another thing, I'm not sure we can or should try to do catalog access
> from every place where an IV might be needed.  In fact, I'd go so far
> as to say that sounds like a recipe for a lot of pain and fragility.
> 
> One potential approach is to modify pg_upgrade to preserve the dbnode
> and relfilenode.  I don't know of any fundamental reason why such a
> change shouldn't be possible, although it is probably a bunch of work,
> and there may be problems with which I am not acquainted.

Actually it doesn't seem to be that tricky. In the --binary-upgrade mode,
pg_dump does record pg_class.oid:

-- For binary upgrade, must preserve pg_class oids
SELECT 
pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('57465'::pg_catalog.oid);

The header comment in pg_upgrade.c indicates that this is because of TOAST. So
I think that dbnode and relfilenode are not preserved just because there was
no strong reason to do so by now.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Tue, May 28, 2019 at 11:27 AM Antonin Houska  wrote:
> We thought that it's more convenient for administrator to enter password. To
> achieve that, we can instead tell the admin how to use the key derivation tool
> (pg_keysetup) and send its output to postgres. So yes, it's possible to always
> receive the key from the "encryption key command". I'll change this.

Sounds good.  I'm not quite sure how this is going to work.  Ideally
you'd like the encryption key command to fetch the key from something
like ssh-agent, or maybe pop up a window on the user's terminal with a
key prompt.  Just reading from stdin and writing to stdout is not
going to be very convenient...

> Maintenance of a long patch series requires additional effort and I was busy
> enough with major code rearrangements so far. I'll continue splitting the
> stuff into more diffs.

Right, I understand.  Whatever can be split off can be reviewed and
committed separately, which will start to ease the burden of
maintaining the patch set.  I hope.  But getting over the initial hump
can be a lot of work.

> > +pg_upgrade. (Therefore the encrypted cluster does not support pg_upgrade 
> > with
> > +the --link option.)
> >
> > That seems pretty unfortunate.  Any way to do better?
>
> The reason is that the initialization vector (IV) for relation page encryption
> is derived from RelFileNode, and both dbNode and relNode can be different in
> the new cluster. Therefore the data of the old cluster need to be decrypted
> and encrypted using the new IV before it's copied to the new cluster. That's
> why we can't use symlink.
>
> As an alternative, I thought of deriving the IV by hashing the
> .. string, but that would mean that the
> "reencryption" is triggered by commands like "ALTER TABLE ... RENAME TO ...",
> "ALTER TABLE ... SET SCHEMA ...", etc.
>
> Currently I'm thinking of making the IV less dependent on RelFileNode
> (e.g. generate a random number for which RelFileNode serves as the seed) and
> storing it in pg_class as a storage parameter. Therefore we wouldn't have to
> pay any extra attention to transfer of the IV to the new cluster. However, the
> IV storage parameter would need special treatment:
>
> 1. DBA should not be able to remove or change it using ALTER TABLE command
> because inability to decrypt the data can be the only outcome.
>
> 2. The \d+ command of psql client should not display the IV. Displaying it
> probably wouldn't be a security risk, but as we encrypt the whole instance,
> the IV would appear for every single table and that might be annoying.
>
> What do you think about this approach?

I don't think it's a very good idea.  For one thing, you can't store
the IV for pg_class in pg_class; that has a circularity problem.  For
another thing, I'm not sure we can or should try to do catalog access
from every place where an IV might be needed.  In fact, I'd go so far
as to say that sounds like a recipe for a lot of pain and fragility.

One potential approach is to modify pg_upgrade to preserve the dbnode
and relfilenode.  I don't know of any fundamental reason why such a
change shouldn't be possible, although it is probably a bunch of work,
and there may be problems with which I am not acquainted.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Thanks of another round of review.

Robert Haas  wrote:

> On Wed, Apr 24, 2019 at 10:48 AM Antonin Houska  wrote:
> > Attached is the next version. It tries to address various problems pointed 
> > out
> > upthread, including documentation.
> 
> It's not immediately evident to me, perhaps because I haven't looked
> at this stuff in a while, what the purpose of 0001 and 0002 is.  I
> think it'd be helpful if you would generate these patches with 'git
> format-patch' and include a meaningful commit message in each patch of
> the series.

ok, will do.

> +   
> +This setting specifies path to executable file that returns
> +either encryption_key= for key
> +and encryption_password= for password.
> +   
> 
> This is incomprehensible.  Executables don't return strings.

That's right, the key should be written to the standard output.

> Also, you can return "either a or b" or you can return "both a and b," but
> you can't return "either a and b".

Sure, this is a thinko.

> - Build with support for SSL (encrypted)
> - connections. This requires the OpenSSL
> - package to be installed.  configure will check
> - for the required header files and libraries to make sure that
> + Build with support for on-disk data encryption
> + or SSL (encrypted) connections. This requires
> + the OpenSSL package to be
> + installed.  configure will check for the
> + required header files and libraries to make sure that
> 
> I think we should instead just remove the word 'connections'.  We
> don't want to have multiple places in the documentation listing all
> the things for which OpenSSL is required.

ok

> Generally, the documentation changes here have a lot of stuff about
> key-or-password, which is quite confusing.  It seems like the idea is
> that you accept either a key or a password from which a key is
> derived, but I don't think that's explained super-clearly, and I
> wonder whether it's unnecessary complexity.  Why not just require a
> key always?

We thought that it's more convenient for administrator to enter password. To
achieve that, we can instead tell the admin how to use the key derivation tool
(pg_keysetup) and send its output to postgres. So yes, it's possible to always
receive the key from the "encryption key command". I'll change this.

> Another general point is that the documentation, comments, and README
> need some work on the English.  They contain lots of information, but
> they read somewhat awkwardly and are hard to understand in some
> places.

ok, I'll try to explain the tricky things in simple senteces.

> Wherever you are adding a large hunk of code into the middle of an
> existing function (e.g. XLogWrite), please consider putting the logic
> in a new function and calling it from the original function, instead
> of just inlining the entire thing.

ok

> xloginsert.c adds a new #include even though there are no other
> changes to that file.  That should not be necessary.  If it is, you've
> broken the principle the header files are compilable independently of
> other header files (except postgres.h, which is a prerequisite for
> every header file).

It's not intentional, I failed to remove it when moving some encryption
related function calls elsewhere.

> -XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr,
> - Size count)
> +XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr,
> Size count)

> Spurious hunk.  Please try to avoid unnecessary changes.

Maybe I added and later removed some argument and forgot to enforce the
maximum line length. I expected pg_indent to fix things like this but it
apparently does not. I'll pay more attention to such things next time.

> + * XXX Currently the function is only called with startptr at page
> + * boundary. If it should change, encryption needs to reflect this fact,
> + * i.e. read data from the beginning of the page. Actually this is a
> + * reason to adjust and use walsender.c:XLogRead().
> 
> This comment and the similar comment at the end of the function are
> not very clear.   It sorta sounds, though, like maybe the decryption
> logic should move from here to the caller or something.  You seem to
> be saying that, after your changes, this function depends on the
> caller using it in a certain way, which is often a sign that you
> haven't abstracted things in the right way.  And if walsender.c's
> version of the logic is better, why not also do that stuff here?

Please consider this an incomplete implementation of the XLOG
decryption. Eventually these restrictions will be relaxed and the decryption
will be hidden from caller. Currently there are two implementations of
XLogRead() in the backend (plus one in the frontend) and I didn't want to
spend too much effort on merging the decryption into core until this another
patch gets committed:

https://commitfest.postgresql.org/23/2098/

The current XLogRead() functions 

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Wed, Apr 24, 2019 at 10:48 AM Antonin Houska  wrote:
> Attached is the next version. It tries to address various problems pointed out
> upthread, including documentation.

It's not immediately evident to me, perhaps because I haven't looked
at this stuff in a while, what the purpose of 0001 and 0002 is.  I
think it'd be helpful if you would generate these patches with 'git
format-patch' and include a meaningful commit message in each patch of
the series.

+   
+This setting specifies path to executable file that returns
+either encryption_key= for key
+and encryption_password= for password.
+   

This is incomprehensible.  Executables don't return strings.  Also,
you can return "either a or b" or you can return "both a and b," but
you can't return "either a and b".

- Build with support for SSL (encrypted)
- connections. This requires the OpenSSL
- package to be installed.  configure will check
- for the required header files and libraries to make sure that
+ Build with support for on-disk data encryption
+ or SSL (encrypted) connections. This requires
+ the OpenSSL package to be
+ installed.  configure will check for the
+ required header files and libraries to make sure that

I think we should instead just remove the word 'connections'.  We
don't want to have multiple places in the documentation listing all
the things for which OpenSSL is required.

Generally, the documentation changes here have a lot of stuff about
key-or-password, which is quite confusing.  It seems like the idea is
that you accept either a key or a password from which a key is
derived, but I don't think that's explained super-clearly, and I
wonder whether it's unnecessary complexity.  Why not just require a
key always?

Another general point is that the documentation, comments, and README
need some work on the English.  They contain lots of information, but
they read somewhat awkwardly and are hard to understand in some
places.

Wherever you are adding a large hunk of code into the middle of an
existing function (e.g. XLogWrite), please consider putting the logic
in a new function and calling it from the original function, instead
of just inlining the entire thing.

xloginsert.c adds a new #include even though there are no other
changes to that file.  That should not be necessary.  If it is, you've
broken the principle the header files are compilable independently of
other header files (except postgres.h, which is a prerequisite for
every header file).

-XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr,
- Size count)
+XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr,
Size count)

Spurious hunk.  Please try to avoid unnecessary changes.

+ * XXX Currently the function is only called with startptr at page
+ * boundary. If it should change, encryption needs to reflect this fact,
+ * i.e. read data from the beginning of the page. Actually this is a
+ * reason to adjust and use walsender.c:XLogRead().

This comment and the similar comment at the end of the function are
not very clear.   It sorta sounds, though, like maybe the decryption
logic should move from here to the caller or something.  You seem to
be saying that, after your changes, this function depends on the
caller using it in a certain way, which is often a sign that you
haven't abstracted things in the right way.  And if walsender.c's
version of the logic is better, why not also do that stuff here?

@@ -52,7 +54,6 @@

 uint32 bootstrap_data_checksum_version = 0; /* No checksum */

-
 #define ALLOC(t, c) \
  ((t *) MemoryContextAllocZero(TopMemoryContext, (unsigned)(c) * sizeof(t)))

Another useless hunk.

+ {
+ RelFileNode fromNode = {srctablespace, src_dboid, InvalidOid};
+ RelFileNode toNode = {dsttablespace, dboid, InvalidOid};
+
+ copydir(srcpath, dstpath, , );
+ }

I suggest instead declaring these variables in the surrounding scope
and initializing them here.  This style is unlike most PostgreSQL
code.

+ case WAIT_EVENT_KDF_FILE_READ:
+ event_name = "KDFFileRead";
+ break;

New wait events need documentation.

+are encrypted differently. Furthermore, as the Postgres page starts with LSN,
+even if only the last encryption block of the page changes, the whole cipher
+text of the page will be different after the next encryption.

But for a hint-bit-only change, this would not be true, unless we
force wal_log_hints.  Sounds like we're doing that, but it might be
worth mentioning it here also.

There's also the question of unlogged tables, which do not have an
LSN.  This logic wouldn't apply there, which might be a problem.

+If the encryption is enabled, the following requirements need to be taken into
+account:
+
+1. The file buffer cannot be positioned at arbitrary offset of the file. If

I think it's no good at all to think about enforcing requirements like
this only when data_encryption is used.  That's just going to lead to

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Bruce Momjian]

On Sat, Apr  6, 2019 at 03:29:13PM +0200, Antonin Houska wrote:
> Robert Haas  wrote:
> 
> > On Fri, Apr 5, 2019 at 11:22 AM Antonin Houska  wrote:
> > > > Wouldn't Tom's proposal to use a stream cipher fix all this?
> > >
> > > Yes it would make the extra alignment unnecessary, but our solution tries 
> > > to
> > > meet specific requirements of disk encryption. Stream cipher appears to be
> > > incompatible with these requirements:
> > >
> > > https://en.wikipedia.org/wiki/Disk_encryption_theory
> > 
> > Hmm.  Well, I don't know what to do about that, but I think this patch
> > is going to be facing an uphill battle if the encrypted and
> > unencrypted WAL formats use different alignment.
> 
> Once you said this could be a problem, I thought about it a bit more. The link
> above tells that stream cipher is not good for disk encryption because it's
> risky to encrypt different data (e.g. old an new version of disk sector) with
> the same key (or rather with the same combination of key and initialization
> vector).
> 
> However WAL is a special case because here we should never need to change the
> data at given position. The only exception is that unused space (sequence of
> zeroes) becomes a valid record. So if we avoid encryption of the unused space,
> it might be o.k. to use the stream cipher for WAL.

Agreed.  Requirement #2 is not needed for WAL:

https://en.wikipedia.org/wiki/Disk_encryption_theory#Problem_definition
2. Data retrieval and storage should both be fast operations, no
matter where on the disk the data is stored.

because you are not writing into random parts of the WAL.

Using the same stream cipher system that SSH uses would make sense,
since it is byte-oriented, and it only encrypts the tail.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Robert Haas  wrote:

> On Fri, Apr 5, 2019 at 11:22 AM Antonin Houska  wrote:
> > > Wouldn't Tom's proposal to use a stream cipher fix all this?
> >
> > Yes it would make the extra alignment unnecessary, but our solution tries to
> > meet specific requirements of disk encryption. Stream cipher appears to be
> > incompatible with these requirements:
> >
> > https://en.wikipedia.org/wiki/Disk_encryption_theory
> 
> Hmm.  Well, I don't know what to do about that, but I think this patch
> is going to be facing an uphill battle if the encrypted and
> unencrypted WAL formats use different alignment.

Once you said this could be a problem, I thought about it a bit more. The link
above tells that stream cipher is not good for disk encryption because it's
risky to encrypt different data (e.g. old an new version of disk sector) with
the same key (or rather with the same combination of key and initialization
vector).

However WAL is a special case because here we should never need to change the
data at given position. The only exception is that unused space (sequence of
zeroes) becomes a valid record. So if we avoid encryption of the unused space,
it might be o.k. to use the stream cipher for WAL.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Fri, Apr 5, 2019 at 11:22 AM Antonin Houska  wrote:
> > Wouldn't Tom's proposal to use a stream cipher fix all this?
>
> Yes it would make the extra alignment unnecessary, but our solution tries to
> meet specific requirements of disk encryption. Stream cipher appears to be
> incompatible with these requirements:
>
> https://en.wikipedia.org/wiki/Disk_encryption_theory

Hmm.  Well, I don't know what to do about that, but I think this patch
is going to be facing an uphill battle if the encrypted and
unencrypted WAL formats use different alignment.

> Currently we try to use the CBC-ESSIV mode. It's worth to mention that in this
> mode, if the page is encrypted twice and if the plain data in the encryption
> block N (i.e. block of 16 bytes) changes before the 2nd encryption, the
> encrypted data will only change in blocks >= N. So the problem of losing
> already flushed WAL records should not happen.

Well, this is just a question of alignment.  If WAL records are at
least 16-byte aligned, then it should be fine.  But I have a feeling
they may just be MAXALIGN'd.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Robert Haas  wrote:

> On Thu, Mar 21, 2019 at 7:46 AM Antonin Houska  wrote:
> > Nevertheless, with the current version of our patch, PG should be resistant
> > against such a partial write anyway because we chose to align XLOG records 
> > to
> > 16 bytes (as long as the encryption is enabled) for the following reasons:
> >
> > If one XLOG record ends and the following one starts in the same encryption
> > block, both records can get corrupted during streaming replication. The
> > scenario looks like: 1) the first record is written on master (the unused 
> > part
> > of the block contains zeroes), 2) the block is encrypted and its initial 
> > part
> > (i.e. the number of bytes occupied by the first record in the plain text) is
> > streamed to slave, 3) the second record is written on master, 4) the
> > containing encryption block is encrypted again and the trailing part (i.e. 
> > the
> > number of bytes occupied by the second record) is streamed, 5) decryption of
> > the block on slave will produce garbage and thus corrupt both records. This 
> > is
> > because the trailing part of the block was filled with zeroes during
> > encryption, but it contains different data at decryption time.
> 
> Wouldn't Tom's proposal to use a stream cipher fix all this?

Yes it would make the extra alignment unnecessary, but our solution tries to
meet specific requirements of disk encryption. Stream cipher appears to be
incompatible with these requirements:

https://en.wikipedia.org/wiki/Disk_encryption_theory

Currently we try to use the CBC-ESSIV mode. It's worth to mention that in this
mode, if the page is encrypted twice and if the plain data in the encryption
block N (i.e. block of 16 bytes) changes before the 2nd encryption, the
encrypted data will only change in blocks >= N. So the problem of losing
already flushed WAL records should not happen.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Thu, Apr 4, 2019 at 11:52 AM Antonin Houska  wrote:
> Robert Haas  wrote:
> > I'm willing to put some effort into trying to get this into v13 if
> > you're willing to keep hacking on it, but there's probably a fair
> > amount to do and a year can go by in a hurry.
>
> That'd be appreciated, especially by my boss. It doesn't seem likely that in
> this situation I'll be assigned many other tasks of higher priority. So yes, I
> expect to have quite some time for this patch. Thanks.

Great.  I think it will be appreciated by my boss, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Robert Haas  wrote:

> I'm willing to put some effort into trying to get this into v13 if
> you're willing to keep hacking on it, but there's probably a fair
> amount to do and a year can go by in a hurry.

That'd be appreciated, especially by my boss. It doesn't seem likely that in
this situation I'll be assigned many other tasks of higher priority. So yes, I
expect to have quite some time for this patch. Thanks.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Thu, Apr 4, 2019 at 9:57 AM Antonin Houska  wrote:
> I think I finally understand. Originally I thought the question is how to
> compute correct page checksum while the hint bits can be changed w/o exclusive
> lock on the buffer. Now I realize that it's more about *recovery*: if the hint
> bit change is followed by a torn page write, the hint bit can get changed on
> disk but the checksum might not get updated. The wrong checksum is detected
> during recovery, but if XLOG does not contain the corresponding full page
> image, we're not able to recover.
>
> And with encryption, the consequence is even worse because torn page write
> causes not only wrong checksum of otherwise useful page, but really damaged
> page.

Correct.

> I'll enforce the FPW in the next version of the patch.

Cool.

I'm willing to put some effort into trying to get this into v13 if
you're willing to keep hacking on it, but there's probably a fair
amount to do and a year can go by in a hurry.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Robert Haas  wrote:

> On Thu, Mar 14, 2019 at 9:26 AM Antonin Houska  wrote:
> > > Hint bits also rely on this principle.  I thought there might be some
> > > interaction between this work and wal_log_hints for this reason, but I see
> > > nothing of that sort in the patch.
> >
> > I'm not sure if the hint bit is still a problem if we first copy the shared
> > buffer to backend-local memory and encrypt it there. That's what the patch
> > does.
> 
> That has the same problem that I described in the previous paragraph,
> except for the relation data files rather than the WAL itself.  See
> the manual's description of wal_log_hints:
> 
>
> When this parameter is on, the
> PostgreSQL
> server writes the entire content of each disk page to WAL during the
> first modification of that page after a checkpoint, even for
> non-critical modifications of so-called hint bits.
>
> 
> For encryption to work, you need that.  A hint bit write might convert
> the page to garbage, just as in the previous example, and this option
> make sure that if that happens, there will be an FPW, allowing you to
> recover...

I think I finally understand. Originally I thought the question is how to
compute correct page checksum while the hint bits can be changed w/o exclusive
lock on the buffer. Now I realize that it's more about *recovery*: if the hint
bit change is followed by a torn page write, the hint bit can get changed on
disk but the checksum might not get updated. The wrong checksum is detected
during recovery, but if XLOG does not contain the corresponding full page
image, we're not able to recover.

And with encryption, the consequence is even worse because torn page write
causes not only wrong checksum of otherwise useful page, but really damaged
page.

I'll enforce the FPW in the next version of the patch.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Thu, Mar 21, 2019 at 7:46 AM Antonin Houska  wrote:
> Nevertheless, with the current version of our patch, PG should be resistant
> against such a partial write anyway because we chose to align XLOG records to
> 16 bytes (as long as the encryption is enabled) for the following reasons:
>
> If one XLOG record ends and the following one starts in the same encryption
> block, both records can get corrupted during streaming replication. The
> scenario looks like: 1) the first record is written on master (the unused part
> of the block contains zeroes), 2) the block is encrypted and its initial part
> (i.e. the number of bytes occupied by the first record in the plain text) is
> streamed to slave, 3) the second record is written on master, 4) the
> containing encryption block is encrypted again and the trailing part (i.e. the
> number of bytes occupied by the second record) is streamed, 5) decryption of
> the block on slave will produce garbage and thus corrupt both records. This is
> because the trailing part of the block was filled with zeroes during
> encryption, but it contains different data at decryption time.

Wouldn't Tom's proposal to use a stream cipher fix all this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Tom Lane  wrote:

> Robert Haas  writes:
> > If the WAL *is* encrypted, the state at this point is that the block
> > is unreadable, because the first 4kB of the block is the first half of
> > the bits that resulted from encrypting 8kB of data that includes the
> > new record, and the second 4kB of the block is the second half of the
> > bits that resulted from encrypting 8kB of data that did not include
> > the new record, and since the new record perturbed every bit on the
> > page with probability ~50%, what that means is you now have garbage.
> > That means that not only did we lose the new record, but we also lost
> > the 3.5kB of good data which the page previously contained.  That's
> > NOT ok.  Some of the changes associated with those WAL records may
> > have been flushed to disk already, or there may be commits in there
> > that were acknowledged to the client, and we can't just lose them.
> 
> ISTM that this is only a problem if you choose the wrong encryption
> method.  One not-wrong encryption method is to use a stream cipher
> --- maybe that's not the exact right technical term, but anyway
> I'm talking about a method which notionally XOR's the cleartext
> data with a random bit stream generated from the encryption key
> (probably along with other knowable inputs such as the block number).
> In such a method, corruption of individual on-disk bytes doesn't
> prevent you from getting the correct decryption of on-disk bytes
> that aren't corrupted.

We actually use a block cipher (with block size 16 bytes), as opposed to
stream cipher. It's true that partial write is a problem because if a single
bit of the cipher text changed, decryption will produce 16 bytes of
garbage. However I'm not sure if partial write can affect as small unit as 16
bytes.

Nevertheless, with the current version of our patch, PG should be resistant
against such a partial write anyway because we chose to align XLOG records to
16 bytes (as long as the encryption is enabled) for the following reasons:

If one XLOG record ends and the following one starts in the same encryption
block, both records can get corrupted during streaming replication. The
scenario looks like: 1) the first record is written on master (the unused part
of the block contains zeroes), 2) the block is encrypted and its initial part
(i.e. the number of bytes occupied by the first record in the plain text) is
streamed to slave, 3) the second record is written on master, 4) the
containing encryption block is encrypted again and the trailing part (i.e. the
number of bytes occupied by the second record) is streamed, 5) decryption of
the block on slave will produce garbage and thus corrupt both records. This is
because the trailing part of the block was filled with zeroes during
encryption, but it contains different data at decryption time.

Alternative approach to this replication problem is that walsender decrypts
the stream and walreceiver encrypts it again. While this can provide us with
the advantage to have master and slave encrypted with different keys, this
approach brings some additional complexity. For example, pg_basebackup would
need to deal with encryption.

This design decision can be changed, but there's one more thing to consider:
if the XLOG stream is decrypted, the decryption cannot be disabled unless the
XLOG records are aligned to 16 bytes (and in turn, the XLOG alignment cannot
be enabled w/o initdb).

-- 
Antonin Houska
https://www.cybertec-postgresql.com

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Fri, Mar 15, 2019 at 5:10 PM Tom Lane  wrote:
> ISTM that this is only a problem if you choose the wrong encryption
> method.  One not-wrong encryption method is to use a stream cipher
> --- maybe that's not the exact right technical term, but anyway
> I'm talking about a method which notionally XOR's the cleartext
> data with a random bit stream generated from the encryption key
> (probably along with other knowable inputs such as the block number).
> In such a method, corruption of individual on-disk bytes doesn't
> prevent you from getting the correct decryption of on-disk bytes
> that aren't corrupted.

Oh, that seems like it might be a really good idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Tom Lane]

Robert Haas  writes:
> If the WAL *is* encrypted, the state at this point is that the block
> is unreadable, because the first 4kB of the block is the first half of
> the bits that resulted from encrypting 8kB of data that includes the
> new record, and the second 4kB of the block is the second half of the
> bits that resulted from encrypting 8kB of data that did not include
> the new record, and since the new record perturbed every bit on the
> page with probability ~50%, what that means is you now have garbage.
> That means that not only did we lose the new record, but we also lost
> the 3.5kB of good data which the page previously contained.  That's
> NOT ok.  Some of the changes associated with those WAL records may
> have been flushed to disk already, or there may be commits in there
> that were acknowledged to the client, and we can't just lose them.

ISTM that this is only a problem if you choose the wrong encryption
method.  One not-wrong encryption method is to use a stream cipher
--- maybe that's not the exact right technical term, but anyway
I'm talking about a method which notionally XOR's the cleartext
data with a random bit stream generated from the encryption key
(probably along with other knowable inputs such as the block number).
In such a method, corruption of individual on-disk bytes doesn't
prevent you from getting the correct decryption of on-disk bytes
that aren't corrupted.

regards, tom lane

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Thu, Mar 14, 2019 at 9:26 AM Antonin Houska  wrote:
> The point of #ifdef USE_ENCRYPTION is that we rely on OpenSSL, so the code
> needs to compile even w/o OpenSSL (of course the encryption won't be enabled
> in that case). I'll try to reduce the use of this construct only to the code
> blocks that really reference the OpenSSL functions.

That sounds like a good place to start.  The other part will be
avoiding referencing the OpenSSL functions from too many places.  For
example, right now we have secure_read() -- which I know some people
hate, or at least hate the name of] -- which lets you send data to the
client without having to care whether that data is going to be
SSL-encrypted on its way over the network.  One can argue about
whether that particular abstraction is properly-designed, but as Tom
recently felt the urge to remind me on another thread, the value of
abstraction in general is not in doubt.  I think we need one here, so
that the SSL-specific bits can be isolated in a relatively small
number of files, and then clients can ask to write a file, or write a
block to a file, or whatever, and ignore the question of whether that
is going to get encrypted somehow behind the scenes.

> Since buffile.c needs to handle this kind of problem (and it already does in
> the last patch version), I think even other than temporary files could be
> handled by this module. The patch below adds functions BufFileOpenTransient(),
> BufFileWriteTransient(), etc. that can replace OpenTransientFile, write(),
> etc. respectively. Once we implement the encryption, these functions will also
> hide handling of the encryption blocks from user. In this (preliminary) patch
> As an example, I applied this approach to ReorderBufferSerializeChange() and
> the function seems a bit simpler now. (ReorderBufferRestoreChange() would need
> more work to adopt this approach.)

Yeah, I don't have time to look at this in detail right now, but this
kind of thing seems like a promising approach to me.

> > The interaction of this capability with certain tricks that PostgreSQL
> > plays needs some thought -- and documentation, at least developer
> > documentation, maybe user documentation.  One area is recovery.
> > Writing WAL relies on the fact that if you are in the midst of
> > rewriting a block and the power fails, bytes that are the same in both
> > the old and new block images will be undisturbed; encryption will make
> > that false.  How's that going to work?
>
> Yes, if only a single bit is different, decryption will turn the whole
> encryption block (16 bytes) into garbage. So we need to enforce
> full_page_writes=on if encryption is enabled. The last version of the patch
> does not do that.

I believe that we need full_page_writes=on and wal_log_hints=on, but
they don't fix the problem that I'm describing.  Suppose that the very
last page in the write-ahead log contains 3.5kB of good data and 7kB
of zeroes.  Now, somebody comes along and writes a new write-ahead log
record that is 1k in size.  In memory, we update the block: it now
contains 4.5kB of good data and 6.5kB of zeroes.  Now, as we're
writing the block to do disk, the power is removed.  Consequently, the
first 4kB hits the platter and the other 4kB disappears into the void.

If the WAL is not encrypted, the state at this point is that the 3.5kB
of good data that we had previously is still there, and the first 500
bytes of the new WAL record are also there, and the rest is missing.
Recovery will detect that there's a valid-looking record at end of
WAL, but when it tries to checksum that record, it will fail, and so
that trailing half-record will just get ignored.  And that's fine.

If the WAL *is* encrypted, the state at this point is that the block
is unreadable, because the first 4kB of the block is the first half of
the bits that resulted from encrypting 8kB of data that includes the
new record, and the second 4kB of the block is the second half of the
bits that resulted from encrypting 8kB of data that did not include
the new record, and since the new record perturbed every bit on the
page with probability ~50%, what that means is you now have garbage.
That means that not only did we lose the new record, but we also lost
the 3.5kB of good data which the page previously contained.  That's
NOT ok.  Some of the changes associated with those WAL records may
have been flushed to disk already, or there may be commits in there
that were acknowledged to the client, and we can't just lose them.

One idea I have for fixing this problem is to encrypt the individual
WAL records rather than the blocks.  That leaks some information, of
course, but maybe not in a way that really matters.

> > Hint bits also rely on this principle.  I thought there might be some
> > interaction between this work and wal_log_hints for this reason, but I see
> > nothing of that sort in the patch.
>
> I'm not sure if the hint bit is still a problem if we first copy the shared
> buffer to 

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Thanks for feedback!

Robert Haas  wrote:

> However, this is also quite invasive.  It changes a lot of code and it
> doesn't do so in a very predictable way.  It's not like you went
> through and replaced every call to write() with a call to
> SpecialEncyptionMagicWrite().  Rather, there are new #ifdef
> USE_ENCRYPTION blocks all over the code base.  I think it will be
> important to look for ways to refactor this functionality to reduce
> that sort of thing to the absolute minimum possible.  Encryption needs
> to be something that the code needs to know about here and there, but
> not everywhere.

The point of #ifdef USE_ENCRYPTION is that we rely on OpenSSL, so the code
needs to compile even w/o OpenSSL (of course the encryption won't be enabled
in that case). I'll try to reduce the use of this construct only to the code
blocks that really reference the OpenSSL functions.

> Some of the changes are things that could perhaps be done as separate,
> preparatory patches.  For instance, pgstat.c gets a heavy refactoring
> to make it do I/O in bigger chunks.  There's no reason that you
> couldn't separate some of those changes out, clean them up, and get
> them committed separately.  It would be more efficient even as things
> stand.  OK, well, there is one reason: we may be getting a
> shared-memory stats collector soon, and if we do, then the need for
> all of this will go away.  But the general point is valid: whatever is
> a useful cleanup apart from this patch should be done separately from
> this patch.

We'll omit the stats collector related changes so far because the patch [1] is
under active development, and that would require us to rebase our patch
often. And if the stats files will eventually go away, we won't need to
encrypt the statistics.

> As far as possible, we need to try to do things in the same way in the
> encrypted and non-encrypted cases.  For example, it's pretty hard to
> feel good about code like this:
> 
> + sz_hdr = sizeof(ReorderBufferDiskChange);
> + if (data_encrypted)
> +#ifdef USE_ENCRYPTION
> + sz_hdr = TYPEALIGN(ENCRYPTION_BLOCK, sz_hdr);
> +#else
> + ENCRYPTION_NOT_SUPPORTED_MSG;
> +#endif /* USE_ENCRYPTION */
> 
> You won't be able to have much confidence that this code works both
> with and without encryption unless you test it both ways, because the
> file format is different in those two cases, and not just by being
> encrypted.  That means that anyone who modifies this code in the
> future has two ways of breaking it.  They can break the normal case,
> or they can break the encrypted case.  If encryption were available as
> a sort of service that everyone could use, then that would probably be
> fine, but like I said above, I think something as invasive as this
> currently is will lead to a lot of complaints.

The problem I tried to solve here is that the whole encryption block (16
bytes) needs to be read and decrypted even if you need only part of its
data. In the snippet above I tried to ensure that the whole blocks are always
read, but I agree this approach is fragile.

Since buffile.c needs to handle this kind of problem (and it already does in
the last patch version), I think even other than temporary files could be
handled by this module. The patch below adds functions BufFileOpenTransient(),
BufFileWriteTransient(), etc. that can replace OpenTransientFile, write(),
etc. respectively. Once we implement the encryption, these functions will also
hide handling of the encryption blocks from user. In this (preliminary) patch
As an example, I applied this approach to ReorderBufferSerializeChange() and
the function seems a bit simpler now. (ReorderBufferRestoreChange() would need
more work to adopt this approach.)

> The code needs a lot more documentation, not only SGML documentation
> but also code comments and probably a README file someplace.

ok, we'll do that.

> The interaction of this capability with certain tricks that PostgreSQL
> plays needs some thought -- and documentation, at least developer
> documentation, maybe user documentation.  One area is recovery.
> Writing WAL relies on the fact that if you are in the midst of
> rewriting a block and the power fails, bytes that are the same in both
> the old and new block images will be undisturbed; encryption will make
> that false.  How's that going to work?

Yes, if only a single bit is different, decryption will turn the whole
encryption block (16 bytes) into garbage. So we need to enforce
full_page_writes=on if encryption is enabled. The last version of the patch
does not do that.

> Hint bits also rely on this principle.  I thought there might be some
> interaction between this work and wal_log_hints for this reason, but I see
> nothing of that sort in the patch.

I'm not sure if the hint bit is still a problem if we first copy the shared
buffer to backend-local memory and encrypt it there. That's what the patch
does.

> Full page writes seem related too; I don't know how something like this can
> be safe 

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Fri, Sep 14, 2018 at 10:04 AM Antonin Houska  wrote:
> Toshi Harada  wrote:
> > Even if you apply "data-at-rest-encryption-wip-2018.07.25.patch" to the 
> > master branch,
> > the same patch error will occur.
>
> The version attached to this email should be applicable to the current
> branch. Sorry for the delay.

I have a few comments on this patch.  It doesn't seem useful to go
through and make really detailed review comments on everything at this
stage, because it's pretty far from being committable, at least IMHO.
However, I have a few general thoughts.

First of all, this is an impressive piece of work.  It's obvious even
from looking at this quickly that a lot of energy has gone into making
this work, and it touches a lot of stuff, not without reason.  For
example, it's quite impressive that this touches things like the
write-ahead log, logical decoding, and the stats collector, all of
which write to disk.

However, this is also quite invasive.  It changes a lot of code and it
doesn't do so in a very predictable way.  It's not like you went
through and replaced every call to write() with a call to
SpecialEncyptionMagicWrite().  Rather, there are new #ifdef
USE_ENCRYPTION blocks all over the code base.  I think it will be
important to look for ways to refactor this functionality to reduce
that sort of thing to the absolute minimum possible.  Encryption needs
to be something that the code needs to know about here and there, but
not everywhere.

Some of the changes are things that could perhaps be done as separate,
preparatory patches.  For instance, pgstat.c gets a heavy refactoring
to make it do I/O in bigger chunks.  There's no reason that you
couldn't separate some of those changes out, clean them up, and get
them committed separately.  It would be more efficient even as things
stand.  OK, well, there is one reason: we may be getting a
shared-memory stats collector soon, and if we do, then the need for
all of this will go away.  But the general point is valid: whatever is
a useful cleanup apart from this patch should be done separately from
this patch.

As far as possible, we need to try to do things in the same way in the
encrypted and non-encrypted cases.  For example, it's pretty hard to
feel good about code like this:

+ sz_hdr = sizeof(ReorderBufferDiskChange);
+ if (data_encrypted)
+#ifdef USE_ENCRYPTION
+ sz_hdr = TYPEALIGN(ENCRYPTION_BLOCK, sz_hdr);
+#else
+ ENCRYPTION_NOT_SUPPORTED_MSG;
+#endif /* USE_ENCRYPTION */

You won't be able to have much confidence that this code works both
with and without encryption unless you test it both ways, because the
file format is different in those two cases, and not just by being
encrypted.  That means that anyone who modifies this code in the
future has two ways of breaking it.  They can break the normal case,
or they can break the encrypted case.  If encryption were available as
a sort of service that everyone could use, then that would probably be
fine, but like I said above, I think something as invasive as this
currently is will lead to a lot of complaints.

The code needs a lot more documentation, not only SGML documentation
but also code comments and probably a README file someplace.  There is
a lot of discussion in the comments here of encryption tweaks, but
there's no general introduction to the topic (what's a tweak?) or --
and I think this is important -- what the idea between the choice of
various tweaks in different places actually is.  There's probably some
motivating philosophy behind the way the tweaks are chosen that should
be explained someplace.  Think about it this way: if some hapless
developer, let's say me, wants to add a new module to PostgreSQL which
happens to write data to disk, that person needs an easy way to
understand what they need to do to make that new code play nice with
encryption.  Right now, the code for every existing module is a little
different (uggh) and there's this encryption tweak stuff that you
about which you have to do something intelligent, but there's nothing
that tells you how to be intelligent about it, at least not that I
see.  People are going to need something that looks more like a
formula that they can just copy, and a good introduction to the
principles in some appropriate place, too.

The interaction of this capability with certain tricks that PostgreSQL
plays needs some thought -- and documentation, at least developer
documentation, maybe user documentation.  One area is recovery.
Writing WAL relies on the fact that if you are in the midst of
rewriting a block and the power fails, bytes that are the same in both
the old and new block images will be undisturbed; encryption will make
that false.  How's that going to work?  Hint bits also rely on this
principle.  I thought there might be some interaction between this
work and wal_log_hints for this reason, but I see nothing of that sort
in the patch.  Full page writes seem related too; I don't know how

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Toshi Harada]

Hi.

Even if you apply "data-at-rest-encryption-wip-2018.07.25.patch" to the master 
branch, 
the same patch error will occur.
(Patch error of pg_rewind was eliminated)


patching file src/backend/replication/logical/reorderbuffer.c
Hunk #9 FAILED at 2464.

patching file src/bin/pg_upgrade/controldata.c
Hunk #1 FAILED at 58.

(See the attached file for the entire patch log)

Antonin Houska  wrote:
> Toshi Harada  wrote:
> 
> > Hi.
> > 
> > If "data-at-rest-encryption-wip-2018.07.25.patch" is applied to PostgreSQL 
> > 11-beta3 released last week, 
> > patch execution will fail as follows.
> 
> > 
> > patching file src/backend/replication/logical/reorderbuffer.c
> > Hunk #9 FAILED at 2464.
> > 
> > 1 out of 7 hunks FAILED -- saving rejects to file 
> > src/bin/pg_rewind/pg_rewind.c.rej
> > 
> > 1 out of 6 hunks FAILED -- saving rejects to file 
> > src/bin/pg_upgrade/controldata.c.rej
> 
> The patch should be applicable to the master branch.
> 
> -- 
> Antonin Houska
> Cybertec Schonig & Schonig GmbH
> Grohrmuhlgasse 26, A-2700 Wiener Neustadt
> Web: https://www.cybertec-postgresql.com
> 


master+patch.log
Description: Binary data

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Antonin Houska]

Toshi Harada  wrote:

> Hi.
> 
> If "data-at-rest-encryption-wip-2018.07.25.patch" is applied to PostgreSQL 
> 11-beta3 released last week, 
> patch execution will fail as follows.

> 
> patching file src/backend/replication/logical/reorderbuffer.c
> Hunk #9 FAILED at 2464.
> 
> 1 out of 7 hunks FAILED -- saving rejects to file 
> src/bin/pg_rewind/pg_rewind.c.rej
> 
> 1 out of 6 hunks FAILED -- saving rejects to file 
> src/bin/pg_upgrade/controldata.c.rej

The patch should be applicable to the master branch.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

"WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Toshi Harada]

Hi.

If "data-at-rest-encryption-wip-2018.07.25.patch" is applied to PostgreSQL 
11-beta3 released last week, 
patch execution will fail as follows.


patching file src/backend/replication/logical/reorderbuffer.c
Hunk #9 FAILED at 2464.

1 out of 7 hunks FAILED -- saving rejects to file 
src/bin/pg_rewind/pg_rewind.c.rej

1 out of 6 hunks FAILED -- saving rejects to file 
src/bin/pg_upgrade/controldata.c.rej



(See the attached file for the entire patch log)

Antonin Houska  wrote:
> Toshi Harada  wrote:
> 
> > Hi.
> > 
> > I applied the patch "WIP: Data at rest encryption" to PostgreSQL 11 - beta 
> > 2 and I'm working on it.
> > 
> > When this patch is applied, the following problem occurs.
> > 
> > * An error occurs when CHECKPOINT is executed during two-phase commit.
> > * After an error occurs, if you stop PostgreSQL, it will never start again.
> > 
> > (1) First, execute PREPARE TRANSACTION.
> > 
> > postgres=# BEGIN;
> > BEGIN
> > postgres=# PREPARE TRANSACTION 'foo';
> > PREPARE TRANSACTION
> > postgres=#
> > 
> > (2) Execute the CHECKPOINT command from another terminal.
> > CHEKPOINT command fails.
> > 
> > postgres=# CHECKPOINT;
> > ERROR:  checkpoint request failed
> > HINT:  Consult recent messages in the server log for details.
> > postgres=#
> 
> The patch version I posted in
> 
> https://www.postgresql.org/message-id/11678.1532519255%40localhost
> 
> fixes an issue (unitialized pointer) that caused failure here, but it was
> SEGFAULT rather than ERROR. And the scope of the bug was broader than just
> CHECKPOINT.
> 
> Can you please test it again with the new version of the patch?
> 
> Anyway, thanks for your reports!
> 
> 
> -- 
> Antonin Houska
> Cybertec Schonig & Schonig GmbH
> Grohrmuhlgasse 26, A-2700 Wiener Neustadt
> Web: https://www.cybertec-postgresql.com
> 


PG11-beta3+0725patch.log
Description: Binary data

Re: "WIP: Data at rest encryption" patch and, 2 phase commit.

[Toshi Harada]

Hi.

Applying https://www.postgresql.org/message-id/11678.1532519255%40localhost 
patch, 
the problem of pg_create_logical_replication_slot () and the 2PC problem were 
solved.

Thanks.

Antonin Houska  wrote:
> Toshi Harada  wrote:
> 
> > Hi.
> > 
> > I applied the patch "WIP: Data at rest encryption" to PostgreSQL 11 - beta 
> > 2 and I'm working on it.
> > 
> > When this patch is applied, the following problem occurs.
> > 
> > * An error occurs when CHECKPOINT is executed during two-phase commit.
> > * After an error occurs, if you stop PostgreSQL, it will never start again.
> > 
> > (1) First, execute PREPARE TRANSACTION.
> > 
> > postgres=# BEGIN;
> > BEGIN
> > postgres=# PREPARE TRANSACTION 'foo';
> > PREPARE TRANSACTION
> > postgres=#
> > 
> > (2) Execute the CHECKPOINT command from another terminal.
> > CHEKPOINT command fails.
> > 
> > postgres=# CHECKPOINT;
> > ERROR:  checkpoint request failed
> > HINT:  Consult recent messages in the server log for details.
> > postgres=#
> 
> The patch version I posted in
> 
> https://www.postgresql.org/message-id/11678.1532519255%40localhost
> 
> fixes an issue (unitialized pointer) that caused failure here, but it was
> SEGFAULT rather than ERROR. And the scope of the bug was broader than just
> CHECKPOINT.
> 
> Can you please test it again with the new version of the patch?
> 
> Anyway, thanks for your reports!
> 
> 
> -- 
> Antonin Houska
> Cybertec Schonig & Schonig GmbH
> Grohrmuhlgasse 26, A-2700 Wiener Neustadt
> Web: https://www.cybertec-postgresql.com
> 

"WIP: Data at rest encryption" patch and, 2 phase commit.

[Toshi Harada]

Hi.

I applied the patch "WIP: Data at rest encryption" to PostgreSQL 11 - beta 2 
and I'm working on it.

When this patch is applied, the following problem occurs.

* An error occurs when CHECKPOINT is executed during two-phase commit.
* After an error occurs, if you stop PostgreSQL, it will never start again.

(1) First, execute PREPARE TRANSACTION.

postgres=# BEGIN;
BEGIN
postgres=# PREPARE TRANSACTION 'foo';
PREPARE TRANSACTION
postgres=#

(2) Execute the CHECKPOINT command from another terminal.
CHEKPOINT command fails.

postgres=# CHECKPOINT;
ERROR:  checkpoint request failed
HINT:  Consult recent messages in the server log for details.
postgres=#


(3) ROLLBACK PREPARED command also fails.

postgres=# ROLLBACK PREPARED 'foo';
ERROR:  could not read two-phase state from WAL at 0/167EBA0
postgres=# 


(4) Shut down the PostgreSQL server.
During shutdown, a "could not read two-phase state from WAL" error occurs.


2018-07-23 14:49:08.924 JST [15821] LOG:  received fast shutdown request
2018-07-23 14:49:08.925 JST [15821] LOG:  aborting any active transactions
2018-07-23 14:49:08.925 JST [15831] FATAL:  terminating connection due to 
administrator command
2018-07-23 14:49:08.928 JST [15821] LOG:  background worker "logical 
replication launcher" (PID 15829) exited with exit code 1
2018-07-23 14:49:08.928 JST [15824] LOG:  shutting down
2018-07-23 14:49:08.935 JST [15824] FATAL:  could not read two-phase state from 
WAL at 0/167EBA0
2018-07-23 14:49:08.936 JST [15821] LOG:  checkpointer process (PID 15824) 
exited with exit code 1
2018-07-23 14:49:08.936 JST [15821] LOG:  terminating any other active server 
processes
2018-07-23 14:49:08.937 JST [15821] LOG:  abnormal database system shutdown
2018-07-23 14:49:08.945 JST [15821] LOG:  database system is shut down


(5) When restarting the PostgreSQL server, an error(could not read two-phase 
state from WAL) occurs 
and the PostgreSQL server can not be started.

2018-07-23 14:49:42.489 JST [15864] LOG:  listening on IPv6 address "::1", port 
5432
2018-07-23 14:49:42.489 JST [15864] LOG:  listening on IPv4 address 
"127.0.0.1", port 5432
2018-07-23 14:49:42.492 JST [15864] LOG:  listening on Unix socket 
"/tmp/.s.PGSQL.5432"
2018-07-23 14:49:42.521 JST [15866] LOG:  database system shutdown was 
interrupted; last known up at 2018-07-23 14:49:08 JST
2018-07-23 14:49:42.674 JST [15866] LOG:  database system was not properly shut 
down; automatic recovery in progress
2018-07-23 14:49:42.676 JST [15866] LOG:  redo starts at 0/167EB60
2018-07-23 14:49:42.676 JST [15866] LOG:  invalid record length at 0/167EC70: 
wanted 24, got 0
2018-07-23 14:49:42.676 JST [15866] LOG:  redo done at 0/167EC30
2018-07-23 14:49:42.677 JST [15866] FATAL:  could not read two-phase state from 
WAL at 0/167EBA0
2018-07-23 14:49:42.678 JST [15864] LOG:  startup process (PID 15866) exited 
with exit code 1
2018-07-23 14:49:42.678 JST [15864] LOG:  aborting startup due to startup 
process failure
2018-07-23 14:49:42.682 JST [15864] LOG:  database system is shut down

Regards.


Harada Toshi.
NTT TechnoCross Corporation

Antonin Houska  wrote:
> Ants Aasma  wrote:
> 
> > Attached to this mail is a work in progress patch that adds an
> > extensible encryption mechanism. There are some loose ends left to tie
> > up, but the general concept and architecture is at a point where it's
> > ready for some feedback, fresh ideas and bikeshedding.
> 
> Rebased patch is attached here, in case it helps to achieve (some of) the
> goals mentioned in the related thread [1].
> 
> Besides encrypting table and WAL pages, it encrypts the temporary files
> (buffile.c), data stored during logical decoding (reorderbuffer.c) and
> statistics temporary files (pgstat.c). Unlike the previous version, SLRU files
> (e.g. CLOG) are not encrypted (it does not seem critical and the encryption
> makes torn page write quite difficult to handle).
> 
> Another difference is that we use the OpenSSL of the (tweaked) AES XTS cipher
> now.
> 
> Binary upgrade from unencrypted to encrypted cluster is not implemented yet.
> 
> 
> [1] 
> https://www.postgresql.org/message-id/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp
> 
> -- 
> Antonin Houska
> Cybertec Schonig & Schonig GmbH
> Grohrmuhlgasse 26, A-2700 Wiener Neustadt
> Web: https://www.cybertec-postgresql.com
> 

Re: "WIP: Data at rest encryption" patch and, 2 phase commit.

[Antonin Houska]

Toshi Harada  wrote:

> Hi.
> 
> I applied the patch "WIP: Data at rest encryption" to PostgreSQL 11 - beta 2 
> and I'm working on it.
> 
> When this patch is applied, the following problem occurs.
> 
> * An error occurs when CHECKPOINT is executed during two-phase commit.
> * After an error occurs, if you stop PostgreSQL, it will never start again.
> 
> (1) First, execute PREPARE TRANSACTION.
> 
> postgres=# BEGIN;
> BEGIN
> postgres=# PREPARE TRANSACTION 'foo';
> PREPARE TRANSACTION
> postgres=#
> 
> (2) Execute the CHECKPOINT command from another terminal.
> CHEKPOINT command fails.
> 
> postgres=# CHECKPOINT;
> ERROR:  checkpoint request failed
> HINT:  Consult recent messages in the server log for details.
> postgres=#

The patch version I posted in

https://www.postgresql.org/message-id/11678.1532519255%40localhost

fixes an issue (unitialized pointer) that caused failure here, but it was
SEGFAULT rather than ERROR. And the scope of the bug was broader than just
CHECKPOINT.

Can you please test it again with the new version of the patch?

Anyway, thanks for your reports!


-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

Re: [HACKERS] WIP: Data at rest encryption

[Toshi Harada]

Hi.

I am interested in a patch of "WIP: Data at rest encryption".
This patch("data-at-rest-encryption-wip-2018.06.27.patch") is applied to 
PostgreSQL 11-beta 2 and it is running.

In the explanation of this patch, since "data stored during logical decoding" 
is written, 
we tried logical decoding by the test_decoding module, but the following error 
occurs when creating a slot.


pgbench_db=# SELECT * FROM pg_create_logical_replication_slot('my_slot', 
'test_decoding');
ERROR:  invalid magic number B419 in log segment 00020010, 
offset 0
pgbench_db=#


(Also, if you run "CREATE SUNSCRIPTION" for logical replication from another 
server, a similar error will occur.)

Question.
In "data-at-rest-encryption-wip-2018.06.27.patch", is logical decoding still 
not implemented?
Or is there a need for another logical decoding plug-in for "WIP: Data at rest 
encryption"?

Regards.

Antonin Houska  wrote:
> Ants Aasma  wrote:
> 
> > Attached to this mail is a work in progress patch that adds an
> > extensible encryption mechanism. There are some loose ends left to tie
> > up, but the general concept and architecture is at a point where it's
> > ready for some feedback, fresh ideas and bikeshedding.
> 
> Rebased patch is attached here, in case it helps to achieve (some of) the
> goals mentioned in the related thread [1].
> 
> Besides encrypting table and WAL pages, it encrypts the temporary files
> (buffile.c), data stored during logical decoding (reorderbuffer.c) and
> statistics temporary files (pgstat.c). Unlike the previous version, SLRU files
> (e.g. CLOG) are not encrypted (it does not seem critical and the encryption
> makes torn page write quite difficult to handle).
> 
> Another difference is that we use the OpenSSL of the (tweaked) AES XTS cipher
> now.
> 
> Binary upgrade from unencrypted to encrypted cluster is not implemented yet.
> 
> 
> [1] 
> https://www.postgresql.org/message-id/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp
> 
> -- 
> Antonin Houska
> Cybertec Schonig & Schonig GmbH
> Grohrmuhlgasse 26, A-2700 Wiener Neustadt
> Web: https://www.cybertec-postgresql.com
>