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

Thanks of another round of review.

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Apr 24, 2019 at 10:48 AM Antonin Houska <ah(at)cybertec(dot)at> 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.

> + <para>
> + This setting specifies path to executable file that returns
> + either <literal>encryption_key=</literal> for key
> + and <literal>encryption_password=</literal> for password.
> + </para>
>
> 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 <acronym>SSL</acronym> (encrypted)
> - connections. This requires the <productname>OpenSSL</productname>
> - package to be installed. <filename>configure</filename> will check
> - for the required header files and libraries to make sure that
> + Build with support for on-disk data encryption
> + or <acronym>SSL</acronym> (encrypted) connections. This requires
> + the <productname>OpenSSL</productname> package to be
> + installed. <filename>configure</filename> 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 are not identical so the decryption specific
code can't be simply copy & pasted.

So far I'll turn this comment into a TODO comment and mention the refactoring
patch there.

> + {
> + RelFileNode fromNode = {srctablespace, src_dboid, InvalidOid};
> + RelFileNode toNode = {dsttablespace, dboid, InvalidOid};
> +
> + copydir(srcpath, dstpath, &fromNode, &toNode);
> + }
>
> I suggest instead declaring these variables in the surrounding scope
> and initializing them here. This style is unlike most PostgreSQL
> code.

ok

> + case WAIT_EVENT_KDF_FILE_READ:
> + event_name = "KDFFileRead";
> + break;
>
> New wait events need documentation.

ok

> +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.

ok

> 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.

I missed that. The only idea I have now is that a "fake LSN" is set even for
unlogged table if encryption is enabled. Actually that LSN would only be a
counter that gets incremented each time the page contents has changed.

> +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
> bugs. I think we need to either find a way to mask this restriction
> from the users of the buffile facility, or else revise all those users
> so that they respect this new rule and enforce it with an
> unconditional assert.

Sorry, this is misunderstanding. The requirements mentioned in the "Temporary
files" section of the README apply to BufFileWrite() and BufFileRead()
internally. These functions take care of the correct buffer positioning. The
caller does not have to care. I'll try to make this part clearer.

> That brings me to another point, which is that a complex patch like
> this really needs to be divided up into smaller patches for review. I
> see you've started that a little bit (but see comments on commit
> messages above) but you should try to go further with it.

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.

> +To store other kinds of data encrypted than the ones above, developers are
> +advised to use BufFileWriteTransient() and BufFileReadTransient() functions
>
> The previous paragraph is about BufFileWrite() and BufFileRead(), and
> that section is titled "temporary files." Now here you say that for
> other kinds of data (not relations, xlog, or temporary files) we
> should use BufFileWriteTransient(). That seems pretty weird, because
> data that it is not temporary files would presumably be, uh, permanent
> files? So then why is Transient in the name? I imagine there are
> good reasons here but it needs to be made clearer.

"Transient" here means that the file descriptor is kept open for rather short
time, typically until transaction boundary (unlike relation files). However
the file remains on disk even if the descriptor is closed. I believe this is
consistent with the use of OpenTransientFile(). The phrase "other kinds of
data" is probably too generic and more explanation is needed.

> +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
<database>.<schema>.<object name> 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?

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