Re: Key management with tests
Hi Bruce, I went through these patches and executed the test script you added for the KMS section, which looks all good. This is a point that looks like a bug - in patch 10, you changed the location and use of *RelFileNodeSkippingWAL()*, but the modified code logic seems different from the original when encryption is not enabled. After applying this patch, it still will execute the set LSN code flow when RelFileNodeSkippingWAL returns true, and encryption not enabled. On Thu, Apr 1, 2021 at 2:47 PM Bruce Momjian wrote: > On Thu, Mar 11, 2021 at 10:31:28PM -0500, Bruce Momjian wrote: > > I have made significant progress on the cluster file encryption feature > so > > it is time for me to post a new set of patches. > > Here is a rebase, to keep the cfbot green. > > -- > Bruce Momjian https://momjian.us > EDB https://enterprisedb.com > > If only the physical world exists, free will is an illusion. > > -- There is no royal road to learning. HighGo Software Co.
Re: Key management with tests
On Mon, Mar 22, 2021 at 08:38:37PM -0400, Bruce Momjian wrote: > > This particular patch (introducing the RelationIsPermanent() macro) > > seems like it'd be a nice thing to commit independently of the rest, > > reducing the size of this patch set..? > > Committed as suggested. Also, I have written a short presentation on where I think we are with cluster file encryption: https://momjian.us/main/writings/pgsql/cfe.pdf -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Key management with tests
On Thu, Mar 18, 2021 at 11:31:34AM -0400, Stephen Frost wrote: > > src/backend/access/gist/gistutil.c | 2 +- > > src/backend/access/heap/heapam_handler.c | 2 +- > > src/backend/catalog/pg_publication.c | 2 +- > > src/backend/commands/tablecmds.c | 10 +- > > src/backend/optimizer/util/plancat.c | 3 +-- > > src/backend/utils/cache/relcache.c | 2 +- > > src/include/utils/rel.h | 10 -- > > src/include/utils/snapmgr.h | 3 +-- > > 8 files changed, 19 insertions(+), 15 deletions(-) > > This particular patch (introducing the RelationIsPermanent() macro) > seems like it'd be a nice thing to commit independently of the rest, > reducing the size of this patch set..? Committed as suggested. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Key management with tests
On Thu, Mar 18, 2021 at 01:46:28PM -0400, Stephen Frost wrote: > * Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote: > > This caught my attention because a comment says "encryption does not > > support WAL-skipped relations", but there's no direct change to the > > definition of RelFileNodeSkippingWAL() to account for that. Perhaps I > > am just overlooking something, since I'm just skimming anyway. > > This is relatively current activity and so it's entirely possible > comments and perhaps code need further updating in this area, but to > explain what's going on in a bit more detail- > > Ultimately, we need to make sure that LSNs aren't re-used. There's two > sources of LSNs today: those for relations which are being written into > the WAL and those for relations which are not (UNLOGGED relations, > specifically). The 'minimal' WAL level introduces complications with Well, the story is a little more complex than that --- we currently have four LSN uses: 1. real LSNs for WAL-logged relfilenodes 2. real LSNs for GiST indexes for non-WAL-logged relfilenodes of permanenet relations 3. fake LSNs for GiST indexes for relfilenodes of non-permanenet relations 4. zero LSNs for non-GiST non-permanenet relations This patch changes it so #4 gets fake LSNs, and slightly adjusts #2 & #3 so the LSNs are always unique. > I'm not sure if it's been explicitly done yet but I believe the idea is, > based on my last discussion with Bruce, at least initially, simply > disallow encrypted clusters from running with wal_level=minimal to avoid > this issue. I adjusted the hint bit code so it potentially could work with wal_level minimal (just for safety), but the code disallows wal_level minimal, and is documented as such. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Key management with tests
On Thu, Mar 18, 2021 at 02:37:43PM -0300, Álvaro Herrera wrote: > On 2021-Mar-18, Stephen Frost wrote: > > This is discussed in src/backend/access/transam/README, specifically the > > section that talks about Skipping WAL for New RelFileNode. Basically, > > it's the 'wal_level=minimal' optimization which allows WAL to be > > skipped. > > Hmm ... that talks about WAL-skipping *changes*, not WAL-skipping > *relations*. I thought WAL-skipping meant unlogged relations, but > I understand now that that's unrelated. In the transam/README, WAL-skip > means a change in a transaction in a relfilenode that, if rolled back, > would disappear; and I'm not sure I understand how the code is handling > the case that a relation is under that condition. > > This caught my attention because a comment says "encryption does not > support WAL-skipped relations", but there's no direct change to the > definition of RelFileNodeSkippingWAL() to account for that. Perhaps I > am just overlooking something, since I'm just skimming anyway. First, thanks for looking at these patches --- I know it isn't easy. Second, you are right that I equated WAL-skipping relfilenodes with relations, and this was wrong. I have updated the attached patch to use the term WAL-skipping "relfilenodes", and checked the rest of the patches for any incorrect 'skipping' term, but didn't find any. If "WAL-skipping relfilenodes" is not clear enough, we should probably rename RelFileNodeSkippingWAL(). -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion. cfe-10-hint_over_cfe-09-test.diff.gz Description: application/gzip
Re: Key management with tests
Greetings, * Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote: > On 2021-Mar-18, Stephen Frost wrote: > > > * Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote: > > > Patch 10 uses the term "WAL-skip relations". What does that mean? Is > > > it "relations that are not WAL-logged"? I suppose we already have a > > > term for this; I'm not sure it's a good idea to invent a different term > > > that is only used in this new place. > > > > This is discussed in src/backend/access/transam/README, specifically the > > section that talks about Skipping WAL for New RelFileNode. Basically, > > it's the 'wal_level=minimal' optimization which allows WAL to be > > skipped. > > Hmm ... that talks about WAL-skipping *changes*, not WAL-skipping > *relations*. I thought WAL-skipping meant unlogged relations, but > I understand now that that's unrelated. In the transam/README, WAL-skip > means a change in a transaction in a relfilenode that, if rolled back, > would disappear; and I'm not sure I understand how the code is handling > the case that a relation is under that condition. > > This caught my attention because a comment says "encryption does not > support WAL-skipped relations", but there's no direct change to the > definition of RelFileNodeSkippingWAL() to account for that. Perhaps I > am just overlooking something, since I'm just skimming anyway. This is relatively current activity and so it's entirely possible comments and perhaps code need further updating in this area, but to explain what's going on in a bit more detail- Ultimately, we need to make sure that LSNs aren't re-used. There's two sources of LSNs today: those for relations which are being written into the WAL and those for relations which are not (UNLOGGED relations, specifically). The 'minimal' WAL level introduces complications with this requirement because tables created (or truncated) inside a transaction are considered permanent once they're committed, but the data pages in those relations don't go into the WAL and the LSNs on the pages of those relations isn't guaranteed to be either unique or even necessarily set, and if we were to generate LSNs for those it would be required to be done by actually advancing the WAL LSN, which would require writing into the WAL and therefore wouldn't be quite the optimization that's expected. I'm not sure if it's been explicitly done yet but I believe the idea is, based on my last discussion with Bruce, at least initially, simply disallow encrypted clusters from running with wal_level=minimal to avoid this issue. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On 2021-Mar-18, Stephen Frost wrote: > * Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote: > > Patch 10 uses the term "WAL-skip relations". What does that mean? Is > > it "relations that are not WAL-logged"? I suppose we already have a > > term for this; I'm not sure it's a good idea to invent a different term > > that is only used in this new place. > > This is discussed in src/backend/access/transam/README, specifically the > section that talks about Skipping WAL for New RelFileNode. Basically, > it's the 'wal_level=minimal' optimization which allows WAL to be > skipped. Hmm ... that talks about WAL-skipping *changes*, not WAL-skipping *relations*. I thought WAL-skipping meant unlogged relations, but I understand now that that's unrelated. In the transam/README, WAL-skip means a change in a transaction in a relfilenode that, if rolled back, would disappear; and I'm not sure I understand how the code is handling the case that a relation is under that condition. This caught my attention because a comment says "encryption does not support WAL-skipped relations", but there's no direct change to the definition of RelFileNodeSkippingWAL() to account for that. Perhaps I am just overlooking something, since I'm just skimming anyway. -- Álvaro Herrera Valdivia, Chile
Re: Key management with tests
Greetings, * Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote: > Patch 10 uses the term "WAL-skip relations". What does that mean? Is > it "relations that are not WAL-logged"? I suppose we already have a > term for this; I'm not sure it's a good idea to invent a different term > that is only used in this new place. This is discussed in src/backend/access/transam/README, specifically the section that talks about Skipping WAL for New RelFileNode. Basically, it's the 'wal_level=minimal' optimization which allows WAL to be skipped. Thanks! Stephen signature.asc Description: PGP signature
Re: Key management with tests
Patch 10 uses the term "WAL-skip relations". What does that mean? Is it "relations that are not WAL-logged"? I suppose we already have a term for this; I'm not sure it's a good idea to invent a different term that is only used in this new place. -- Álvaro Herrera39°49'30"S 73°17'W
Re: Key management with tests
On Thu, Mar 18, 2021 at 11:31:34AM -0400, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Thu, Mar 11, 2021 at 10:31:28PM -0500, Bruce Momjian wrote: > > > I have made significant progress on the cluster file encryption feature so > > > it is time for me to post a new set of patches. > > > > Here is a rebase, to keep the cfbot green. > > Good stuff. Yes, I was happy I got to a stage where the encryption actually did something useful. > > >From 110358c9ce8764f0c41c12dd37dabde57a92cf1f Mon Sep 17 00:00:00 2001 > > From: Bruce Momjian > > Date: Mon, 15 Mar 2021 10:20:32 -0400 > > Subject: [PATCH] cfe-11-persistent_over_cfe-10-hint squash commit > > > > --- > > src/backend/access/gist/gistutil.c | 2 +- > > src/backend/access/heap/heapam_handler.c | 2 +- > > src/backend/catalog/pg_publication.c | 2 +- > > src/backend/commands/tablecmds.c | 10 +- > > src/backend/optimizer/util/plancat.c | 3 +-- > > src/backend/utils/cache/relcache.c | 2 +- > > src/include/utils/rel.h | 10 -- > > src/include/utils/snapmgr.h | 3 +-- > > 8 files changed, 19 insertions(+), 15 deletions(-) > > This particular patch (introducing the RelationIsPermanent() macro) > seems like it'd be a nice thing to commit independently of the rest, > reducing the size of this patch set..? OK, if no one objects I will apply it in the next few days. The macro is used more in my later patches, which I will not apply now. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Thu, Mar 11, 2021 at 10:31:28PM -0500, Bruce Momjian wrote: > > I have made significant progress on the cluster file encryption feature so > > it is time for me to post a new set of patches. > > Here is a rebase, to keep the cfbot green. Good stuff. > >From 110358c9ce8764f0c41c12dd37dabde57a92cf1f Mon Sep 17 00:00:00 2001 > From: Bruce Momjian > Date: Mon, 15 Mar 2021 10:20:32 -0400 > Subject: [PATCH] cfe-11-persistent_over_cfe-10-hint squash commit > > --- > src/backend/access/gist/gistutil.c | 2 +- > src/backend/access/heap/heapam_handler.c | 2 +- > src/backend/catalog/pg_publication.c | 2 +- > src/backend/commands/tablecmds.c | 10 +- > src/backend/optimizer/util/plancat.c | 3 +-- > src/backend/utils/cache/relcache.c | 2 +- > src/include/utils/rel.h | 10 -- > src/include/utils/snapmgr.h | 3 +-- > 8 files changed, 19 insertions(+), 15 deletions(-) This particular patch (introducing the RelationIsPermanent() macro) seems like it'd be a nice thing to commit independently of the rest, reducing the size of this patch set..? Thanks! Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Fri, Feb 5, 2021 at 07:53:18PM -0500, Bruce Momjian wrote: > On Fri, Feb 5, 2021 at 05:21:22PM -0500, Stephen Frost wrote: > > > I disagree. If we only warn about some parts, attackers will just > > > attack other parts. It will also give users a false sense of security. > > > If you can get the keys, it doesn't matter if there is one or ten ways > > > of getting them, if they are all of equal difficulty. Same with > > > modifying the system files. > > > > I agree that there's an additional concern around the keys and that we > > would want to have a solid way to avoid having them be compromised. We > > might not be able to guarantee that attackers who can write to PGDATA > > can't gain access to the keys in the first implementation, but I don't > > see that as a problem- the TDE capability would still provide protection > > against improper disposal and some other use-cases, which is useful. I > > Agreed. > > > do think it'd be useful to consider how we could provide protection > > against an attacker who has write access from being able to acquire the > > keys, but that seems like a tractable problem. Following that, we could > > look at how to provide integrity checking for principal data, using one > > of the outlined approaches or maybe something else entirely. Lastly, > > perhaps we can find a way to provide confidentiality and integrity for > > other parts of the system. > > Yes, we should consider it, and I want to have this discussion. Ideally > we could implement that now, because it might be harder later. However, > I don't see how we can add additional security protections without > adding a lot more complexity. You are right we might have better ideas > later. I added a Limitations section so we can consider future improvements: https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Limitations -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Fri, Feb 5, 2021 at 05:21:22PM -0500, Stephen Frost wrote: > > I disagree. If we only warn about some parts, attackers will just > > attack other parts. It will also give users a false sense of security. > > If you can get the keys, it doesn't matter if there is one or ten ways > > of getting them, if they are all of equal difficulty. Same with > > modifying the system files. > > I agree that there's an additional concern around the keys and that we > would want to have a solid way to avoid having them be compromised. We > might not be able to guarantee that attackers who can write to PGDATA > can't gain access to the keys in the first implementation, but I don't > see that as a problem- the TDE capability would still provide protection > against improper disposal and some other use-cases, which is useful. I Agreed. > do think it'd be useful to consider how we could provide protection > against an attacker who has write access from being able to acquire the > keys, but that seems like a tractable problem. Following that, we could > look at how to provide integrity checking for principal data, using one > of the outlined approaches or maybe something else entirely. Lastly, > perhaps we can find a way to provide confidentiality and integrity for > other parts of the system. Yes, we should consider it, and I want to have this discussion. Ideally we could implement that now, because it might be harder later. However, I don't see how we can add additional security protections without adding a lot more complexity. You are right we might have better ideas later. > Each of these steps is a useful improvement in its own right and will > open up more opportunities for PG to be used. It wasn't my intent to > suggest otherwise, but rather to see if there was an opportunity to get > a few things done at once if it wasn't too impactful. I agree now that > it makes sense to focus on the first step, so we can hopefully get that > accomplished. OK, good. > > I think postmaster.opts is used for pg_ctl reload. I think the question > > is whether the value of maliciously writable PGDATA being able to read > > the keys, while not protecting or detecting all malicious > > writes/db-modifications, is worth it. And, while I listed the files > > above, there are probably many more ways to break the system. > > postmaster.opts is used for pg_ctl restart, just to be clear. Yes, sorry, "restart". > As I try to state above- I don't think we need to provide any specific > protections against a malicious writer for plain encryption to be > useful for some important use-cases. Providing protections against a > malicious writer being able to access the keys is certainly important > as, if they acquire the keys, they would be able to trivially both > decrypt the data and modify any other data they wished to, so it seems > likely that solving that would be the first step towards protecting > against a malicious writer, after which it's useful to think about what > else we could provide integrity checking of, and principal data strikes > me as the next sensible step, followed by what's essentially metadata. Agreed. > > See above --- I think we can't just say we close _most_ of the doors > > here, and I am afraid there will be more and more cases we miss. It > > feels too open-ended. For example, imagine modifying a PGDATA file so > > it is a symbolic link to another file that is not in PGDATA? Seems that > > would break all sorts of security restrictions, and that's just a new > > idea I came up with today. > > It's not clear how that would provide the attacker with much, if > anything. Not sure myself either. > > What I don't want to do is to add a lot of complexity to the system, and > > not really gain any meaningful security. > > Integrity is very meaningful to security, but key management would > certainly come first because if an attacker is able to acquire the keys > then they can circumvent any integrity check being done by simply using > the key. I appreciate that protecting the keys is non-trivial but it's > absolutely critical as everything else falls apart if the key is > compromised. I don't think we should be thinking that we're going to be Agreed, > done with key management or with providing ways to acquire keys even if > the currently proposed patches go in- we'll undoubtably need to provide > other options in the future. There's an interesting point in this > regarding how the flexibility of the shell-script based approach also > introduces this risk that an attacker could modify it and write the key > out to somewhere that they could get at pretty easily. Having support > for directly fetching the key from the Linux kernel or the various > vaulting systems would avoid this risk, I would think. Maybe there's a Agreed. > way to get PG to dump the key out of system memory by modifying other > files in PGDATA but that's surely quite a bit more difficult. > Ultimately, I don't
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Fri, Feb 5, 2021 at 01:14:35PM -0500, Stephen Frost wrote: > > > I looked further. First, I don't think we are going to be able to > > > protect at all against users who have _write_ access on the OS running > > > Postgres. It would be too easy to just read process memory, or modify > > > ~/.profile. > > > > I don't think anyone is really expecting that we'll be able to come up > > with a way to protect against attackers who have fully compromised the > > OS to the point where they can read/write OS memory, or even the PG unix > > account. I'm certainly not suggesting that there is a way to do that or > > that it's an attack vector we are trying to address here. > > OK, that's good. > > > > I think the only possible option would be to try to give some protection > > > against users with write access to PGDATA, where PGDATA is on another > > > server, e.g., via NFS. We can't protect against all db modifications, > > > for reasons outlined above, but we might be able to protect against > > > write users being able to _read_ the keys and therefore decrypt data. > > > > That certainly seems like a worthy goal. I also really want to stress > > that I don't think anyone is expecting us to be able to "protect" > > against users who have write access to the system- write access to files > > is really an OS level issue and there's not much we can do once someone > > has found a way to circumvent that (we can try to help the OS by doing > > things like using SELinux, of course, but that's a different > > discussion). At the point that an attacker has gotten write access, the > > Agreed. > > > best we can do is complain loudly if we detect unexpected modifications. > > Ideally, we would be able to do that for everything, but certainly doing > > it for the principal data would go a long way and is far better than > > nothing. > > I disagree. If we only warn about some parts, attackers will just > attack other parts. It will also give users a false sense of security. > If you can get the keys, it doesn't matter if there is one or ten ways > of getting them, if they are all of equal difficulty. Same with > modifying the system files. I agree that there's an additional concern around the keys and that we would want to have a solid way to avoid having them be compromised. We might not be able to guarantee that attackers who can write to PGDATA can't gain access to the keys in the first implementation, but I don't see that as a problem- the TDE capability would still provide protection against improper disposal and some other use-cases, which is useful. I do think it'd be useful to consider how we could provide protection against an attacker who has write access from being able to acquire the keys, but that seems like a tractable problem. Following that, we could look at how to provide integrity checking for principal data, using one of the outlined approaches or maybe something else entirely. Lastly, perhaps we can find a way to provide confidentiality and integrity for other parts of the system. Each of these steps is a useful improvement in its own right and will open up more opportunities for PG to be used. It wasn't my intent to suggest otherwise, but rather to see if there was an opportunity to get a few things done at once if it wasn't too impactful. I agree now that it makes sense to focus on the first step, so we can hopefully get that accomplished. > > There are certainly already users out there who intentionally make > > postgresql.auto.conf owned by root/root, zero-sized, and monitor it to > > make sure that it isn't updated. postgresql.conf actually is also often > > monitored for changes by a change management system of some kind and may > > also be owned by root/root already. I suspect that postmaster.opts is > > not monitored as closely, but that's probably due more to the fact that > > we don't really document it as a configuration system file and it can't > > be put outside of PGDATA. Having a way to move it outside of PGDATA or > > just not have it be used at all (do we really need it..?) would be > > another way to address that risk though. > > I think postmaster.opts is used for pg_ctl reload. I think the question > is whether the value of maliciously writable PGDATA being able to read > the keys, while not protecting or detecting all malicious > writes/db-modifications, is worth it. And, while I listed the files > above, there are probably many more ways to break the system. postmaster.opts is used for pg_ctl restart, just to be clear. As I try to state above- I don't think we need to provide any specific protections against a malicious writer for plain encryption to be useful for some important use-cases. Providing protections against a malicious writer being able to access the keys is certainly important as, if they acquire the keys, they would be able to trivially both decrypt the data and modify any
Re: Key management with tests
On Fri, Feb 5, 2021 at 01:14:35PM -0500, Stephen Frost wrote: > > I looked further. First, I don't think we are going to be able to > > protect at all against users who have _write_ access on the OS running > > Postgres. It would be too easy to just read process memory, or modify > > ~/.profile. > > I don't think anyone is really expecting that we'll be able to come up > with a way to protect against attackers who have fully compromised the > OS to the point where they can read/write OS memory, or even the PG unix > account. I'm certainly not suggesting that there is a way to do that or > that it's an attack vector we are trying to address here. OK, that's good. > > I think the only possible option would be to try to give some protection > > against users with write access to PGDATA, where PGDATA is on another > > server, e.g., via NFS. We can't protect against all db modifications, > > for reasons outlined above, but we might be able to protect against > > write users being able to _read_ the keys and therefore decrypt data. > > That certainly seems like a worthy goal. I also really want to stress > that I don't think anyone is expecting us to be able to "protect" > against users who have write access to the system- write access to files > is really an OS level issue and there's not much we can do once someone > has found a way to circumvent that (we can try to help the OS by doing > things like using SELinux, of course, but that's a different > discussion). At the point that an attacker has gotten write access, the Agreed. > best we can do is complain loudly if we detect unexpected modifications. > Ideally, we would be able to do that for everything, but certainly doing > it for the principal data would go a long way and is far better than > nothing. I disagree. If we only warn about some parts, attackers will just attack other parts. It will also give users a false sense of security. If you can get the keys, it doesn't matter if there is one or ten ways of getting them, if they are all of equal difficulty. Same with modifying the system files. > Now, that said, I don't know that we absolutely must have that in the > first release of TDE support for PG. In thinking about this, I would > say we have two basic options: I skipped this part since I think we need a fully secure plan before considering page format changes. We don't need it for our currently outlined feature-set. > > Looking at PGDATA, we have, at least: > > > > postgresql.conf > > pg_hba.conf > > postmaster.opts > > postgresql.conf.auto > > > > which could be exploited to cause reading of the cluster key or process > > memory. The first two can be located outside of PGDATA but the last two > > currently cannot. > > There are certainly already users out there who intentionally make > postgresql.auto.conf owned by root/root, zero-sized, and monitor it to > make sure that it isn't updated. postgresql.conf actually is also often > monitored for changes by a change management system of some kind and may > also be owned by root/root already. I suspect that postmaster.opts is > not monitored as closely, but that's probably due more to the fact that > we don't really document it as a configuration system file and it can't > be put outside of PGDATA. Having a way to move it outside of PGDATA or > just not have it be used at all (do we really need it..?) would be > another way to address that risk though. I think postmaster.opts is used for pg_ctl reload. I think the question is whether the value of maliciously writable PGDATA being able to read the keys, while not protecting or detecting all malicious writes/db-modifications, is worth it. And, while I listed the files above, there are probably many more ways to break the system. > > The problem is that this is a limited use-case, and there are probably > > other problems I am not considering. It seems too error-prone to even > > try protect against this, but it does limit the value of this feature. > > I don't think we need to consider it a failing of the capability every > time we think of something else that really should be addressed when > considering this attack vector. We aren't going to be releasing this > and saying "we guarantee that this protects against an attacker who has > write access to PGDATA". Instead, we would be documenting "XYZ, when > enabled, is used to validate the integrity of ABC data. Individuals > concerned with unexpected modifications to their system should consider > independently monitoring files D, E, F. Note that there is currently no > explicit protection against or detection of unexpected or malicious > modification of other parts of the system such as the transaction > record.", or something along those lines. Hardening guidelines would > also recommend things like having postgresql.conf moved out of PGDATA > and owned by root/root, etc. Users would then have the ability to > evaluate if what we're providing
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Wed, Feb 3, 2021 at 01:16:32PM -0500, Bruce Momjian wrote: > > On Wed, Feb 3, 2021 at 10:33:57AM -0500, Stephen Frost wrote: > > > I doubt anyone would actually stipulate that they *guarantee* detection > > > of malicious writes, and I don't think we should either, but certainly > > > the other systems which provide TDE do so in a manner that provides both > > > confidentiality and integrity. The big O, at least, documents that they > > > use SHA-1 for their integrity checking, though they also provide an > > > option which disables it. If we used an additional fork to provide the > > > integrity then we could also give users the option of either having > > > integrity included or not. > > > > I thought more about this at an abstract level. If you are worried > > about malicious users _reading_ data, you can encrypt the sensitive > > parts, e.g., heap/index/WAL/temp, and leave some unencrypted, like > > pg_xact. Reading pg_xact is pretty useless if you can't read the heap > > pages. Reading postgresql.conf.auto, the external key retrieval > > scripts, etc. are useless too. > > > > However, when you are trying to protect against write access, you have > > to really encrypt _everything_, because the system is very > > interdependent, and changing one part where _reading_ is safe can affect > > other parts that must remain secure. You can modify > > postgresql.conf.auto to capture the cluster key, or maybe even change > > something to dump out the data keys from memory. You can modify pg_xact > > to affect how heap pages are interpreted. > > > > My point is that being able to detect malicious heap/index writes really > > doesn't gain us any security since there are much more serious writes > > that can be made, and protecting against those more serious writes would > > cause unacceptable Postgres source code changes which will probably > > never be implemented. > > I looked further. First, I don't think we are going to be able to > protect at all against users who have _write_ access on the OS running > Postgres. It would be too easy to just read process memory, or modify > ~/.profile. I don't think anyone is really expecting that we'll be able to come up with a way to protect against attackers who have fully compromised the OS to the point where they can read/write OS memory, or even the PG unix account. I'm certainly not suggesting that there is a way to do that or that it's an attack vector we are trying to address here. > I think the only possible option would be to try to give some protection > against users with write access to PGDATA, where PGDATA is on another > server, e.g., via NFS. We can't protect against all db modifications, > for reasons outlined above, but we might be able to protect against > write users being able to _read_ the keys and therefore decrypt data. That certainly seems like a worthy goal. I also really want to stress that I don't think anyone is expecting us to be able to "protect" against users who have write access to the system- write access to files is really an OS level issue and there's not much we can do once someone has found a way to circumvent that (we can try to help the OS by doing things like using SELinux, of course, but that's a different discussion). At the point that an attacker has gotten write access, the best we can do is complain loudly if we detect unexpected modifications. Ideally, we would be able to do that for everything, but certainly doing it for the principal data would go a long way and is far better than nothing. Now, that said, I don't know that we absolutely must have that in the first release of TDE support for PG. In thinking about this, I would say we have two basic options: - Keep the same page layout, requiring that integrity data must be stored elsewhere, eg: another fork - Use a different page layout when TDE is enabled, making room for integrity information to be included on each page There's a set of pros and cons for these: Same page layout pros: - Simpler and less impactful on the overall system - With integrity data stored elsewhere, could possibly be something that's optional to enable/disable on a per-table basis - Potential to do things like have an unencrypted primary and an encrypted replica, providing an easier migration path Same page layout cons: - Integrity information must be stored elsewhere - Increases the reads/memory that is needed, since we have to look up the integrity information on every read. - Increases the writes that have to be done since we'd be dirtying multiple pages instead of just the main fork (though this isn't exactly unusual- there's the vis map, and indexes, etc, but it'd be yet another thing we're updating) Different page layout pros: - Avoids extra reads/writes for the integrity information - Once done, this might provide us with a way to add other page level information in the future while
Re: Key management with tests
On Wed, Feb 3, 2021 at 01:16:32PM -0500, Bruce Momjian wrote: > On Wed, Feb 3, 2021 at 10:33:57AM -0500, Stephen Frost wrote: > > I doubt anyone would actually stipulate that they *guarantee* detection > > of malicious writes, and I don't think we should either, but certainly > > the other systems which provide TDE do so in a manner that provides both > > confidentiality and integrity. The big O, at least, documents that they > > use SHA-1 for their integrity checking, though they also provide an > > option which disables it. If we used an additional fork to provide the > > integrity then we could also give users the option of either having > > integrity included or not. > > I thought more about this at an abstract level. If you are worried > about malicious users _reading_ data, you can encrypt the sensitive > parts, e.g., heap/index/WAL/temp, and leave some unencrypted, like > pg_xact. Reading pg_xact is pretty useless if you can't read the heap > pages. Reading postgresql.conf.auto, the external key retrieval > scripts, etc. are useless too. > > However, when you are trying to protect against write access, you have > to really encrypt _everything_, because the system is very > interdependent, and changing one part where _reading_ is safe can affect > other parts that must remain secure. You can modify > postgresql.conf.auto to capture the cluster key, or maybe even change > something to dump out the data keys from memory. You can modify pg_xact > to affect how heap pages are interpreted. > > My point is that being able to detect malicious heap/index writes really > doesn't gain us any security since there are much more serious writes > that can be made, and protecting against those more serious writes would > cause unacceptable Postgres source code changes which will probably > never be implemented. I looked further. First, I don't think we are going to be able to protect at all against users who have _write_ access on the OS running Postgres. It would be too easy to just read process memory, or modify ~/.profile. I think the only possible option would be to try to give some protection against users with write access to PGDATA, where PGDATA is on another server, e.g., via NFS. We can't protect against all db modifications, for reasons outlined above, but we might be able to protect against write users being able to _read_ the keys and therefore decrypt data. Looking at PGDATA, we have, at least: postgresql.conf pg_hba.conf postmaster.opts postgresql.conf.auto which could be exploited to cause reading of the cluster key or process memory. The first two can be located outside of PGDATA but the last two currently cannot. The problem is that this is a limited use-case, and there are probably other problems I am not considering. It seems too error-prone to even try protect against this, but it does limit the value of this feature. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Wed, Feb 3, 2021 at 10:33:57AM -0500, Stephen Frost wrote: > > I am thinking group read-access might be a requirement for cluster file > > encryption to be effective. > > People certainly do use group read-access, but I don't see that as being > a requirement for cluster file encryption to be effective, it's just one > thing TDE can address, among others, as discussed. Agreed. > > This also does not protect against users who have read access to > > database process memory all in-memory data pages and data > > encryption keys are stored unencrypted in memory, so an attacker who > > --> is able to read memory can decrypt the entire cluster. The Postgres > > --> operating system user and the operating system administrator, e.g., > > --> the root user, have such access. > > That's helpful, +1. Good. > > > Uh, well, they could modify postgresql.conf to change the script to save > > > the secret returned by the script before returning it to the PG server. > > > We could require postgresql.conf to be somewhere secure, but then how do > > > we know that is secure? I just don't see a clean solution here, but the > > > idea that you write and then wait for the key to show up seems like a > > > very valid way of attack, and it took me a while to be able to > > > articulate it. > > postgresql.conf isn't always writable by the postgres user, though > postgresql.auto.conf is likely to always be. I'm not sure how much of a > concern that is, but it we wanted to take steps to explicitly address > this issue, we could have some kind of 'secure' postgresql.conf file > which we would encourage users to make owned by root and whose values > wouldn't be allowed to be overridden once set. Well, I think there is a lot more than postgresql.conf to worry about --- see below. > > Let's suppose you lock down your cluster --- the non-PGDATA files are > > owned by root, postgresql.conf and pg_hba.conf are moved out of PGDATA > > and are not writable by the database OS user, or we have the PGDATA > > directory on another server, so the adversary can only write to the > > remote PGDATA directory. > > > > What can they do? Well, they can't modify pg_proc to add a shared > > library since pg_proc is encrypted, so we have to focus on files needed > > before encryption starts or files that can't be easily encrypted. > > This isn't accurate- just because it's encrypted doesn't mean they can't > modify it. That's exactly why integrity is important, because an > attacker absolutely could modify the files directly and potentially > exploit the system through those modifications. They can't easily modify it to inject a shared object referenced into a system column, was my point --- also see below. > > They could create postgresql.conf.auto in PGDATA, and modify > > cluster_key_command to capture the key, or they could modify preload > > libraries or archive command to call a command to read memory as the PG > > OS user and write the key out somewhere, or use the key to rewrite the > > database files --- those wouldn't even need a database restart, just a > > reload. > > They would need to actually be able to effect that reload though. This > is where the question comes up as to just what attack vector we're > trying to address. It's certainly possible that an attacker has only > access to the stored data in an off-line fashion (eg: a hard drive that > was mistakenly thrown away without being properly wiped) and that's one > of the cases which is addressed by cluster encryption. An attacker > might have access to the LUN that PG is running on but not to the > running server itself, which it seems like is what you're contemplating > here. That's a much harder attack vector to fully protect against and > we might need to do more than we're currently contemplating to address > it- but I don't think we necessarily must solve for all cases in the > first pass at this. See below. > > They could also modify pg_xact files so that, even though the heap/index > > files are encrypted, how the contents of those files are interpreted > > would change. > > Yes, ideally, we'd encrypt/integrity check just about every part of the > running system and that's one area the patch doesn't address- things > like temporary files and other parts. It is worse than that --- see below. > > In summary, to detect malicious user writes, you would need to protect > > the files used before encryption starts (root owned or owned by another > > user?), and encrypt all files after encryption starts --- any other > > approach would probably leave open attack vectors, and I don't think > > there is sufficient community desire to add such boundaries. > > There's going to be some attack vectors that TDE doesn't address. We > should identify and document those where we're able to. We could offer > up some mitigations (eg: strongly suggest monitoring of key utilization > such that if the KEK is used without a reboot of the system or
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Mon, Feb 1, 2021 at 07:47:57PM -0500, Bruce Momjian wrote: > > On Mon, Feb 1, 2021 at 06:31:32PM -0500, Stephen Frost wrote: > > > * Bruce Momjian (br...@momjian.us) wrote: > > > > The purpose of cluster file encryption is to prevent users with read > > > > access to the directories used to store database files and write-ahead > > > > log files from being able to access the data stored in those files. > > > > For example, when using cluster file encryption, users who have read > > > > access to the cluster directories for backup purposes will not be able > > > > to decrypt the data stored in these files. It also protects against > > > > decrypted data access after media theft. > > > > > > That's one valid use-case and it particularly makes sense to consider, > > > now that we support group read-access to the data cluster. The last > > > > Do enough people use group read-access to be useful? > > I am thinking group read-access might be a requirement for cluster file > encryption to be effective. People certainly do use group read-access, but I don't see that as being a requirement for cluster file encryption to be effective, it's just one thing TDE can address, among others, as discussed. > > > line seems a bit unclear- I would update it to say: > > > Cluster file encryption also provides data-at-rest security, protecting > > > users from data loss should the physical media on which the cluster is > > > stored be stolen, improperly deprovisioned (not wiped or destroyed), or > > > otherwise ends up in the hands of an attacker. > > > > I have split the section into three paragraphs, trimmed down some of the > > suggested text, and added it. Full version below. > > Here is an updated doc description of memory reading: > > This also does not protect against users who have read access to > database process memory all in-memory data pages and data > encryption keys are stored unencrypted in memory, so an attacker who > --> is able to read memory can decrypt the entire cluster. The Postgres > --> operating system user and the operating system administrator, e.g., > --> the root user, have such access. That's helpful, +1. > > > > File system write access can allow for unauthorized file system data > > > > decryption if the writes can be used to weaken the system's security > > > > and this weakened system is later supplied with externally-stored > > > > keys. > > > > > > This isn't very clear as to exactly what the concern is or how an > > > attacker would be able to thwart the system if they had write access to > > > it. An attacker with write access could possibly attempt to replace the > > > existing keys, but with the key wrapping that we're using, that should > > > result in just a decryption failure (unless, of course, the attacker has > > > the actual KEK that was used, but that's not terribly interesting to > > > worry about since then they could just go access the files directly). > > > > Uh, well, they could modify postgresql.conf to change the script to save > > the secret returned by the script before returning it to the PG server. > > We could require postgresql.conf to be somewhere secure, but then how do > > we know that is secure? I just don't see a clean solution here, but the > > idea that you write and then wait for the key to show up seems like a > > very valid way of attack, and it took me a while to be able to > > articulate it. postgresql.conf isn't always writable by the postgres user, though postgresql.auto.conf is likely to always be. I'm not sure how much of a concern that is, but it we wanted to take steps to explicitly address this issue, we could have some kind of 'secure' postgresql.conf file which we would encourage users to make owned by root and whose values wouldn't be allowed to be overridden once set. > Let's suppose you lock down your cluster --- the non-PGDATA files are > owned by root, postgresql.conf and pg_hba.conf are moved out of PGDATA > and are not writable by the database OS user, or we have the PGDATA > directory on another server, so the adversary can only write to the > remote PGDATA directory. > > What can they do? Well, they can't modify pg_proc to add a shared > library since pg_proc is encrypted, so we have to focus on files needed > before encryption starts or files that can't be easily encrypted. This isn't accurate- just because it's encrypted doesn't mean they can't modify it. That's exactly why integrity is important, because an attacker absolutely could modify the files directly and potentially exploit the system through those modifications. > They could create postgresql.conf.auto in PGDATA, and modify > cluster_key_command to capture the key, or they could modify preload > libraries or archive command to call a command to read memory as the PG > OS user and write the key out somewhere, or use the key to rewrite the >
Re: Key management with tests
On Mon, Feb 1, 2021 at 07:47:57PM -0500, Bruce Momjian wrote: > On Mon, Feb 1, 2021 at 06:31:32PM -0500, Stephen Frost wrote: > > * Bruce Momjian (br...@momjian.us) wrote: > > > The purpose of cluster file encryption is to prevent users with read > > > access to the directories used to store database files and write-ahead > > > log files from being able to access the data stored in those files. > > > For example, when using cluster file encryption, users who have read > > > access to the cluster directories for backup purposes will not be able > > > to decrypt the data stored in these files. It also protects against > > > decrypted data access after media theft. > > > > That's one valid use-case and it particularly makes sense to consider, > > now that we support group read-access to the data cluster. The last > > Do enough people use group read-access to be useful? I am thinking group read-access might be a requirement for cluster file encryption to be effective. > > line seems a bit unclear- I would update it to say: > > Cluster file encryption also provides data-at-rest security, protecting > > users from data loss should the physical media on which the cluster is > > stored be stolen, improperly deprovisioned (not wiped or destroyed), or > > otherwise ends up in the hands of an attacker. > > I have split the section into three paragraphs, trimmed down some of the > suggested text, and added it. Full version below. Here is an updated doc description of memory reading: This also does not protect against users who have read access to database process memory all in-memory data pages and data encryption keys are stored unencrypted in memory, so an attacker who --> is able to read memory can decrypt the entire cluster. The Postgres --> operating system user and the operating system administrator, e.g., --> the root user, have such access. > > > File system write access can allow for unauthorized file system data > > > decryption if the writes can be used to weaken the system's security > > > and this weakened system is later supplied with externally-stored keys. > > > > This isn't very clear as to exactly what the concern is or how an > > attacker would be able to thwart the system if they had write access to > > it. An attacker with write access could possibly attempt to replace the > > existing keys, but with the key wrapping that we're using, that should > > result in just a decryption failure (unless, of course, the attacker has > > the actual KEK that was used, but that's not terribly interesting to > > worry about since then they could just go access the files directly). > > Uh, well, they could modify postgresql.conf to change the script to save > the secret returned by the script before returning it to the PG server. > We could require postgresql.conf to be somewhere secure, but then how do > we know that is secure? I just don't see a clean solution here, but the > idea that you write and then wait for the key to show up seems like a > very valid way of attack, and it took me a while to be able to > articulate it. Let's suppose you lock down your cluster --- the non-PGDATA files are owned by root, postgresql.conf and pg_hba.conf are moved out of PGDATA and are not writable by the database OS user, or we have the PGDATA directory on another server, so the adversary can only write to the remote PGDATA directory. What can they do? Well, they can't modify pg_proc to add a shared library since pg_proc is encrypted, so we have to focus on files needed before encryption starts or files that can't be easily encrypted. They could create postgresql.conf.auto in PGDATA, and modify cluster_key_command to capture the key, or they could modify preload libraries or archive command to call a command to read memory as the PG OS user and write the key out somewhere, or use the key to rewrite the database files --- those wouldn't even need a database restart, just a reload. They could also modify pg_xact files so that, even though the heap/index files are encrypted, how the contents of those files are interpreted would change. In summary, to detect malicious user writes, you would need to protect the files used before encryption starts (root owned or owned by another user?), and encrypt all files after encryption starts --- any other approach would probably leave open attack vectors, and I don't think there is sufficient community desire to add such boundaries. How do other database systems guarantee to detect malicious writes? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Mon, Feb 1, 2021 at 06:31:32PM -0500, Stephen Frost wrote: > * Bruce Momjian (br...@momjian.us) wrote: > > The purpose of cluster file encryption is to prevent users with read > > access to the directories used to store database files and write-ahead > > log files from being able to access the data stored in those files. > > For example, when using cluster file encryption, users who have read > > access to the cluster directories for backup purposes will not be able > > to decrypt the data stored in these files. It also protects against > > decrypted data access after media theft. > > That's one valid use-case and it particularly makes sense to consider, > now that we support group read-access to the data cluster. The last Do enough people use group read-access to be useful? > line seems a bit unclear- I would update it to say: > Cluster file encryption also provides data-at-rest security, protecting > users from data loss should the physical media on which the cluster is > stored be stolen, improperly deprovisioned (not wiped or destroyed), or > otherwise ends up in the hands of an attacker. I have split the section into three paragraphs, trimmed down some of the suggested text, and added it. Full version below. > > File system write access can allow for unauthorized file system data > > decryption if the writes can be used to weaken the system's security > > and this weakened system is later supplied with externally-stored keys. > > This isn't very clear as to exactly what the concern is or how an > attacker would be able to thwart the system if they had write access to > it. An attacker with write access could possibly attempt to replace the > existing keys, but with the key wrapping that we're using, that should > result in just a decryption failure (unless, of course, the attacker has > the actual KEK that was used, but that's not terribly interesting to > worry about since then they could just go access the files directly). Uh, well, they could modify postgresql.conf to change the script to save the secret returned by the script before returning it to the PG server. We could require postgresql.conf to be somewhere secure, but then how do we know that is secure? I just don't see a clean solution here, but the idea that you write and then wait for the key to show up seems like a very valid way of attack, and it took me a while to be able to articulate it. > Until and unless we solve the issue around storing the GCM tags for each > page, we will have the risk that an attacker could modify a page in a > manner that we wouldn't detect. This is the biggest concern that I have > currently with the existing TDE patch sets. Well, GCM certainly can detect page modification, but it can't detect removing pages from the end of the table, or, since the nonce is LSN/pageno, you could copy a page from another table that has the same offset into another table, particularly with partitioning where the tables have the same columns. We might be able to protect against the later with some kind of table-id in the nonce, but I don't see how table truncation can be detected without adding a whole lot of overhead and complexity. And if we can't protect against those two, why bother with detecting single-page modifications? We have to do a full job for it to be useful. > There's two options that I see around how to address that issue- either > we arrange to create space in the page for the tag, such as by making > the 'special' space on a page a bit bigger and making sure that > everything understands that, or we'll need to add another fork in which > we store the tags (and possibly other TDE/encryption related > information). If we go with a fork then it should be possible to do WAL > streaming from an unencrypted cluster to an encrypted one, which would > be pretty neat, but it means another fork and another page that has to > be read/written every time we modify a page. Getting some input into > the trade-offs here would be really helpful. I don't think it's really > reasonable to go out with TDE without having figured out the integrity > side. Certainly, when I review things like NIST 800-53, it's very clear > that the requirement is for both confidentiality *and* integrity. Wow, well, if they are both required, and we can't do both, is it valuable to do just one? Yes, we can do something later, but what if we have no idea how to implement the second part? Your fork idea above might need to store some table-id used for the nonce (to prevent copying from another table) and the number of pages in the table, which fixes the integrity check issue, but adds a lot of complexity and perhaps overhead. > > This also does not protect from users who have read access to system > > memory. This also does not detect or protect against users with write > > access from removing or modifying database files. > > The last seems a bit obvious, but the first sentence quoted above is
Re: Key management with tests
On Mon, Feb 1, 2021 at 06:34:53PM -0500, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Sat, Jan 30, 2021 at 08:23:11AM -0500, Tom Kincaid wrote: > > > I propose that we meet to discuss what approach we want to use to move TDE > > > forward. We then start a new thread with a proposal on the approach > > > and finalize it via community consensus. I will invite Bruce, Stephen and > > > Masahiko to this meeting. If anybody else would like to participate in > > > this > > > discussion and subsequently in the effort to get TDE in PG1x, please let > > > me > > > know. Assuming Bruce, Stephen and Masahiko are down for this, I (or a > > > volunteer > > > from this meeting) will post the proposal for how we move this patch > > > forward in > > > another thread. Hopefully, we can get consensus on that and subsequently > > > restart the execution of delivering this feature. > > > > We got complaints that decisions were not publicly discussed, or were > > too long, so I am not sure this helps. > > If the notes are published afterwords as an explanation of why certain > choices were made, I suspect it'd be reasonably well received. The > concern about back-room discussions is more that decisions are made > without explanation as to why, provided we avoid that, I believe they > can be helpful. Well, I thought that was what the wiki was, but I guess not. I did remove some of the decision logic recently since we had made a final decision. However, most of the questions were not covered on the wiki, since, as I said, everyone comes with a different need for details. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Sat, Jan 30, 2021 at 08:23:11AM -0500, Tom Kincaid wrote: > > I propose that we meet to discuss what approach we want to use to move TDE > > forward. We then start a new thread with a proposal on the approach > > and finalize it via community consensus. I will invite Bruce, Stephen and > > Masahiko to this meeting. If anybody else would like to participate in this > > discussion and subsequently in the effort to get TDE in PG1x, please let me > > know. Assuming Bruce, Stephen and Masahiko are down for this, I (or a > > volunteer > > from this meeting) will post the proposal for how we move this patch > > forward in > > another thread. Hopefully, we can get consensus on that and subsequently > > restart the execution of delivering this feature. > > We got complaints that decisions were not publicly discussed, or were > too long, so I am not sure this helps. If the notes are published afterwords as an explanation of why certain choices were made, I suspect it'd be reasonably well received. The concern about back-room discussions is more that decisions are made without explanation as to why, provided we avoid that, I believe they can be helpful. So, +1 for my part to have the conversation. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Fri, Jan 29, 2021 at 05:40:37PM -0500, Stephen Frost wrote: > > I hope it's pretty clear that I'm also very much in support of both this > > effort with the KMS and of TDE in general- TDE is specifically, > > Yes, thanks. I know we have privately talked about this recently, but > it is nice to have it in public like this. Certainly happy to lend my support and to spend some time working on this to move it forward. > > repeatedly, called out as a capability whose lack is blocking PG from > > being able to be used for certain use-cases that it would otherwise be > > well suited for, and that's really unfortunate. > > So, below, I am going to copy two doc paragraphs from the patch: > > The purpose of cluster file encryption is to prevent users with read > access to the directories used to store database files and write-ahead > log files from being able to access the data stored in those files. > For example, when using cluster file encryption, users who have read > access to the cluster directories for backup purposes will not be able > to decrypt the data stored in these files. It also protects against > decrypted data access after media theft. That's one valid use-case and it particularly makes sense to consider, now that we support group read-access to the data cluster. The last line seems a bit unclear- I would update it to say: Cluster file encryption also provides data-at-rest security, protecting users from data loss should the physical media on which the cluster is stored be stolen, improperly deprovisioned (not wiped or destroyed), or otherwise ends up in the hands of an attacker. > File system write access can allow for unauthorized file system data > decryption if the writes can be used to weaken the system's security > and this weakened system is later supplied with externally-stored keys. This isn't very clear as to exactly what the concern is or how an attacker would be able to thwart the system if they had write access to it. An attacker with write access could possibly attempt to replace the existing keys, but with the key wrapping that we're using, that should result in just a decryption failure (unless, of course, the attacker has the actual KEK that was used, but that's not terribly interesting to worry about since then they could just go access the files directly). Until and unless we solve the issue around storing the GCM tags for each page, we will have the risk that an attacker could modify a page in a manner that we wouldn't detect. This is the biggest concern that I have currently with the existing TDE patch sets. There's two options that I see around how to address that issue- either we arrange to create space in the page for the tag, such as by making the 'special' space on a page a bit bigger and making sure that everything understands that, or we'll need to add another fork in which we store the tags (and possibly other TDE/encryption related information). If we go with a fork then it should be possible to do WAL streaming from an unencrypted cluster to an encrypted one, which would be pretty neat, but it means another fork and another page that has to be read/written every time we modify a page. Getting some input into the trade-offs here would be really helpful. I don't think it's really reasonable to go out with TDE without having figured out the integrity side. Certainly, when I review things like NIST 800-53, it's very clear that the requirement is for both confidentiality *and* integrity. > This also does not protect from users who have read access to system > memory. This also does not detect or protect against users with write > access from removing or modifying database files. The last seems a bit obvious, but the first sentence quoted above is important to make clear. I might even say: All of the pages in memory and all of the keys which are used for the encryption and decryption are stored in the clear in memory and therefore an attacker who is able to read the memory allocated by PostgreSQL would be able to decrypt the enitre cluster. > Given what I said above, is the value of this feature for compliance, or > for actual additional security? If it just compliance, are we willing > to add all of this code just for that, even if it has limited security > value? We should answer this question now, and if we don't want it, > let's document that so users know and can consider alternatives. The feature is for both compliance and additional security. While there are other ways to achieve data-at-rest encryption, they are not always available, for a variety of reasons. > FYI, I don't think we can detect or protect against writers modifying > the data files --- even if we could do it on a block level, they could > remove trailing pages (might cause index lookup failures) or copy > pages from other tables at the same offset. Therefore, I think we can > only offer
Re: Key management with tests
On Sat, Jan 30, 2021 at 08:23:11AM -0500, Tom Kincaid wrote: > I propose that we meet to discuss what approach we want to use to move TDE > forward. We then start a new thread with a proposal on the approach > and finalize it via community consensus. I will invite Bruce, Stephen and > Masahiko to this meeting. If anybody else would like to participate in this > discussion and subsequently in the effort to get TDE in PG1x, please let me > know. Assuming Bruce, Stephen and Masahiko are down for this, I (or a > volunteer > from this meeting) will post the proposal for how we move this patch forward > in > another thread. Hopefully, we can get consensus on that and subsequently > restart the execution of delivering this feature. We got complaints that decisions were not publicly discussed, or were too long, so I am not sure this helps. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Fri, Jan 29, 2021 at 05:05:06PM +0900, Masahiko Sawada wrote: > TBH I’m confused a bit about the recent situation of this patch, but > I Yes, it is easy to get confused. > can contribute to KMS work by discussing, writing, reviewing, and > testing the patch. Also, I can work on the data encryption part of TDE Great. > (we need more discussion on that though). If the community concerns > about the high-level design and thinks the design reviews by > cryptography experts are still needed, we would need to do that first > since the data encryption part of TDE depends on KMS. As far as I I totally agree. While we don't need to commit the key management patch to the tree before moving forward, we should have agreement on the key management patch before doing more work on this. If we can't agree on the key management part, there is no value in working further, as I stated in an earlier email. > know, we have done that many times on pgsql-hackers, on offl-line and > including the discussion on the past proposal, etc but given that the > community still has a concern, it seems that we haven’t been able > to share the details of the discussion enough that led to the design > decision or the design is still not good. Honestly, I’m not sure how > this feature can get consensus. But maybe we would need to have a Yes, I am also confused. > break from refining the patch now and we need to marshal the > discussions so far and the point behind the design so that everyone > can understand why this feature is designed in that way. To do that, > it might be a good start to sort the wiki page since it has data > encryption part, KMS, and ToDo mixed. What I ended up doing is to moving the majority of the non-data-encryption part of the wiki into the patch, either in docs or README files, since people asked for more of this in the patch, and having the information in two places is confusing. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Dear All. Thank you for all opinions and discussions regarding the KMS/TDE function. First of all, to get to the point of this email, I want to participate in anything I can do (review or development) when TDE related development is in progress. If there is a meeting related to it, I can't communicate because of my poor English skills, but I would like to attend if it is only possible to listen. I didn't understand KMS and didn't participate in the direct development, so I didn't comment on anything so far. Still, when TDE development starts, I wanted to join in the discussion and meeting if there was anything I could do. However, since I have a complicated and insufficient English ability to communicate in English, maybe I will rarely say anything in meetings (voice and video meetings). But I would like to attend the discussion if it is only possible to listen. Also, if the wiki page and other mail threads related to TDE start, I'll join in discussions if there is anything I can do. Best regards. Moon. On Sat, Jan 30, 2021 at 10:23 PM Tom Kincaid wrote: > > > > > > Thanks Stephen, Bruce and Masahiko, > >> >> > discussions so far and the point behind the design so that everyone >> > can understand why this feature is designed in that way. To do that, >> > it might be a good start to sort the wiki page since it has data >> > encryption part, KMS, and ToDo mixed. >> >> I hope it's pretty clear that I'm also very much in support of both this >> effort with the KMS and of TDE in general- TDE is specifically, >> repeatedly, called out as a capability whose lack is blocking PG from >> being able to be used for certain use-cases that it would otherwise be >> well suited for, and that's really unfortunate. > > > It is clear you are supportive. > > As you know, I share your point of view that PG adoption is suffering for > certain use cases because it does not have TDE. > >> I appreciate the recent discussion and reviews of the KMS in particular, >> and of the patches which have been sent enabling TDE based on the KMS >> patches. Having them be relatively independent seems to be an ongoing >> concern and perhaps we should figure out a way to more clearly put them >> together. That is- the KMS patches have been posted on one thread, and >> TDE PoC patches which use the KMS patches have been on another thread, >> leading some to not realize that there's already been TDE PoC work done >> based on the KMS patches. Seems like it might make sense to get one >> patch set which goes all the way from the KMS and includes the TDE PoC, >> even if they don't all go in at once. > > > Sounds good, thanks Masahiko, let's see if we can get consensus on the > approach for moving this forward see below. > >> >> >> together, as a few on this thread have voiced, but there's no doubt that >> this is a large project and it's hard to see how we could possibly >> commit all of it at once. > > > I propose that we meet to discuss what approach we want to use to move TDE > forward. We then start a new thread with a proposal on the approach and > finalize it via community consensus. I will invite Bruce, Stephen and > Masahiko to this meeting. If anybody else would like to participate in this > discussion and subsequently in the effort to get TDE in PG1x, please let me > know. Assuming Bruce, Stephen and Masahiko are down for this, I (or a > volunteer from this meeting) will post the proposal for how we move this > patch forward in another thread. Hopefully, we can get consensus on that and > subsequently restart the execution of delivering this feature. > > > > >> >> Thanks! >> >> Stephen > > > > -- > Thomas John Kincaid >
Re: Key management with tests
Thanks Stephen, Bruce and Masahiko, > > discussions so far and the point behind the design so that everyone > > can understand why this feature is designed in that way. To do that, > > it might be a good start to sort the wiki page since it has data > > encryption part, KMS, and ToDo mixed. > > I hope it's pretty clear that I'm also very much in support of both this > effort with the KMS and of TDE in general- TDE is specifically, > repeatedly, called out as a capability whose lack is blocking PG from > being able to be used for certain use-cases that it would otherwise be > well suited for, and that's really unfortunate. > It is clear you are supportive. As you know, I share your point of view that PG adoption is suffering for certain use cases because it does not have TDE. I appreciate the recent discussion and reviews of the KMS in particular, > and of the patches which have been sent enabling TDE based on the KMS > patches. Having them be relatively independent seems to be an ongoing > concern and perhaps we should figure out a way to more clearly put them > together. That is- the KMS patches have been posted on one thread, and > TDE PoC patches which use the KMS patches have been on another thread, > leading some to not realize that there's already been TDE PoC work done > based on the KMS patches. Seems like it might make sense to get one > patch set which goes all the way from the KMS and includes the TDE PoC, > even if they don't all go in at once. > Sounds good, thanks Masahiko, let's see if we can get consensus on the approach for moving this forward see below. > > together, as a few on this thread have voiced, but there's no doubt that > this is a large project and it's hard to see how we could possibly > commit all of it at once. > I propose that we meet to discuss what approach we want to use to move TDE forward. We then start a new thread with a proposal on the approach and finalize it via community consensus. I will invite Bruce, Stephen and Masahiko to this meeting. If anybody else would like to participate in this discussion and subsequently in the effort to get TDE in PG1x, please let me know. Assuming Bruce, Stephen and Masahiko are down for this, I (or a volunteer from this meeting) will post the proposal for how we move this patch forward in another thread. Hopefully, we can get consensus on that and subsequently restart the execution of delivering this feature. > Thanks! > > Stephen > -- Thomas John Kincaid
Re: Key management with tests
Greetings, * Masahiko Sawada (sawada.m...@gmail.com) wrote: > On Fri, Jan 29, 2021 at 5:22 AM Bruce Momjian wrote: > > On Thu, Jan 28, 2021 at 02:41:09PM -0500, Tom Kincaid wrote: > > > I would also like to add a "not wanted" entry for this feature on the > > > TODO list, baaed on the feature's limited usefulness, but I already > > > asked about that and no one seems to feel we don't want it. > > > > > > > > > I want to avoid seeing this happen. As a result of a lot of customer and > > > user > > > discussions, around their criteria for choosing a database, I believe TDE > > > is an > > > important feature and having it appear with a "not-wanted" tag will keep > > > the > > > version of PostgreSQL released by the community out of certain (and > > > possibly > > > growing) number of deployment scenarios which I don't think anybody wants > > > to > > > see. > > > > With pg_upgrade, I could work on it out of the tree until it became > > popular, with a small non-user-visible part in the backend. With the > > Windows port, the port wasn't really visible to users until it we ready. > > > > For the key management part of TDE, it can't be done outside the tree, > > and it is user-visible before it is useful, so that restricts how much > > incremental work can be committed to the tree for TDE. I highlighted > > that concern emails months ago, but never got any feedback --- now it > > seems people are realizing the ramifications of that. > > > > > I think the current situation to be as follows (if I missed something > > > please > > > let me know): > > > > > > 1) We need to get the current patch for Key Management reviewed and tested > > > further. > > > > > > I spoke to Bruce just now he will see if can get somebody to do this. > > > > Well, if we don't get anyone committed to working on the data encryption > > part of TDE, the key management part is useless, so why review/test it > > further? > > > > Although Sawada-san and Stephen Frost worked on the patch, they have not > > commented much on my additions, and only a few others have commented on > > the code, and there has been no discussion on who is working on the next > > steps. This indicates to me that there is little interest in moving > > this feature forward, > > TBH I’m confused a bit about the recent situation of this patch, but I > can contribute to KMS work by discussing, writing, reviewing, and > testing the patch. Also, I can work on the data encryption part of TDE > (we need more discussion on that though). If the community concerns > about the high-level design and thinks the design reviews by > cryptography experts are still needed, we would need to do that first > since the data encryption part of TDE depends on KMS. As far as I > know, we have done that many times on pgsql-hackers, on offl-line and > including the discussion on the past proposal, etc but given that the > community still has a concern, it seems that we haven’t been able to > share the details of the discussion enough that led to the design > decision or the design is still not good. Honestly, I’m not sure how > this feature can get consensus. But maybe we would need to have a > break from refining the patch now and we need to marshal the > discussions so far and the point behind the design so that everyone > can understand why this feature is designed in that way. To do that, > it might be a good start to sort the wiki page since it has data > encryption part, KMS, and ToDo mixed. I hope it's pretty clear that I'm also very much in support of both this effort with the KMS and of TDE in general- TDE is specifically, repeatedly, called out as a capability whose lack is blocking PG from being able to be used for certain use-cases that it would otherwise be well suited for, and that's really unfortunate. I appreciate the recent discussion and reviews of the KMS in particular, and of the patches which have been sent enabling TDE based on the KMS patches. Having them be relatively independent seems to be an ongoing concern and perhaps we should figure out a way to more clearly put them together. That is- the KMS patches have been posted on one thread, and TDE PoC patches which use the KMS patches have been on another thread, leading some to not realize that there's already been TDE PoC work done based on the KMS patches. Seems like it might make sense to get one patch set which goes all the way from the KMS and includes the TDE PoC, even if they don't all go in at once. I'm happy to go look over the KMS patches again if that'd be helpful and to comment on the TDE PoC. I can also spend some time trying to improve on each, as I've already done. A few of the larger concerns that I have revolve around how to store integrity information (I've tried to find a way to make room for such information in our existing page layout and, perhaps unsuprisingly, it's far from trivial to do so in a way that will avoid breaking the existing page layout, or where the
Re: Key management with tests
On Fri, Jan 29, 2021 at 5:22 AM Bruce Momjian wrote: > > On Thu, Jan 28, 2021 at 02:41:09PM -0500, Tom Kincaid wrote: > > I would also like to add a "not wanted" entry for this feature on the > > TODO list, baaed on the feature's limited usefulness, but I already > > asked about that and no one seems to feel we don't want it. > > > > > > I want to avoid seeing this happen. As a result of a lot of customer and > > user > > discussions, around their criteria for choosing a database, I believe TDE > > is an > > important feature and having it appear with a "not-wanted" tag will keep the > > version of PostgreSQL released by the community out of certain (and possibly > > growing) number of deployment scenarios which I don't think anybody wants to > > see. > > With pg_upgrade, I could work on it out of the tree until it became > popular, with a small non-user-visible part in the backend. With the > Windows port, the port wasn't really visible to users until it we ready. > > For the key management part of TDE, it can't be done outside the tree, > and it is user-visible before it is useful, so that restricts how much > incremental work can be committed to the tree for TDE. I highlighted > that concern emails months ago, but never got any feedback --- now it > seems people are realizing the ramifications of that. > > > I think the current situation to be as follows (if I missed something please > > let me know): > > > > 1) We need to get the current patch for Key Management reviewed and tested > > further. > > > > I spoke to Bruce just now he will see if can get somebody to do this. > > Well, if we don't get anyone committed to working on the data encryption > part of TDE, the key management part is useless, so why review/test it > further? > > Although Sawada-san and Stephen Frost worked on the patch, they have not > commented much on my additions, and only a few others have commented on > the code, and there has been no discussion on who is working on the next > steps. This indicates to me that there is little interest in moving > this feature forward, TBH I’m confused a bit about the recent situation of this patch, but I can contribute to KMS work by discussing, writing, reviewing, and testing the patch. Also, I can work on the data encryption part of TDE (we need more discussion on that though). If the community concerns about the high-level design and thinks the design reviews by cryptography experts are still needed, we would need to do that first since the data encryption part of TDE depends on KMS. As far as I know, we have done that many times on pgsql-hackers, on offl-line and including the discussion on the past proposal, etc but given that the community still has a concern, it seems that we haven’t been able to share the details of the discussion enough that led to the design decision or the design is still not good. Honestly, I’m not sure how this feature can get consensus. But maybe we would need to have a break from refining the patch now and we need to marshal the discussions so far and the point behind the design so that everyone can understand why this feature is designed in that way. To do that, it might be a good start to sort the wiki page since it has data encryption part, KMS, and ToDo mixed. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Key management with tests
On Thu, Jan 28, 2021 at 02:41:09PM -0500, Tom Kincaid wrote: > I would also like to add a "not wanted" entry for this feature on the > TODO list, baaed on the feature's limited usefulness, but I already > asked about that and no one seems to feel we don't want it. > > > I want to avoid seeing this happen. As a result of a lot of customer and user > discussions, around their criteria for choosing a database, I believe TDE is > an > important feature and having it appear with a "not-wanted" tag will keep the > version of PostgreSQL released by the community out of certain (and possibly > growing) number of deployment scenarios which I don't think anybody wants to > see. With pg_upgrade, I could work on it out of the tree until it became popular, with a small non-user-visible part in the backend. With the Windows port, the port wasn't really visible to users until it we ready. For the key management part of TDE, it can't be done outside the tree, and it is user-visible before it is useful, so that restricts how much incremental work can be committed to the tree for TDE. I highlighted that concern emails months ago, but never got any feedback --- now it seems people are realizing the ramifications of that. > I think the current situation to be as follows (if I missed something please > let me know): > > 1) We need to get the current patch for Key Management reviewed and tested > further. > > I spoke to Bruce just now he will see if can get somebody to do this. Well, if we don't get anyone committed to working on the data encryption part of TDE, the key management part is useless, so why review/test it further? Although Sawada-san and Stephen Frost worked on the patch, they have not commented much on my additions, and only a few others have commented on the code, and there has been no discussion on who is working on the next steps. This indicates to me that there is little interest in moving this feature forward, which is why I started asking if it could be labeled as "not wanted". -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Hello, > > I don't think it makes sense to think about committing this to v14. I > > believe it only makes sense if we have a TDE patch that is relatively > > close to committable that can be used with it. I also don't think that > > this patch is in good enough shape to commit yet in terms of where > > it's at in terms of quality; I think it needs more review first, > > hopefully including review from people who can comment intelligently > > specifically on the cryptography aspects of it. However, the > > challenges don't seem insurmountable. There's also still some question > > in my mind about whether the design choices here (one KEK, 2 DEKs, one > > for data and one for WAL) have enough consensus. I don't have a > > considered opinion on that, partly because I'm not quite sure what the > > reasonable alternatives are, but it seems that other people had some > > questions about it, IIUC. > > While I am willing to make requested adjustments to the patch, I don't > plan to work on this feaure any further, assuming your analysis above is > correct. If after years we are still not sure this is the right > direction, I don't see any point in moving forward with the later > pieces, which are even more complicated. I will join the group of > people that feel there will never be consensus on implementing this > feature in the community, so it is not worth trying. > > I would also like to add a "not wanted" entry for this feature on the > TODO list, baaed on the feature's limited usefulness, but I already > asked about that and no one seems to feel we don't want it. > I want to avoid seeing this happen. As a result of a lot of customer and user discussions, around their criteria for choosing a database, I believe TDE is an important feature and having it appear with a "not-wanted" tag will keep the version of PostgreSQL released by the community out of certain (and possibly growing) number of deployment scenarios which I don't think anybody wants to see. I think the current situation to be as follows (if I missed something please let me know): 1) We need to get the current patch for Key Management reviewed and tested further. I spoke to Bruce just now he will see if can get somebody to do this. 2) We need to start working on the actual TDE implementation and get it pretty close to final before we start committing smaller portions of the feature. Unfortunately, on this front, the only things, I think I can offer are: a) Ask for volunteers to work on the TDE implementation. b) Facilitate the work between volunteers. c) Prod folks along and cheer as we go. So I will start with (a), do we have any volunteers who feel they can contribute regularly for a while and would like to be part of a team that moves this forward? I now better understand why the OpenSSL project has had such serious > problems in the past. > > Updated patch attached as seven attachments. > > -- > Bruce Momjian https://momjian.us > EDB https://enterprisedb.com > > The usefulness of a cup is in its emptiness, Bruce Lee > > -- Thomas John Kincaid
Re: Key management with tests
On Tue, Jan 26, 2021 at 05:53:01PM -0500, Bruce Momjian wrote: > On Tue, Jan 26, 2021 at 03:24:30PM -0500, Robert Haas wrote: > > I'm wondering whether you've considered storing all the keys in one > > file instead of a file per key. The reason I ask is that it seems to > > me that the key rotation procedure would be a lot simpler if it were > > all in one file. You could just create a temporary file and atomically > > rename it over the existing file. If you see a temporary file you're > > always free to remove it. This is a lot simpler than what you have > > right now. The "repair" concept pretty much goes away completely, > > which seems nice. Granted I don't know exactly how to store multiple > > keys in one file, but I bet there's some way to do it. > > We envisioned allowing heap/index key rotation by having a standby with > the same WAL key as the primary but different heap/index keys so that we > can failover to the standby to change the heap/index key and then change > the WAL key. This separation allows that. We also might need some > additional keys later and this allows that. I do like simplicity, but > the complexity here seems to serve a need. Just to close this issue, several scripts, e,g., PIV, AWS, need to store data to indicate the cluster encryption key used, and those need to be kept synchronized with the wrapped data keys. Having separate directories for each cluster key version allows that to work cleanly. > > The README in src/backend/crypto does not explain how the scripts in > > that directory are intended to be used. If I want to use AWS Secrets > > Manager with this feature, I can see that I should use > > ckey_aws.sh.sample as a basis for that integration, but I don't know > > what I should do with the script because the README says nothing about > > it. I am frankly pretty doubtful about the idea of shipping a bunch of > > /bin/sh scripts as a best practice; for one thing, that's totally > > unusable on Windows, and it also means that this is dependent on > > /bin/sh existing and having the behavior we expect and on all the > > other executables in these scripts as well. But, at the very least, > > there needs to be a clearer explanation of how the scripts are > > intended to be used, which parts people are supposed to modify, what > > arguments they're going to get called with, and things like that. > > I added comments to most of the scripts. I don't know what more I can > do, or what other language would be appropriate. I think someone would need to write Windows versions of these scripts. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Tue, Jan 26, 2021 at 11:15 AM Bruce Momjian wrote: > This version fixes OpenSSL detection and improves docs for initdb > interactions. Hi, I'm wondering whether you've considered storing all the keys in one file instead of a file per key. The reason I ask is that it seems to me that the key rotation procedure would be a lot simpler if it were all in one file. You could just create a temporary file and atomically rename it over the existing file. If you see a temporary file you're always free to remove it. This is a lot simpler than what you have right now. The "repair" concept pretty much goes away completely, which seems nice. Granted I don't know exactly how to store multiple keys in one file, but I bet there's some way to do it. The way in which you are posting these patches is quite unlike what most people do when posting patches to this list. You seem to have generated a bunch of patches using 'git format-patch' but then concatenated them all together in a single file. It would be helpful if you could do this more like the way that is now standard on this list. Not only that, but the patches don't have meaningful commit messages in them, and don't seem to be meaningfully split for easy review. They just say things like 'crypto squash commit'. Compare this to for example what I did on the "cleaning up a few CLOG-related things" thread where the commits appear in a logical sequence, and each one has a meaningful commit message. Or here's an example from someone else -- http://postgr.es/m/be72abfa-e62e-eb81-4e70-1b57fe6dc...@amazon.com -- and note the inclusion of authorship information in the commit messages, so that the source of the code can be easily understood. The README in src/backend/crypto does not explain how the scripts in that directory are intended to be used. If I want to use AWS Secrets Manager with this feature, I can see that I should use ckey_aws.sh.sample as a basis for that integration, but I don't know what I should do with the script because the README says nothing about it. I am frankly pretty doubtful about the idea of shipping a bunch of /bin/sh scripts as a best practice; for one thing, that's totally unusable on Windows, and it also means that this is dependent on /bin/sh existing and having the behavior we expect and on all the other executables in these scripts as well. But, at the very least, there needs to be a clearer explanation of how the scripts are intended to be used, which parts people are supposed to modify, what arguments they're going to get called with, and things like that. The comments in cipher.c and cipher_openssl.c could be improved to explain that they are alternatives to each other. Perhaps the former could be renamed to something like cipher_failure.c or cipher_noimpl.c for clarity. I believe that a StaticAssertStmt could be used to check the length of the encryption_methods[] array, so that if someone changes NUM_ENCRYPTION_METHODS without updating the array, compilation fails. See UserAuthName[] for an example of how to do this. You seem to have omitted to update the documentation with the names of the new wait events that you added. In process_postgres_switches(), when there's a multi-line comment followed by a single line of actual code, I prefer to include braces around the whole thing. There might be some disagreement on what is best here, though. What are the consequences of the placement of the code in PostgresMain() for processes other than user backends and walsenders? I think that the way you have it, background workers would not have access to keys, nor auxiliary processes like the checkpointer ... at least in the EXEC_BACKEND case. In the non-EXEC_BACKEND case you have the postmaster doing it, so then I'm not sure why it has to be redone for every backend. Won't they just inherit the data from the postmaster? Has this code been meaningfully tested on Windows? How do we know that it works? Maybe we need to think about adding some asserts that guarantee that any process that attempts to access a buffer has the key manager initialized; I bet such assertions would fail at least on Windows as the code looks now. I don't think it makes sense to think about committing this to v14. I believe it only makes sense if we have a TDE patch that is relatively close to committable that can be used with it. I also don't think that this patch is in good enough shape to commit yet in terms of where it's at in terms of quality; I think it needs more review first, hopefully including review from people who can comment intelligently specifically on the cryptography aspects of it. However, the challenges don't seem insurmountable. There's also still some question in my mind about whether the design choices here (one KEK, 2 DEKs, one for data and one for WAL) have enough consensus. I don't have a considered opinion on that, partly because I'm not quite sure what the reasonable alternatives are, but it seems that other people had some questions about
Re: Key management with tests
On Mon, Jan 25, 2021 at 08:12:01PM -0300, Álvaro Herrera wrote:
> In patch 1,
>
> * The docs are not clear on what happens if --auth-prompt is not given
> but an auth prompt is required for the program to work. Should it exit
> with a status other than 0?
Uh, I think the docs talk about this:
It can prompt from the terminal if
option>--authprompt is used. In the parameter
value, %R is replaced by a file descriptor
number opened to the terminal that started the server. A file
descriptor is only available if enabled at server start via
-R. If %R is specified and
no file descriptor is available, the server will not start.
The code is:
case 'R':
{
char fd_str[20];
if (terminal_fd == -1)
{
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("cluster key command referenced
%%R, but --authprompt not specified")));
}
Does that help?
> * BootStrapKmgr claims it is called by initdb, but that doesn't seem to
> be the case.
Well, initdb starts the postmaster in --boot mode, and that calls
BootStrapKmgr(). Does that help?
> * Also, BootStrapKmgr is the only one that checks USE_OPENSSL; what if a
> with-openssl build inits the datadir, and then a non-openssl runs it?
> What if it's the other way around? I think you'd get a failure in
> stat() ...
Wow, I never considered that. I have added a check to InitializeKmgr().
Thanks.
> * ... oh, KMGR_DIR_PID is used but not defined anywhere. Is it defined
> in some later commit? If so, then I think you've chosen to split the
> patch series wrong.
OK, fixed. It is in include/common/kmgr_utils.c, which was in #3.
> May I suggest to use "git format-patch" to produce the patch files? When
> working with a series like this, trying to do patch handling manually
> like you seem to be doing, is much more time-consuming and error prone.
> For example, with a branch containing individual commits, you could use
> git rebase -i origin/master -x "make install check-world"
> or similar, so that each commit is built and tested individually.
I used "git format-patch". Are you asking for seven commits that then
generate seven files via one format-patch run? Or is the primary issue
that you want compile testing for each patch?
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
In patch 1, * The docs are not clear on what happens if --auth-prompt is not given but an auth prompt is required for the program to work. Should it exit with a status other than 0? * BootStrapKmgr claims it is called by initdb, but that doesn't seem to be the case. * Also, BootStrapKmgr is the only one that checks USE_OPENSSL; what if a with-openssl build inits the datadir, and then a non-openssl runs it? What if it's the other way around? I think you'd get a failure in stat() ... * ... oh, KMGR_DIR_PID is used but not defined anywhere. Is it defined in some later commit? If so, then I think you've chosen to split the patch series wrong. May I suggest to use "git format-patch" to produce the patch files? When working with a series like this, trying to do patch handling manually like you seem to be doing, is much more time-consuming and error prone. For example, with a branch containing individual commits, you could use git rebase -i origin/master -x "make install check-world" or similar, so that each commit is built and tested individually. -- Álvaro Herrera Valdivia, Chile Al principio era UNIX, y UNIX habló y dijo: "Hello world\n". No dijo "Hello New Jersey\n", ni "Hello USA\n".
Re: Key management with tests
On Mon, Jan 18, 2021 at 04:38:47PM -0500, Robert Haas wrote: > To me, it wouldn't make sense to commit a full README for a TDE > feature that we don't have yet with a key management patch, but the > way that they'll interact with each other has to be clear. The > doc/database-encryption.sgml file that Bruce included in the patch is > a decent start on explaining the design, though I think it needs more > work and more details, perhaps including some of the things Andres > mentioned. Sure. > To be honest, after reading over that SGML documentation a bit, I'm > somewhat skeptical about whether it really makes sense to think about > committing the key management part separately. It seems to have no use > independent of the main feature, and it in fact embeds very specific For usefulness, it does enable passphrase prompting for the TLS private key. > details of how the main feature is expected to work. For example, the > documentation says that key #0 will be used for data files, and key #1 > for WAL. There seems to be no suggestion that the key management > portion of this can be used to manage encryption keys generally for > whatever purposes someone might have; it's all about the needs of a > particular TDE implementation. Typically, we would not commit We originally were going to have SQL-level keys, but many felt they weren't useful. > something like that separately, or only once the main patch was done, > with the two commits occurring in a relatively short time period. > Otherwise, as Bruce already noted, we can end up with something that > is documented and visible to users but doesn't actually work yet. Yep, that is the risk. > Some more specific comments on data-encryption.sgml: > > * The documentation explains that the purpose of having a WAL key > separate from the data file key is so that the data file keys can > "eventually" be rotated. It's not clear whether this means that we > might eventually have that feature or that we might eventually be able > to rotate, after failing over. If this kind of thing is possible, > we'll eventually need documentation on how to do it. I have clarified that saying "future release". > * The reasons for use a DEK and a KEK are not explained. I realize > it's not an uncommon practice and that other systems do it, but I > think a few sentences of explanation wouldn't be a bad idea. Even if > we are supposing that hackers who want to have input into this feature > have to be knowledgeable about cryptography, I don't think we can > reasonably suppose that for users. I added a little about that in the docs. > * "For example" is at one point followed by a period rather than a > colon or comma. Fixed. > * In the "Internals" subsection, the last sentence doesn't seem to be > grammatical. I wonder if it's missing the word "or"'. Fixed. > * The part about integrity-checking keys on startup isn't clear. It > makes it sound like we still have a copy of the KEK lying around > someplace against which we can compare, which I assume is not the case > since it would be really insecure. I rewored that entire section. See if it is better now. > * I think it's going to be pretty important that we can easily switch > to other cryptographic algorithms as they are discovered, so I don't > like the fact that this is tied specifically to AES. (In fact, > kmgr_utils.h makes it sound like we're specifically locked into > AES256, but that contradicts the documentation, so I guess there's > some clarification needed here about what exactly KMGR_CLUSTER_KEY_LEN > is doing.) As far as possible we should try to make this generic, like > supporting any cipher that SSL has which has property X. It seems > relatively inevitable that every currently popular cryptographic > algorithm will at some point in the future be judged weak and > worthless, just as has already happened with MD5 and some variants of > SHA, both of which used to be considered state of the art. It seems > equally inevitable that new and stronger algorithms will continued to > be devised, and we'll want to adopt those easily. That is a nifty idea. Right now I just pass the integer length around, and store it in pg_control, but if we define macros, we can easily abstract this and easily allow for new methods. If others like that, I will start on it now. > I'm not sure to what extent this a serious flaw in the patch and to > what extent it's just a matter of tweaking the wording of some things, > but I think this is actually an extremely critical design point where > we had better be certain we've got it right. Few things would be > sadder than to get a TDE feature and then have to rip it out again > because it couldn't be upgraded to work with newer crypto algorithms > with reasonable effort. Yep. > Notes on other parts of the documentation: > > * The documentation for initdb -K doesn't list the valid values of the > parameter, only the default. Probably we should be specifying an Fixed. > algorithm
Re: Key management with tests
I met with Bruce and Stephen this afternoon to discuss the feedback we received so far (prior to Robert's note which I haven't fully digested yet) on this patch. Here is what we plan to do: 1) Bruce is going to gather all the details from the Wiki and build a README for the TDE Key Management patch. In addition, it will include details about the implementation, the data structures involved and the locks that are taken and general technical implementation approach. 2) Stephen is going to write up the overall design of TDE. Between these two patches, we hope to cover what Andres is asking for and what Robert is asking for in his reply on this thread which I haven't fully digested yet. Stephen's documentation patch will also make reference to Neil Chen's TDE prototype for making use of this Key Management patch to encrypt and decrypt heap pages as well as index pages. https://www.postgresql.org/message-id/CAA3qoJ=qto5jcsbjqfdbt9ikux9xkmc5bxcrd7ryse+xsme...@mail.gmail.com 3) Tom will work to find somebody who will sign up as a reviewer upon the next submission of this patch. (Somebody who is not an author). Could we get feedback if this feels like enough to get this patch (which will include just the Key Management portion of TDE) to a state where it can be reviewed and assuming the review issues are resolved with consensus be committed? On Mon, Jan 18, 2021 at 2:00 PM Andres Freund wrote: > > On 2021-01-18 13:58:20 -0500, Bruce Momjian wrote: > > On Mon, Jan 18, 2021 at 09:42:54AM -0800, Andres Freund wrote: > > > Personally, but I admit that there's legitimate reasons to differ on > > > that note, I don't think it's reasonable for a feature this invasive to > > > commit preliminary patches without the major subsequent patches being in > > > a shape that allows reviewing the whole picture. > > > > OK, if that is a requirement, I can't help anymore since there are > > already complaints that the patch is too large to review, even if broken > > into pieces. Please let me know what the community decides. > > Those aren't conflicting demands. Having later patches around to > validate the design of earlier patches doesn't necessitates that the > later patches need to be reviewed at the same time. -- Thomas John Kincaid
Re: Key management with tests
On Mon, Jan 18, 2021 at 2:00 PM Tom Kincaid wrote: > Some of the missing things you mention above are about the design of > TDE feature in general. However, this patch is about Key Management > which is going part of the larger TDE feature. So it feels as though > there is the need for a general design document about the overall > vision / approach for TDE and a specific design doc. for Key > Management. Is it appropriate to include both of those in the same > patch? To me, it wouldn't make sense to commit a full README for a TDE feature that we don't have yet with a key management patch, but the way that they'll interact with each other has to be clear. The doc/database-encryption.sgml file that Bruce included in the patch is a decent start on explaining the design, though I think it needs more work and more details, perhaps including some of the things Andres mentioned. To be honest, after reading over that SGML documentation a bit, I'm somewhat skeptical about whether it really makes sense to think about committing the key management part separately. It seems to have no use independent of the main feature, and it in fact embeds very specific details of how the main feature is expected to work. For example, the documentation says that key #0 will be used for data files, and key #1 for WAL. There seems to be no suggestion that the key management portion of this can be used to manage encryption keys generally for whatever purposes someone might have; it's all about the needs of a particular TDE implementation. Typically, we would not commit something like that separately, or only once the main patch was done, with the two commits occurring in a relatively short time period. Otherwise, as Bruce already noted, we can end up with something that is documented and visible to users but doesn't actually work yet. Some more specific comments on data-encryption.sgml: * The documentation explains that the purpose of having a WAL key separate from the data file key is so that the data file keys can "eventually" be rotated. It's not clear whether this means that we might eventually have that feature or that we might eventually be able to rotate, after failing over. If this kind of thing is possible, we'll eventually need documentation on how to do it. * The reasons for use a DEK and a KEK are not explained. I realize it's not an uncommon practice and that other systems do it, but I think a few sentences of explanation wouldn't be a bad idea. Even if we are supposing that hackers who want to have input into this feature have to be knowledgeable about cryptography, I don't think we can reasonably suppose that for users. * "For example" is at one point followed by a period rather than a colon or comma. * In the "Internals" subsection, the last sentence doesn't seem to be grammatical. I wonder if it's missing the word "or"'. * The part about integrity-checking keys on startup isn't clear. It makes it sound like we still have a copy of the KEK lying around someplace against which we can compare, which I assume is not the case since it would be really insecure. * I think it's going to be pretty important that we can easily switch to other cryptographic algorithms as they are discovered, so I don't like the fact that this is tied specifically to AES. (In fact, kmgr_utils.h makes it sound like we're specifically locked into AES256, but that contradicts the documentation, so I guess there's some clarification needed here about what exactly KMGR_CLUSTER_KEY_LEN is doing.) As far as possible we should try to make this generic, like supporting any cipher that SSL has which has property X. It seems relatively inevitable that every currently popular cryptographic algorithm will at some point in the future be judged weak and worthless, just as has already happened with MD5 and some variants of SHA, both of which used to be considered state of the art. It seems equally inevitable that new and stronger algorithms will continued to be devised, and we'll want to adopt those easily. I'm not sure to what extent this a serious flaw in the patch and to what extent it's just a matter of tweaking the wording of some things, but I think this is actually an extremely critical design point where we had better be certain we've got it right. Few things would be sadder than to get a TDE feature and then have to rip it out again because it couldn't be upgraded to work with newer crypto algorithms with reasonable effort. Notes on other parts of the documentation: * The documentation for initdb -K doesn't list the valid values of the parameter, only the default. Probably we should be specifying an algorithm here and not just a bit count. Otherwise, like I say above, what happens when AES gives way to something else? It'd be easy to say -K BFT256 instead of -K AES256, but if AES is assumed and it's no longer what we want them we have problems. This kind of thing probably needs to be cleaned up in a bunch of places. * I don't see the
Re: Key management with tests
On 2021-01-18 13:58:20 -0500, Bruce Momjian wrote: > On Mon, Jan 18, 2021 at 09:42:54AM -0800, Andres Freund wrote: > > Personally, but I admit that there's legitimate reasons to differ on > > that note, I don't think it's reasonable for a feature this invasive to > > commit preliminary patches without the major subsequent patches being in > > a shape that allows reviewing the whole picture. > > OK, if that is a requirement, I can't help anymore since there are > already complaints that the patch is too large to review, even if broken > into pieces. Please let me know what the community decides. Those aren't conflicting demands. Having later patches around to validate the design of earlier patches doesn't necessitates that the later patches need to be reviewed at the same time.
Re: Key management with tests
> > I have to admit I was kind of baffled that the wiki page wasn't > > sufficient, because it is one of the longest Postgres feature > > explanations I have seen, but I now think the missing part is tying > > the wiki contents to the code implementation. If that is it, please > > confirm. If it is something else, also explain. > > I don't think the wiki right now covers what's needed. The "Overview", > "Threat model" and "Scope of TDE" are a start, but beyond that it's > missing a bunch of things. And it's not in the source tree (we'll soon > have multiple versions of postgres with increasing levels of TDE > features, the wiki doesn't help with that) > Thanks, the versioning issue makes sense for the design document needing to be part of the the source tree. As I was reading the README for the patch Amit referenced and as I am going through this patch, I feel the desire to incorporate diagrams. Are design diagrams ever incorporated in the source tree as a part of the design description of a feature? If not, any concerns about doing that? I think that is likely where I can contribute the most. > Missing: > - talks about cluster wide encyrption being simpler, without mentioning > what it's being compared to, and what makes it simpler > - no differentiation from file system / block level encryption > - there's no explanation of which/why specific crypto primitives were > chosen, what the tradeoffs are > - no explanation which keys exists, stored where > - the key management patch introduces new files, not documented > - there's new types of lock files, possibility of interrupted > operations, ... - no documentation of what that means > - there's no documentation what "key wrapping" actually precisely is, > what the danger of the two-tier model is, ... > - are there dangers in not encrypting zero pages etc? > - ... > Some of the missing things you mention above are about the design of TDE feature in general. However, this patch is about Key Management which is going part of the larger TDE feature. So it feels as though there is the need for a general design document about the overall vision / approach for TDE and a specific design doc. for Key Management. Is it appropriate to include both of those in the same patch? Something along the lines here is the overall design of TDE and here is how the Key Management portion is designed and implemented. I guess in that case, follow on patches for TDE could refer to the overall design described in this patch. > > > Personally, but I admit that there's legitimate reasons to differ on > that note, I don't think it's reasonable for a feature this invasive to > commit preliminary patches without the major subsequent patches being in > a shape that allows reviewing the whole picture. > > Greetings, > > Andres Freund -- Thomas John Kincaid
Re: Key management with tests
On Mon, Jan 18, 2021 at 09:42:54AM -0800, Andres Freund wrote: > Personally, but I admit that there's legitimate reasons to differ on > that note, I don't think it's reasonable for a feature this invasive to > commit preliminary patches without the major subsequent patches being in > a shape that allows reviewing the whole picture. OK, if that is a requirement, I can't help anymore since there are already complaints that the patch is too large to review, even if broken into pieces. Please let me know what the community decides. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Hi, On 2021-01-18 12:06:35 -0500, Bruce Momjian wrote: > On Mon, Jan 18, 2021 at 10:50:37AM -0500, Bruce Momjian wrote: > > OK, I looked at that and it is good, and I see my patch is missing that. > > Are people looking for me to take the wiki content, expand on it and tie > > it to the code that will be applied, or something else like all the > > various crypto options and why we chose what we did beyond what is > > already on the wiki? I can easily go from what we have on the wiki to > > implementation code steps, but the other part is harder to explain and > > that is why I offered to talk to people via voice. > > Just to clarify why voice calls can be helpful --- if you have to get > into "you have to understand X to understand Y", that's where a voice > call works best, because understanding X will require understanding > A/B/C, and everyone's missing pieces are different, so you have to > customize it for the individual. I don't think anybody argued against having voice calls. > You can explain some of this in a README, but trying to cover all of it > leads to a combinatorial problem of trying to explain everything. > Ideally the wiki page can be expanded so people can ask and answer all > posted issues, perhaps in a Q format. Someone could go through the > archives and post why certain decisions were made, and link to the > original emails. > > I have to admit I was kind of baffled that the wiki page wasn't > sufficient, because it is one of the longest Postgres feature > explanations I have seen, but I now think the missing part is tying > the wiki contents to the code implementation. If that is it, please > confirm. If it is something else, also explain. I don't think the wiki right now covers what's needed. The "Overview", "Threat model" and "Scope of TDE" are a start, but beyond that it's missing a bunch of things. And it's not in the source tree (we'll soon have multiple versions of postgres with increasing levels of TDE features, the wiki doesn't help with that) Missing: - talks about cluster wide encyrption being simpler, without mentioning what it's being compared to, and what makes it simpler - no differentiation from file system / block level encryption - there's no explanation of which/why specific crypto primitives were chosen, what the tradeoffs are - no explanation which keys exists, stored where - the key management patch introduces new files, not documented - there's new types of lock files, possibility of interrupted operations, ... - no documentation of what that means - there's no documentation what "key wrapping" actually precisely is, what the danger of the two-tier model is, ... - are there dangers in not encrypting zero pages etc? - ... Personally, but I admit that there's legitimate reasons to differ on that note, I don't think it's reasonable for a feature this invasive to commit preliminary patches without the major subsequent patches being in a shape that allows reviewing the whole picture. Greetings, Andres Freund
Re: Key management with tests
On Mon, Jan 18, 2021 at 10:50:37AM -0500, Bruce Momjian wrote: > OK, I looked at that and it is good, and I see my patch is missing that. > Are people looking for me to take the wiki content, expand on it and tie > it to the code that will be applied, or something else like all the > various crypto options and why we chose what we did beyond what is > already on the wiki? I can easily go from what we have on the wiki to > implementation code steps, but the other part is harder to explain and > that is why I offered to talk to people via voice. Just to clarify why voice calls can be helpful --- if you have to get into "you have to understand X to understand Y", that's where a voice call works best, because understanding X will require understanding A/B/C, and everyone's missing pieces are different, so you have to customize it for the individual. You can explain some of this in a README, but trying to cover all of it leads to a combinatorial problem of trying to explain everything. Ideally the wiki page can be expanded so people can ask and answer all posted issues, perhaps in a Q format. Someone could go through the archives and post why certain decisions were made, and link to the original emails. I have to admit I was kind of baffled that the wiki page wasn't sufficient, because it is one of the longest Postgres feature explanations I have seen, but I now think the missing part is tying the wiki contents to the code implementation. If that is it, please confirm. If it is something else, also explain. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Sat, Jan 16, 2021 at 10:58:47PM -0800, Andres Freund wrote: > Hi, > > On 2021-01-17 11:54:57 +0530, Amit Kapila wrote: > > On Sun, Jan 17, 2021 at 5:38 AM Tom Kincaid > > wrote: > > > Admittedly I am a novice on this topic, and the majority of the > > > PostgreSQL source code, however I am hopeful enough (those of you who > > > know me understand that I suffer from eternal optimism) that I am > > > going to attempt to help. > > > > > > Is there a design document for a Postgres feature of this size and > > > scope that people feel would serve as a good example? Alternatively, > > > is there a design document template that has been successfully used in > > > the past? > > > > > > > We normally write the design considerations and choices we made with > > the reasons in README and code comments. Personally, I am not sure if > > there is a need for any specific document per-se but a README and > > detailed comments in the code should suffice what people are worried > > about here. > > Right. It could be a README file, or a long comment at a start of one of > the files. It doesn't matter too much. What matters is that people that > haven't been on those phone call can understand the design and the > implications it has. OK, so does the wiki page contain most of what you want, but is missing the connection between the design and the code? https://wiki.postgresql.org/wiki/Transparent_Data_Encryption -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Sun, Jan 17, 2021 at 11:54:57AM +0530, Amit Kapila wrote: > > Is there a design document for a Postgres feature of this size and > > scope that people feel would serve as a good example? Alternatively, > > is there a design document template that has been successfully used in > > the past? > > We normally write the design considerations and choices we made with > the reasons in README and code comments. Personally, I am not sure if > there is a need for any specific document per-se but a README and > detailed comments in the code should suffice what people are worried > about here. It is mostly from the perspective that other developers > reading the code, want to do bug-fix, or later enhance that code > should be able to understand it. One recent example I can give is > Peter's work on bottom-up deletion [1] which I have read today where I > find that the design is captured via README, appropriate comments in > the code, and documentation. This feature is quite different and > probably a lot more new concepts are being introduced but I hope that > will give you some clue. > > [1] - > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d168b666823b6e0bcf60ed19ce24fb5fb91b8ccf OK, I looked at that and it is good, and I see my patch is missing that. Are people looking for me to take the wiki content, expand on it and tie it to the code that will be applied, or something else like all the various crypto options and why we chose what we did beyond what is already on the wiki? I can easily go from what we have on the wiki to implementation code steps, but the other part is harder to explain and that is why I offered to talk to people via voice. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Sun, Jan 17, 2021 at 07:50:13PM -0500, Robert Haas wrote: > On Fri, Jan 15, 2021 at 7:56 PM Andres Freund wrote: > > I think that's not at all acceptable. I don't mind hashing out details > > on calls / off-list, but the design needs to be public, documented, and > > reviewable. And if it's something the community can't understand, then > > it can't get in. We're going to have to maintain this going forward. > > I agree. If the community is unable to clearly understand what > something is, and why we should have it, then we shouldn't have it -- > even if the reason is that we're too dumb to understand, as Bruce I am not sure why you are brining intelligence into this discussion. You have to understand Postgres internals and cryptography tradeoffs to understand why some of the design decisions were made. It is a knowledge issue, not an intelligence issue. The wiki page is the result of those phone discussions. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Fri, Jan 15, 2021 at 7:56 PM Andres Freund wrote: > I think that's not at all acceptable. I don't mind hashing out details > on calls / off-list, but the design needs to be public, documented, and > reviewable. And if it's something the community can't understand, then > it can't get in. We're going to have to maintain this going forward. I agree. If the community is unable to clearly understand what something is, and why we should have it, then we shouldn't have it -- even if the reason is that we're too dumb to understand, as Bruce seems to be alleging. I don't really think I believe the theory that community members by and large are too dumb to understand encryption. Many features have provoked long and painful discussions about the design and yet got into the tree in the end with documentation of that design, and I don't see why that couldn't be done for this one, too. I think it can and should, and the fact that the work hasn't been done is one of several blockers for this patch. But even if I'm wrong, and the real problem is that everyone except the select group of people on these off-list phone calls are too stupid to understand this, then that's still a reason not to accept the patch. The code that's in our source tree is maintained by communal effort, and that means communal understanding is important. Frankly, it's more important in this particular case than in some others. TDE is in great demand, so if it gets into the tree, it's likely to get a lot of use. The preparatory patches, such as this one, would at that point be getting a lot of use, too. That means many people, not just hackers, will have to understand them and answer questions about them. They are also likely to get a lot of scrutiny from a security point of view, so we should have a way that we can be confident that we know why we believe them to be secure. If a security researcher shows up and says "your stuff is broken," we are not going to get away with it "no it isn't, because we discussed it on a Friday call with a closed group of people and decided it was OK." Our reasoning is going to have to be documented. That doesn't guarantee that it will be correct, but makes it possible to distinguish between defects in design, defects in particular parts of the code, and non-defects, which is otherwise impossible. Meanwhile, even if security researches are as happy with our TDE implementation as they could possibly be, a feature that changes the on-disk format can't erase our ability to solve other problems with the database. Databases using TDE are still going to have corruption, for example, but now a corrupted page has a good chance of being completely unreadable rather than just garbled. You certainly aren't going to be able to just run pg_filedump on it. I think even if we do a great job explaining to everybody what impact TDE and its preparatory patches are likely to have on the system, there's likely to be a lot of cases where users blame the database for eating their data when the real culprit is the OS or the hardware, just because such cases are bound to get harder to investigate, which could have a very negative effect on the perceptions of PostgreSQL's quality. But if the TDE itself is magic that only designated smart people on special calls can understand, then it's going to be far worse, because that'll mean when any kind of TDE problems comes up, nobody else can help debug anything. While I would like to have TDE in PostgreSQL, I would not like to have it on those terms. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Key management with tests
Hi, On 2021-01-17 11:54:57 +0530, Amit Kapila wrote: > On Sun, Jan 17, 2021 at 5:38 AM Tom Kincaid wrote: > > Admittedly I am a novice on this topic, and the majority of the > > PostgreSQL source code, however I am hopeful enough (those of you who > > know me understand that I suffer from eternal optimism) that I am > > going to attempt to help. > > > > Is there a design document for a Postgres feature of this size and > > scope that people feel would serve as a good example? Alternatively, > > is there a design document template that has been successfully used in > > the past? > > > > We normally write the design considerations and choices we made with > the reasons in README and code comments. Personally, I am not sure if > there is a need for any specific document per-se but a README and > detailed comments in the code should suffice what people are worried > about here. Right. It could be a README file, or a long comment at a start of one of the files. It doesn't matter too much. What matters is that people that haven't been on those phone call can understand the design and the implications it has. > It is mostly from the perspective that other developers > reading the code, want to do bug-fix, or later enhance that code > should be able to understand it. I'd add the perspective of code reviewers as well. > One recent example I can give is > Peter's work on bottom-up deletion [1] which I have read today where I > find that the design is captured via README, appropriate comments in > the code, and documentation. This feature is quite different and > probably a lot more new concepts are being introduced but I hope that > will give you some clue. > > [1] - > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d168b666823b6e0bcf60ed19ce24fb5fb91b8ccf This is a great example. Greetings, Andres Freund
Re: Key management with tests
On Sun, Jan 17, 2021 at 5:38 AM Tom Kincaid wrote: > > > > > I think that's not at all acceptable. I don't mind hashing out details > > > > on calls / off-list, but the design needs to be public, documented, and > > > > reviewable. And if it's something the community can't understand, then > > > > it can't get in. We're going to have to maintain this going forward. > > > > > > OK, so we don't want it. That's fine with me. > > > > That's not what I said... > > > > > I think the majority of us believe that it is important we take this > first step towards a solid TDE implementation in PostgreSQL that is > built around the community processes which involves general consensus. > > Before this feature falls into the “we will never do it because we > will never build consensus" category and community PostgreSQL > potentially gets locked out of more deployment scenarios that require > this feature I would like to see if I can help with this current > attempt at it. I will share that I am concerned that if the people who > have been involved in this to date can’t get this in, it will never > happen. > > Admittedly I am a novice on this topic, and the majority of the > PostgreSQL source code, however I am hopeful enough (those of you who > know me understand that I suffer from eternal optimism) that I am > going to attempt to help. > > Is there a design document for a Postgres feature of this size and > scope that people feel would serve as a good example? Alternatively, > is there a design document template that has been successfully used in > the past? > We normally write the design considerations and choices we made with the reasons in README and code comments. Personally, I am not sure if there is a need for any specific document per-se but a README and detailed comments in the code should suffice what people are worried about here. It is mostly from the perspective that other developers reading the code, want to do bug-fix, or later enhance that code should be able to understand it. One recent example I can give is Peter's work on bottom-up deletion [1] which I have read today where I find that the design is captured via README, appropriate comments in the code, and documentation. This feature is quite different and probably a lot more new concepts are being introduced but I hope that will give you some clue. [1] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d168b666823b6e0bcf60ed19ce24fb5fb91b8ccf -- With Regards, Amit Kapila.
Re: Key management with tests
> > > I think that's not at all acceptable. I don't mind hashing out details > > > on calls / off-list, but the design needs to be public, documented, and > > > reviewable. And if it's something the community can't understand, then > > > it can't get in. We're going to have to maintain this going forward. > > > > OK, so we don't want it. That's fine with me. > > That's not what I said... > I think the majority of us believe that it is important we take this first step towards a solid TDE implementation in PostgreSQL that is built around the community processes which involves general consensus. Before this feature falls into the “we will never do it because we will never build consensus" category and community PostgreSQL potentially gets locked out of more deployment scenarios that require this feature I would like to see if I can help with this current attempt at it. I will share that I am concerned that if the people who have been involved in this to date can’t get this in, it will never happen. Admittedly I am a novice on this topic, and the majority of the PostgreSQL source code, however I am hopeful enough (those of you who know me understand that I suffer from eternal optimism) that I am going to attempt to help. Is there a design document for a Postgres feature of this size and scope that people feel would serve as a good example? Alternatively, is there a design document template that has been successfully used in the past? I could guess based on things I have observed reading this list for many years. However, if there is something that those who are deeply involved in the development effort feel would suffice as an example of a "good design document" or a "good design template" sharing it would be greatly appreciated.
Re: Key management with tests
On Fri, Jan 15, 2021 at 08:20:36PM -0800, Andres Freund wrote: > On 2021-01-15 20:49:10 -0500, Bruce Momjian wrote: >> What Perl tap tests run initdb and manage the cluster? I didn't find >> any. > > find . -name '*.pl'|xargs grep 'use PostgresNode;' > > should give you a nearly complete list. Just to add that all the perl modules we use for the tests are within src/test/perl/. The coolest tests are within src/bin/ and src/test/. -- Michael signature.asc Description: PGP signature
Re: Key management with tests
Hi, On 2021-01-15 20:49:10 -0500, Bruce Momjian wrote: > On Fri, Jan 15, 2021 at 04:56:24PM -0800, Andres Freund wrote: > > On 2021-01-15 19:21:32 -0500, Bruce Momjian wrote: > > > You have to understand cryptography and Postgres internals to understand > > > the design, and I don't think it is realistic to explain that all to the > > > community. We did much of this in voice calls over months because it > > > was too much of a burden to explain all the cryptographic details so > > > everyone could follow along. > > > > I think that's not at all acceptable. I don't mind hashing out details > > on calls / off-list, but the design needs to be public, documented, and > > reviewable. And if it's something the community can't understand, then > > it can't get in. We're going to have to maintain this going forward. > > OK, so we don't want it. That's fine with me. That's not what I said... > > This isn't specific to this topic? I don't really understand why this > > specific feature gets to avoid normal community development processes? > > What is being avoided? You previously pushed a patch without tests, now you want to push a patch that was barely reviewed and also doesn't contain an explanation of the design. I mean: > > > You have to understand cryptography and Postgres internals to understand > > > the design, and I don't think it is realistic to explain that all to the > > > community. We did much of this in voice calls over months because it > > > was too much of a burden to explain all the cryptographic details so > > > everyone could follow along. really is very far from the normal community process. Again, how is this supposed to be maintained in the future, if it's based on a design that's only understandable to the people on those phone calls? > > We have had perl tap tests for quite a while now? And all new tests that > > aren't regression / isolation tests are expected to be written in it. > > What Perl tap tests run initdb and manage the cluster? I didn't find > any. find . -name '*.pl'|xargs grep 'use PostgresNode;' should give you a nearly complete list. Greetings, Andres Freund
Re: Key management with tests
On Fri, Jan 15, 2021 at 04:56:24PM -0800, Andres Freund wrote: > On 2021-01-15 19:21:32 -0500, Bruce Momjian wrote: > > You have to understand cryptography and Postgres internals to understand > > the design, and I don't think it is realistic to explain that all to the > > community. We did much of this in voice calls over months because it > > was too much of a burden to explain all the cryptographic details so > > everyone could follow along. > > I think that's not at all acceptable. I don't mind hashing out details > on calls / off-list, but the design needs to be public, documented, and > reviewable. And if it's something the community can't understand, then > it can't get in. We're going to have to maintain this going forward. OK, so we don't want it. That's fine with me. > I don't mean to say that we need to re-hash all design details from > scratch - but that there needs to be an explanation somewhere that > describes what's being done on a medium-high level, and what drove those > design decisions. I thought the TODO list was that, and the email threads. > > > The wiki page doesn't really describe a design either. It has a very > > > long todo, a bunch of implementation details, but no design. > > > > I am not sure what design document you are requesting. I thought the > > TODO was that. > > The TODO in > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements > is a design document? Yes. > > > Nor did 978f869b99 include much in the way of design description. > > > > > > You cannot expect anybody to review a patch if developing some basic > > > understanding of the intended design requires reading hundreds of > > > messages in which the design evolved. And I don't think it's acceptable > > > to push it due to lack of further feedback, given this situation - the > > > lack of design description is a blocker in itself. > > > > OK, I will just move on to something else then. It is not worth the > > feature to go into that kind of discussion again. I am willing to have > > voice calls with individuals to explain the logic, but repeatedly > > explaining it to the entire group I find unproductive. I don't think > > another 400-email thread would help anyone. > > Explaining something over voice doesn't help with people in a year or > five trying to understand the code and the design, so they can adapt it > when making half-related changes. Nor do I see why another 400 email > thread would be a necessary consequence of you explaining the design > that you came up with. I have underestimated the amount of discussion this has required repeatedly, and I don't want to make that mistake again. > This isn't specific to this topic? I don't really understand why this > specific feature gets to avoid normal community development processes? What is being avoided? > > > - tests: > > > - wait, a .sh test script? No, we shouldn't add any more of those, > > > they're nightmare across platforms > > > > The script originatad from pg_upgrade. I don't know how to do things > > like initdb and stuff another way, at least in our code. > > We have had perl tap tests for quite a while now? And all new tests that > aren't regression / isolation tests are expected to be written in it. What Perl tap tests run initdb and manage the cluster? I didn't find any. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Hi, On 2021-01-15 19:21:32 -0500, Bruce Momjian wrote: > On Fri, Jan 15, 2021 at 02:37:56PM -0800, Andres Freund wrote: > > On 2021-01-15 16:47:19 -0500, Bruce Momjian wrote: > > > > I am not even sure there is a consensus on the design, without which > > > > any commit is always premature. > > > > > > If people want changes, I need to hear about it here. I have address > > > everything people have mentioned in these threads so far. > > > > I don't even know how anybody is supposed to realistically review the > > design or the patch: > > > > This thread started at > > https://postgr.es/m/20210101045047.GB30966%40momjian.us - there's no > > reference to any discussion of the design at all and the supposed links > > to code are dead. > > You have to understand cryptography and Postgres internals to understand > the design, and I don't think it is realistic to explain that all to the > community. We did much of this in voice calls over months because it > was too much of a burden to explain all the cryptographic details so > everyone could follow along. I think that's not at all acceptable. I don't mind hashing out details on calls / off-list, but the design needs to be public, documented, and reviewable. And if it's something the community can't understand, then it can't get in. We're going to have to maintain this going forward. I don't mean to say that we need to re-hash all design details from scratch - but that there needs to be an explanation somewhere that describes what's being done on a medium-high level, and what drove those design decisions. > > The last version of the code that I see posted ([1]), has the useless > > commit message of "key squash commit" - nothing else. There's no design > > documentation included in the patch either, as far as I can tell. > > > > Manually searching for the topic brings me to > > https://www.postgresql.org/message-id/20201202213814.GG20285%40momjian.us > > , a thread of 52 messages, which provides a bit more context, but > > largely just references another thread and a wiki article. The link to > > the other thread is into the middle of a 112 message thread. > > > > The wiki page doesn't really describe a design either. It has a very > > long todo, a bunch of implementation details, but no design. > > I am not sure what design document you are requesting. I thought the > TODO was that. The TODO in https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements is a design document? > > Nor did 978f869b99 include much in the way of design description. > > > > You cannot expect anybody to review a patch if developing some basic > > understanding of the intended design requires reading hundreds of > > messages in which the design evolved. And I don't think it's acceptable > > to push it due to lack of further feedback, given this situation - the > > lack of design description is a blocker in itself. > > OK, I will just move on to something else then. It is not worth the > feature to go into that kind of discussion again. I am willing to have > voice calls with individuals to explain the logic, but repeatedly > explaining it to the entire group I find unproductive. I don't think > another 400-email thread would help anyone. Explaining something over voice doesn't help with people in a year or five trying to understand the code and the design, so they can adapt it when making half-related changes. Nor do I see why another 400 email thread would be a necessary consequence of you explaining the design that you came up with. This isn't specific to this topic? I don't really understand why this specific feature gets to avoid normal community development processes? > > - tests: > > - wait, a .sh test script? No, we shouldn't add any more of those, > > they're nightmare across platforms > > The script originatad from pg_upgrade. I don't know how to do things > like initdb and stuff another way, at least in our code. We have had perl tap tests for quite a while now? And all new tests that aren't regression / isolation tests are expected to be written in it. Greetings, Andres Freund
Re: Key management with tests
Hi, On 2021-01-15 16:47:19 -0500, Bruce Momjian wrote: > On Fri, Jan 15, 2021 at 04:23:22PM -0500, Robert Haas wrote: > > On Fri, Jan 15, 2021 at 3:49 PM Bruce Momjian wrote: > > I don't think that's appropriate. Several prominent community members > > have told you that the patch, as committed the first time, needed a > > lot more work. There hasn't been enough time between then and now for > > you, or anyone, to do that amount of work. This patch needs detailed > > and substantial review from senior community members, and multiple > > rounds of feedback and improvement, before it should be considered for > > commit. > > > > I am not even sure there is a consensus on the design, without which > > any commit is always premature. > > If people want changes, I need to hear about it here. I have address > everything people have mentioned in these threads so far. I don't even know how anybody is supposed to realistically review the design or the patch: This thread started at https://postgr.es/m/20210101045047.GB30966%40momjian.us - there's no reference to any discussion of the design at all and the supposed links to code are dead. The last version of the code that I see posted ([1]), has the useless commit message of "key squash commit" - nothing else. There's no design documentation included in the patch either, as far as I can tell. Manually searching for the topic brings me to https://www.postgresql.org/message-id/20201202213814.GG20285%40momjian.us , a thread of 52 messages, which provides a bit more context, but largely just references another thread and a wiki article. The link to the other thread is into the middle of a 112 message thread. The wiki page doesn't really describe a design either. It has a very long todo, a bunch of implementation details, but no design. Nor did 978f869b99 include much in the way of design description. You cannot expect anybody to review a patch if developing some basic understanding of the intended design requires reading hundreds of messages in which the design evolved. And I don't think it's acceptable to push it due to lack of further feedback, given this situation - the lack of design description is a blocker in itself. There's a few things that stand out on a very very brief scan: - the patch badly needs to be split up into independently reviewable pieces - tests: - wait, a .sh test script? No, we shouldn't add any more of those, they're nightmare across platforms - Do the tests actually do anything useful? It's not clear to me what they are trying to achieve. En/Decrypting test vectors doesn't seem to buy that much? - the new pg_alterckey is completely untested - the pg_upgrade paths is untested - .. - Without further comment BootStrapKmgr() does "copy cluster file encryption keys from an old cluster?", but there's no explanation as to why / when that's the case. Presumably pg_upgrade, but, uh, explain that. - pg_alterckey.c - appears to create it's own cluster lock file, using its own routine for doing so. How does that lock file interact with the running server? - retrieve_cluster_keys() is missing (void). I think this is at the very least a month away from being committable, even if the design were completely correct (which I do not know, see above). Greetings, Andres Freund [1] https://www.postgresql.org/message-id/20210115204926.GD8740%40momjian.us
Re: Key management with tests
On Fri, Jan 15, 2021 at 04:59:17PM -0500, Robert Haas wrote: > On Fri, Jan 15, 2021 at 4:47 PM Bruce Momjian wrote: > > If people want changes, I need to hear about it here. I have address > > everything people have mentioned in these threads so far. > > That does not match my perception of the situation. Well, that's not very specific, is it? You might be confusing the POC data encryption patch that was posted in this thread with the key management patch that I am working on. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Fri, Jan 15, 2021 at 2:59 PM Robert Haas wrote: > On Fri, Jan 15, 2021 at 4:47 PM Bruce Momjian wrote: > > If people want changes, I need to hear about it here. I have address > > everything people have mentioned in these threads so far. > > That does not match my perception of the situation. > > Looking at the Commitfest there are three authors and no reviewers. Given the previous incident at minimum each of the people in the Commitfest should add their approval to commit this patch to this thread. And while committers get some leeway, in this case having a non-author review and sign-off on it being ready-to-commit seems like it should be required. David J.
Re: Key management with tests
On Fri, Jan 15, 2021 at 4:47 PM Bruce Momjian wrote: > If people want changes, I need to hear about it here. I have address > everything people have mentioned in these threads so far. That does not match my perception of the situation. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Key management with tests
On Fri, Jan 15, 2021 at 04:23:22PM -0500, Robert Haas wrote: > On Fri, Jan 15, 2021 at 3:49 PM Bruce Momjian wrote: > > I am planning to apply this next week. > > I don't think that's appropriate. Several prominent community members > have told you that the patch, as committed the first time, needed a > lot more work. There hasn't been enough time between then and now for > you, or anyone, to do that amount of work. This patch needs detailed > and substantial review from senior community members, and multiple > rounds of feedback and improvement, before it should be considered for > commit. > > I am not even sure there is a consensus on the design, without which > any commit is always premature. If people want changes, I need to hear about it here. I have address everything people have mentioned in these threads so far. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Fri, Jan 15, 2021 at 3:49 PM Bruce Momjian wrote: > I am planning to apply this next week. I don't think that's appropriate. Several prominent community members have told you that the patch, as committed the first time, needed a lot more work. There hasn't been enough time between then and now for you, or anyone, to do that amount of work. This patch needs detailed and substantial review from senior community members, and multiple rounds of feedback and improvement, before it should be considered for commit. I am not even sure there is a consensus on the design, without which any commit is always premature. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Key management with tests
On Tue, Jan 12, 2021 at 01:46:53PM -0500, Bruce Momjian wrote: > On Tue, Jan 12, 2021 at 01:15:44PM -0500, Bruce Momjian wrote: > > Well, we have eight unused bits in the IV, so we could just increment > > that for every hint bit change that uses the same LSN, and then force a > > dummy WAL record when that 8-bit counter overflows --- that seems > > simpler than logging hint bits. > > Sorry, I was incorrect. The IV is 16 bytes, made up of the LSN (8 > bytes), and the page number (4 bytes). That leaves 4 bytes unused or > 2^32 values for hint bit changes before we have to generate a dummy LSN > record. I just did a massive update to the Transparent Data Encryption wiki page to make it more readable and updated it with current decisions: https://wiki.postgresql.org/wiki/Transparent_Data_Encryption -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Thank you for your reply， On Wed, Jan 13, 2021 at 12:08 AM Stephen Frost wrote: > > No, we can't 'modify the page format as we wish'- if we change away from > using a C structure then we're going to be modifying quite a bit of > code which otherwise doesn't need to be changed. The proposed flag > doesn't actually make a different page format work, the only thing it > would do would be to allow some parts of the cluster to be encrypted and > other parts not be, but I don't know that that's actually a useful > capability or a good reason to use one of those bits. Having it handled > on a cluster level, at initdb time through pg_control, seems like it'd > work just fine. > > Yes, I realized that for cluster-level encryption, it would be unwise to flag a single page(Unless we want to do it at relation-level). Forgive me for not describing clearly, the 'modify the page' I said means the method you mentioned, not modifying the C structure. My original motivation is to avoid storing in an unconventional format without a description of the C structure. However, as I just said, it seems that we should not set the flag for a single page. Maybe it's enough to just add a comment description?
Re: Key management with tests
Hi,
On 2021-01-11 20:12:00 +0900, Masahiko Sawada wrote:
> diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
> index 32b5d62e1f..d474af753c 100644
> --- a/contrib/bloom/blinsert.c
> +++ b/contrib/bloom/blinsert.c
> @@ -177,6 +177,7 @@ blbuildempty(Relation index)
>* XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record. Therefore, we need
>* this even when wal_level=minimal.
>*/
> + PageEncryptInplace(metapage, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO);
> PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
> smgrwrite(index->rd_smgr, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
> (char *) metapage, true);
There's quite a few places doing encryption + checksum + smgwrite now. I
strongly suggest splitting that off into a helper routine in a
preparatory patch.
> @@ -528,6 +529,8 @@ BootstrapModeMain(void)
>
> InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
>
> + InitializeBufferEncryption();
> +
> /* Initialize stuff for bootstrap-file processing */
> for (i = 0; i < MAXATTR; i++)
> {
Why are we initializing this here instead of postmaster? As far as I can
tell that just leads to redundant work instead of doing it once?
> +/*-
> + * We use both page LSN and page number to create a nonce for each page. Page
> + * LSN is 8 byte, page number is 4 byte, and the maximum required counter for
> + * AES-CTR is 2048, which fits in 3 byte. Since the length of IV is 16 byte
> + * it's fine. Using the LSN and page number as part of the nonce has
> + * three benefits:
> + *
> + * 1. We don't need to decrypt/re-encrypt during CREATE DATABASE since the
> page
> + * contents are the same in both places, and once one database changes its
> pages,
> + * it gets a new LSN, and hence a new nonce.
> + * 2. For each change of an 8k page, we get a new nonce, so we are not
> encrypting
> + * different data with the same nonce/IV.
> + * 3. We avoid requiring pg_upgrade to preserve database oids, tablespace
> oids,
> + * relfilenodes.
I think 3) also has a few minor downsides - by not including information
identifying a relation a potential attacker with access to the data
directory has more chances to get the database to decrypt data by
e.g. switching relation files around.
> @@ -2792,12 +2793,15 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
>*/
> bufBlock = BufHdrGetBlock(buf);
>
> + bufToWrite = PageEncryptCopy((Page) bufBlock, buf->tag.forkNum,
> +
> buf->tag.blockNum);
> +
> /*
>* Update page checksum if desired. Since we have only shared lock on
> the
>* buffer, other processes might be updating hint bits in it, so we must
>* copy the page to private storage if we do checksumming.
>*/
> - bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum);
> + bufToWrite = PageSetChecksumCopy((Page) bufToWrite, buf->tag.blockNum);
>
> if (track_io_timing)
> INSTR_TIME_SET_CURRENT(io_start);
So now we copy the page twice, not just once, if both checksums and
encryption is enabled? That doesn't seem right.
> @@ -3677,6 +3683,21 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
> {
> dirtied = true; /* Means "will be dirtied by
> this action" */
>
> + /*
> + * We will dirty the page but the page lsn is not
> changed if we
> + * doesn't write a backup block. We don't want to
> encrypt the
> + * different bits stream with the same combination of
> nonce and key
> + * since in buffer encryption the page lsn is a part of
> nonce.
> + * Therefore we WAL-log no-op record just to move page
> lsn forward if
> + * we doesn't write a backup block, even when this is
> not the first
> + * modification in this checkpoint round.
> + */
> + if (XLogRecPtrIsInvalid(lsn) && DataEncryptionEnabled())
> + {
> + lsn = log_noop();
> + Assert(!XLogRecPtrIsInvalid(lsn));
> + }
> +
Aren't you doing a WAL record while holding the buffer header lock here?
You can't do things like WAL insertions while holding a spinlock.
I don't see how it is safe / correct to use a noop record here. A noop
record isn't associated with the page, so WAL replay isn't going to
perform the same LSN modification.
Also, why is it OK to modify the LSN without, if necessary, logging an FPI?
> +char *
> +PageEncryptCopy(Page page, ForkNumber forknum, BlockNumber blkno)
> +{
> + static char *pageCopy = NULL;
> +
> + /* If we don't need a checksum, just return the
Re: Key management with tests
On Tue, Jan 12, 2021 at 01:57:11PM -0500, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Tue, Jan 12, 2021 at 01:44:05PM -0500, Stephen Frost wrote: > > > * Bruce Momjian (br...@momjian.us) wrote: > > > > Well, we have eight unused bits in the IV, so we could just increment > > > > that for every hint bit change that uses the same LSN, and then force a > > > > dummy WAL record when that 8-bit counter overflows --- that seems > > > > simpler than logging hint bits. > > > > > > Sure, as long as we have a place to store that information.. We need to > > > have the full IV available when we go to decrypt the page. > > > > Oh, yeah, we would need that counter recorded since previously the IV > > was made up of already-recorded information, i.e., the page LSN and page > > number. However, the reason don't WAL-log hint bits always is because > > we can afford to lose them, but in this case, any counter we need to > > store will need to be WAL logged since we can't affort to lose that > > counter value for decryption --- that gets us back to WAL-logging > > something during hint bit changes. :-( > > I don't think that's actually the case..? The hole I'm talking about is > there exclusively for post-encryption storage of the tag and maybe this > part of the IV and would be zero'd out in the FPIs that actually go into > the WAL (which would be encrypted with the WAL key, not the data key). > All we would need to be confident of is that if the page with the hint > bit update gets encrypted and written out that the IV counter gets > incremented and also written out as part of that write. OK, got it. I have added this to the wiki: https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements wal_log_hints will be enabled automatically in encryption mode. However, more than one hit change between checkpoints does not cause WAL activity, which would cause the same LSN to be used for different page images. This means we need a page-stored counter, to be used in the four unused bytes of the IV. This prevents multiple page writes during the same checkpoint interval from using the same IV. Counter changes do not need to be WAL logged since we either get the page from the WAL (which is only encrypted with the WAL data key), or from disk, which is durable. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Jan 12, 2021 at 01:44:05PM -0500, Stephen Frost wrote: > > * Bruce Momjian (br...@momjian.us) wrote: > > > Well, we have eight unused bits in the IV, so we could just increment > > > that for every hint bit change that uses the same LSN, and then force a > > > dummy WAL record when that 8-bit counter overflows --- that seems > > > simpler than logging hint bits. > > > > Sure, as long as we have a place to store that information.. We need to > > have the full IV available when we go to decrypt the page. > > Oh, yeah, we would need that counter recorded since previously the IV > was made up of already-recorded information, i.e., the page LSN and page > number. However, the reason don't WAL-log hint bits always is because > we can afford to lose them, but in this case, any counter we need to > store will need to be WAL logged since we can't affort to lose that > counter value for decryption --- that gets us back to WAL-logging > something during hint bit changes. :-( I don't think that's actually the case..? The hole I'm talking about is there exclusively for post-encryption storage of the tag and maybe this part of the IV and would be zero'd out in the FPIs that actually go into the WAL (which would be encrypted with the WAL key, not the data key). All we would need to be confident of is that if the page with the hint bit update gets encrypted and written out that the IV counter gets incremented and also written out as part of that write. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Tue, Jan 12, 2021 at 01:44:05PM -0500, Stephen Frost wrote: > * Bruce Momjian (br...@momjian.us) wrote: > > Well, we have eight unused bits in the IV, so we could just increment > > that for every hint bit change that uses the same LSN, and then force a > > dummy WAL record when that 8-bit counter overflows --- that seems > > simpler than logging hint bits. > > Sure, as long as we have a place to store that information.. We need to > have the full IV available when we go to decrypt the page. Oh, yeah, we would need that counter recorded since previously the IV was made up of already-recorded information, i.e., the page LSN and page number. However, the reason don't WAL-log hint bits always is because we can afford to lose them, but in this case, any counter we need to store will need to be WAL logged since we can't affort to lose that counter value for decryption --- that gets us back to WAL-logging something during hint bit changes. :-( -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Tue, Jan 12, 2021 at 01:15:44PM -0500, Bruce Momjian wrote: > On Tue, Jan 12, 2021 at 01:11:29PM -0500, Stephen Frost wrote: > > I don't think there's any doubt that we need to make sure that the IV is > > distinct and advancing the LSN to get a new one when needed for this > > case seems like it's probably the way to do that. Hint bit change > > visibility to users isn't really at issue here- we can't use the same IV > > multiple times. The two options that we have are to either not actually > > update the hint bit in such a case, or to make sure to change the > > LSN/IV. Another option would be to, if we're able to make a hole to put > > the GCM tag on to the page somewhere, further widen that hole to include > > an additional space for a counter that would be mixed into the IV, to > > avoid having to do an XLOG NOOP. > > Well, we have eight unused bits in the IV, so we could just increment > that for every hint bit change that uses the same LSN, and then force a > dummy WAL record when that 8-bit counter overflows --- that seems > simpler than logging hint bits. Sorry, I was incorrect. The IV is 16 bytes, made up of the LSN (8 bytes), and the page number (4 bytes). That leaves 4 bytes unused or 2^32 values for hint bit changes before we have to generate a dummy LSN record. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Jan 12, 2021 at 01:11:29PM -0500, Stephen Frost wrote: > > > I think one big question is that, since we are using a streaming cipher, > > > do we care about hint bit changes showing to users? I actually don't > > > know. If we do, some kind of dummy LSN record might be required, as you > > > suggested. > > > > I don't think there's any doubt that we need to make sure that the IV is > > distinct and advancing the LSN to get a new one when needed for this > > case seems like it's probably the way to do that. Hint bit change > > visibility to users isn't really at issue here- we can't use the same IV > > multiple times. The two options that we have are to either not actually > > update the hint bit in such a case, or to make sure to change the > > LSN/IV. Another option would be to, if we're able to make a hole to put > > the GCM tag on to the page somewhere, further widen that hole to include > > an additional space for a counter that would be mixed into the IV, to > > avoid having to do an XLOG NOOP. > > Well, we have eight unused bits in the IV, so we could just increment > that for every hint bit change that uses the same LSN, and then force a > dummy WAL record when that 8-bit counter overflows --- that seems > simpler than logging hint bits. Sure, as long as we have a place to store that information.. We need to have the full IV available when we go to decrypt the page. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Tue, Jan 12, 2021 at 01:11:29PM -0500, Stephen Frost wrote: > > I think one big question is that, since we are using a streaming cipher, > > do we care about hint bit changes showing to users? I actually don't > > know. If we do, some kind of dummy LSN record might be required, as you > > suggested. > > I don't think there's any doubt that we need to make sure that the IV is > distinct and advancing the LSN to get a new one when needed for this > case seems like it's probably the way to do that. Hint bit change > visibility to users isn't really at issue here- we can't use the same IV > multiple times. The two options that we have are to either not actually > update the hint bit in such a case, or to make sure to change the > LSN/IV. Another option would be to, if we're able to make a hole to put > the GCM tag on to the page somewhere, further widen that hole to include > an additional space for a counter that would be mixed into the IV, to > avoid having to do an XLOG NOOP. Well, we have eight unused bits in the IV, so we could just increment that for every hint bit change that uses the same LSN, and then force a dummy WAL record when that 8-bit counter overflows --- that seems simpler than logging hint bits. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Jan 12, 2021 at 09:40:53PM +0900, Masahiko Sawada wrote: > > > This says: > > > > > > > > > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements > > > > > > wal_log_hints will be enabled automatically in encryption mode. > > > > > > Does that help? > > > > IIUC it helps but not enough. When wal_log_hints is enabled, we write > > a full-page image when updating hint bits if it's the first time > > change for the page since the last checkpoint. But I'm concerned that > > what if we change hint bits again after the page is flushed. We would > > mark the page as dirtied but not write any WAL, leaving the page lsn > > as it is. > > I updated the wiki to be: > > > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements > > wal_log_hints will be enabled automatically in encryption mode. However, > more than one hit change between checkpoints does not cause WAL > activity, which would cause the same LSN to be used for different pages > images. > > I think one big question is that, since we are using a streaming cipher, > do we care about hint bit changes showing to users? I actually don't > know. If we do, some kind of dummy LSN record might be required, as you > suggested. I don't think there's any doubt that we need to make sure that the IV is distinct and advancing the LSN to get a new one when needed for this case seems like it's probably the way to do that. Hint bit change visibility to users isn't really at issue here- we can't use the same IV multiple times. The two options that we have are to either not actually update the hint bit in such a case, or to make sure to change the LSN/IV. Another option would be to, if we're able to make a hole to put the GCM tag on to the page somewhere, further widen that hole to include an additional space for a counter that would be mixed into the IV, to avoid having to do an XLOG NOOP. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On 2021-01-12 13:03:14 -0500, Bruce Momjian wrote: > I think one big question is that, since we are using a streaming cipher, > do we care about hint bit changes showing to users? I actually don't > know. If we do, some kind of dummy LSN record might be required, as you > suggested. That'd lead to a *massive* increase of WAL record volume. It's one thing to WAL log hint bit writes once per page per checkpoint. It's another to do so on every single hint bit write.
Re: Key management with tests
On Tue, Jan 12, 2021 at 09:40:53PM +0900, Masahiko Sawada wrote: > > This says: > > > > > > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements > > > > wal_log_hints will be enabled automatically in encryption mode. > > > > Does that help? > > IIUC it helps but not enough. When wal_log_hints is enabled, we write > a full-page image when updating hint bits if it's the first time > change for the page since the last checkpoint. But I'm concerned that > what if we change hint bits again after the page is flushed. We would > mark the page as dirtied but not write any WAL, leaving the page lsn > as it is. I updated the wiki to be: https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements wal_log_hints will be enabled automatically in encryption mode. However, more than one hit change between checkpoints does not cause WAL activity, which would cause the same LSN to be used for different pages images. I think one big question is that, since we are using a streaming cipher, do we care about hint bit changes showing to users? I actually don't know. If we do, some kind of dummy LSN record might be required, as you suggested. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Neil Chen (carpenter.nail...@gmail.com) wrote: > On Tue, Jan 12, 2021 at 10:47 AM Stephen Frost wrote: > > This is an interesting question but ultimately I don't think we should > > be looking at this from the perspective of allowing arbitrary changes to > > the page format. The challenge is that much of the page format, today, > > is defined by a C struct and changing the way that works would require a > > great deal of code to be modified and turn this into a massive effort, > > assuming we wish to have the same compiled binary able to work with both > > unencrypted and encrypted clusters, which I do believe is a requirement. > > > > The thought that I had was to, instead, try to figure out if we could > > fudge some space by, say, putting a 128-bit 'hole' at the end of the > > page and just move pd_special back, effectively making the page seem > > 'smaller' to all of the code that uses it, except for the code that > > knows how to do the decryption. I ran into some trouble with that but > > haven't quite sorted out what happened yet. Other ideas would be to put > > it before pd_special, or maybe somewhere else, but a lot depends on the > > code's expectations. > > I agree that we should not make too many changes to affect the use of > unencrypted clusters. But as a personal opinion only, I don't think it's a > good idea to add some "implicit" tricks. To provide an inspiration, can we > add a flag to mark whether the page format has been changed: Sure, of course we could add such a flag, but I don't see how that would actually help with the issue? > In this way, I think it has little effect on the unencrypted cluster, and > we can also modify the page format as we wish. Of course, it's also > possible that I didn't understand your design correctly, or there's > something wrong with my idea. :D No, we can't 'modify the page format as we wish'- if we change away from using a C structure then we're going to be modifying quite a bit of code which otherwise doesn't need to be changed. The proposed flag doesn't actually make a different page format work, the only thing it would do would be to allow some parts of the cluster to be encrypted and other parts not be, but I don't know that that's actually a useful capability or a good reason to use one of those bits. Having it handled on a cluster level, at initdb time through pg_control, seems like it'd work just fine. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Tue, Jan 12, 2021 at 11:09 AM Bruce Momjian wrote: > > On Tue, Jan 12, 2021 at 09:32:54AM +0900, Masahiko Sawada wrote: > > On Tue, Jan 12, 2021 at 3:23 AM Stephen Frost wrote: > > > Right, or ensure that the actual IV used is distinct (such as by using > > > another bit in the IV to distinguish logged-vs-unlogged), but it seems > > > saner to just use a different key, ultimately. > > > > Agreed. > > > > I think we also need to consider how to make sure nonce is unique when > > making a page dirty by updating hint bits. Hint bit update changes the > > page contents but doesn't change the page lsn if we already write a > > full page write. In the PoC patch, I logged a dummy WAL record > > (XLOG_NOOP) just to move the page lsn forward, but since this is > > required even when changing the page is not the first time since the > > last checkpoint we might end up logging too many dummy WAL records. > > This says: > > > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements > > wal_log_hints will be enabled automatically in encryption mode. > > Does that help? IIUC it helps but not enough. When wal_log_hints is enabled, we write a full-page image when updating hint bits if it's the first time change for the page since the last checkpoint. But I'm concerned that what if we change hint bits again after the page is flushed. We would mark the page as dirtied but not write any WAL, leaving the page lsn as it is. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
Re: Key management with tests
Hi Stephen, On Tue, Jan 12, 2021 at 10:47 AM Stephen Frost wrote: > > This is an interesting question but ultimately I don't think we should > be looking at this from the perspective of allowing arbitrary changes to > the page format. The challenge is that much of the page format, today, > is defined by a C struct and changing the way that works would require a > great deal of code to be modified and turn this into a massive effort, > assuming we wish to have the same compiled binary able to work with both > unencrypted and encrypted clusters, which I do believe is a requirement. > > The thought that I had was to, instead, try to figure out if we could > fudge some space by, say, putting a 128-bit 'hole' at the end of the > page and just move pd_special back, effectively making the page seem > 'smaller' to all of the code that uses it, except for the code that > knows how to do the decryption. I ran into some trouble with that but > haven't quite sorted out what happened yet. Other ideas would be to put > it before pd_special, or maybe somewhere else, but a lot depends on the > code's expectations. > > I agree that we should not make too many changes to affect the use of unencrypted clusters. But as a personal opinion only, I don't think it's a good idea to add some "implicit" tricks. To provide an inspiration, can we add a flag to mark whether the page format has been changed: --- a/src/include/storage/bufpage.h +++ b/src/include/storage/bufpage.h @@ -181,8 +185,9 @@ typedef PageHeaderData *PageHeader; #define PD_PAGE_FULL 0x0002 /* not enough free space for new tuple? */ #define PD_ALL_VISIBLE 0x0004 /* all tuples on page are visible to * everyone */ +#define PD_PAGE_ENCRYPTED 0x0008 /* Is page encrypted? */ -#define PD_VALID_FLAG_BITS 0x0007 /* OR of all valid pd_flags bits */ +#define PD_VALID_FLAG_BITS 0x000F /* OR of all valid pd_flags bits */ /* * Page layout version number 0 is for pre-7.3 Postgres releases. @@ -389,6 +394,13 @@ PageValidateSpecialPointer(Page page) #define PageClearAllVisible(page) \ (((PageHeader) (page))->pd_flags &= ~PD_ALL_VISIBLE) +#define PageIsEncrypted(page) \ + (((PageHeader) (page))->pd_flags & PD_PAGE_ENCRYPTED) +#define PageSetEncrypted(page) \ + (((PageHeader) (page))->pd_flags |= PD_PAGE_ENCRYPTED) +#define PageClearEncrypted(page) \ + (((PageHeader) (page))->pd_flags &= ~PD_PAGE_ENCRYPTED) + #define PageIsPrunable(page, oldestxmin) \ ( \ AssertMacro(TransactionIdIsNormal(oldestxmin)), \ In this way, I think it has little effect on the unencrypted cluster, and we can also modify the page format as we wish. Of course, it's also possible that I didn't understand your design correctly, or there's something wrong with my idea. :D -- There is no royal road to learning. HighGo Software Co.
Re: Key management with tests
On Tue, Jan 12, 2021 at 09:32:54AM +0900, Masahiko Sawada wrote: > On Tue, Jan 12, 2021 at 3:23 AM Stephen Frost wrote: > > Right, or ensure that the actual IV used is distinct (such as by using > > another bit in the IV to distinguish logged-vs-unlogged), but it seems > > saner to just use a different key, ultimately. > > Agreed. > > I think we also need to consider how to make sure nonce is unique when > making a page dirty by updating hint bits. Hint bit update changes the > page contents but doesn't change the page lsn if we already write a > full page write. In the PoC patch, I logged a dummy WAL record > (XLOG_NOOP) just to move the page lsn forward, but since this is > required even when changing the page is not the first time since the > last checkpoint we might end up logging too many dummy WAL records. This says: https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements wal_log_hints will be enabled automatically in encryption mode. Does that help? -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Tue, Jan 12, 2021 at 3:23 AM Stephen Frost wrote: > > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Mon, Jan 11, 2021 at 12:54:49PM -0500, Stephen Frost wrote: > > > Although, another approach and one that I've discussed a bit with Bruce, > > > is to have more keys- such as a key for temporary files, and perhaps > > > even a key for logged relations and a different for unlogged.. Or > > > > Yes, we have to make sure the nonce (computed as LSN/pageno) is never > > reused, so if we have several LSN usage "spaces", they need different > > data keys. > > Right, or ensure that the actual IV used is distinct (such as by using > another bit in the IV to distinguish logged-vs-unlogged), but it seems > saner to just use a different key, ultimately. Agreed. I think we also need to consider how to make sure nonce is unique when making a page dirty by updating hint bits. Hint bit update changes the page contents but doesn't change the page lsn if we already write a full page write. In the PoC patch, I logged a dummy WAL record (XLOG_NOOP) just to move the page lsn forward, but since this is required even when changing the page is not the first time since the last checkpoint we might end up logging too many dummy WAL records. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Mon, Jan 11, 2021 at 02:19:22PM -0500, Stephen Frost wrote: > > outputs from the GCM encryption and is what provides the integrity / > > authentication of the encrypted data to be able to detect if it's been > > modified. Unfortunately, while the page checksum will continue to be > > used and available for checking against disk corruption, it's not > > sufficient. Hence, ideally, we'd find a spot to stick the 128-bit tag > > on each page. > > Agreed. Would checksums be of any value with GCM? The value would be to allow testing of the database integrity, to the amount allowed by the checksum, to be done without having access to the encryption keys, and because there's not much else we'd be using those bits for if we didn't. > > Given that, clearly, it's not possible to go from an unencrypted cluster > > to an encrypted cluster without rewriting the entire cluster, we aren't > > bound to maintain the on-disk page format, we should be able to > > accomadate including the tag somewhere. Unfortuantely, it doesn't seem > > quite as trivial as I'd hoped since there are parts of the code which > > make assumptions about the page beyond perhaps what they should be, but > > I'm still hopeful that it won't be *too* hard to do. > > OK, thanks. Are there other page improvements we should make when we > are requiring a page rewrite? This is an interesting question but ultimately I don't think we should be looking at this from the perspective of allowing arbitrary changes to the page format. The challenge is that much of the page format, today, is defined by a C struct and changing the way that works would require a great deal of code to be modified and turn this into a massive effort, assuming we wish to have the same compiled binary able to work with both unencrypted and encrypted clusters, which I do believe is a requirement. The thought that I had was to, instead, try to figure out if we could fudge some space by, say, putting a 128-bit 'hole' at the end of the page and just move pd_special back, effectively making the page seem 'smaller' to all of the code that uses it, except for the code that knows how to do the decryption. I ran into some trouble with that but haven't quite sorted out what happened yet. Other ideas would be to put it before pd_special, or maybe somewhere else, but a lot depends on the code's expectations. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Mon, Jan 11, 2021 at 02:19:22PM -0500, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Mon, Jan 11, 2021 at 01:23:27PM -0500, Stephen Frost wrote: > > > Yes, and it avoids the issue of using a single key for too much, which > > > is also a concern. The remaining larger issues are to figure out a > > > place to put the tag for each page, and the relatively simple matter of > > > programming a mechanism to cache the keys we're commonly using (current > > > key for encryption, recently used keys for decryption) since we'll > > > eventually get to a point of having written out more data than we are > > > going to keep keys in memory for. > > > > I thought the LSN range would be stored with the keys, so there is no > > need to tag the LSN on each page. > > Yes, LSN range would be stored with the keys in some fashion (maybe just > the start of a particular LSN range would be in the filename of the key > for that range...). The 'tag' that I'm referring to there is one of the Oh, that tag, yes, we need to add that to each page. I thought you mean an LSN-range-key tag. > outputs from the GCM encryption and is what provides the integrity / > authentication of the encrypted data to be able to detect if it's been > modified. Unfortunately, while the page checksum will continue to be > used and available for checking against disk corruption, it's not > sufficient. Hence, ideally, we'd find a spot to stick the 128-bit tag > on each page. Agreed. Would checksums be of any value with GCM? > Given that, clearly, it's not possible to go from an unencrypted cluster > to an encrypted cluster without rewriting the entire cluster, we aren't > bound to maintain the on-disk page format, we should be able to > accomadate including the tag somewhere. Unfortuantely, it doesn't seem > quite as trivial as I'd hoped since there are parts of the code which > make assumptions about the page beyond perhaps what they should be, but > I'm still hopeful that it won't be *too* hard to do. OK, thanks. Are there other page improvements we should make when we are requiring a page rewrite? -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Mon, Jan 11, 2021 at 01:23:27PM -0500, Stephen Frost wrote: > > Yes, and it avoids the issue of using a single key for too much, which > > is also a concern. The remaining larger issues are to figure out a > > place to put the tag for each page, and the relatively simple matter of > > programming a mechanism to cache the keys we're commonly using (current > > key for encryption, recently used keys for decryption) since we'll > > eventually get to a point of having written out more data than we are > > going to keep keys in memory for. > > I thought the LSN range would be stored with the keys, so there is no > need to tag the LSN on each page. Yes, LSN range would be stored with the keys in some fashion (maybe just the start of a particular LSN range would be in the filename of the key for that range...). The 'tag' that I'm referring to there is one of the outputs from the GCM encryption and is what provides the integrity / authentication of the encrypted data to be able to detect if it's been modified. Unfortunately, while the page checksum will continue to be used and available for checking against disk corruption, it's not sufficient. Hence, ideally, we'd find a spot to stick the 128-bit tag on each page. Given that, clearly, it's not possible to go from an unencrypted cluster to an encrypted cluster without rewriting the entire cluster, we aren't bound to maintain the on-disk page format, we should be able to accomadate including the tag somewhere. Unfortuantely, it doesn't seem quite as trivial as I'd hoped since there are parts of the code which make assumptions about the page beyond perhaps what they should be, but I'm still hopeful that it won't be *too* hard to do. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Mon, Jan 11, 2021 at 01:23:27PM -0500, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Mon, Jan 11, 2021 at 12:54:49PM -0500, Stephen Frost wrote: > > > Although, another approach and one that I've discussed a bit with Bruce, > > > is to have more keys- such as a key for temporary files, and perhaps > > > even a key for logged relations and a different for unlogged.. Or > > > > Yes, we have to make sure the nonce (computed as LSN/pageno) is never > > reused, so if we have several LSN usage "spaces", they need different > > data keys. > > Right, or ensure that the actual IV used is distinct (such as by using > another bit in the IV to distinguish logged-vs-unlogged), but it seems > saner to just use a different key, ultimately. Yes, we have eight unused bit in the Nonce right now. > > > perhaps sets of keys for each which automatically are rotating every X > > > number of GB based on the LSN... Which is a big part of why key > > > management is such an important part of this effort. > > > > Yes, this would avoid the need to failover to a standby for data key > > rotation. > > Yes, and it avoids the issue of using a single key for too much, which > is also a concern. The remaining larger issues are to figure out a > place to put the tag for each page, and the relatively simple matter of > programming a mechanism to cache the keys we're commonly using (current > key for encryption, recently used keys for decryption) since we'll > eventually get to a point of having written out more data than we are > going to keep keys in memory for. I thought the LSN range would be stored with the keys, so there is no need to tag the LSN on each page. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Mon, Jan 11, 2021 at 12:54:49PM -0500, Stephen Frost wrote: > > Although, another approach and one that I've discussed a bit with Bruce, > > is to have more keys- such as a key for temporary files, and perhaps > > even a key for logged relations and a different for unlogged.. Or > > Yes, we have to make sure the nonce (computed as LSN/pageno) is never > reused, so if we have several LSN usage "spaces", they need different > data keys. Right, or ensure that the actual IV used is distinct (such as by using another bit in the IV to distinguish logged-vs-unlogged), but it seems saner to just use a different key, ultimately. > > perhaps sets of keys for each which automatically are rotating every X > > number of GB based on the LSN... Which is a big part of why key > > management is such an important part of this effort. > > Yes, this would avoid the need to failover to a standby for data key > rotation. Yes, and it avoids the issue of using a single key for too much, which is also a concern. The remaining larger issues are to figure out a place to put the tag for each page, and the relatively simple matter of programming a mechanism to cache the keys we're commonly using (current key for encryption, recently used keys for decryption) since we'll eventually get to a point of having written out more data than we are going to keep keys in memory for. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Mon, Jan 11, 2021 at 12:54:49PM -0500, Stephen Frost wrote: > Although, another approach and one that I've discussed a bit with Bruce, > is to have more keys- such as a key for temporary files, and perhaps > even a key for logged relations and a different for unlogged.. Or Yes, we have to make sure the nonce (computed as LSN/pageno) is never reused, so if we have several LSN usage "spaces", they need different data keys. > perhaps sets of keys for each which automatically are rotating every X > number of GB based on the LSN... Which is a big part of why key > management is such an important part of this effort. Yes, this would avoid the need to failover to a standby for data key rotation. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Mon, Jan 11, 2021 at 08:12:00PM +0900, Masahiko Sawada wrote: > > Looking at the patch, it supports three algorithms but only > > PG_CIPHER_AES_KWP is used in the core for now: > > > > +/* > > + * Supported symmetric encryption algorithm. These identifiers are passed > > + * to pg_cipher_ctx_create() function, and then actual encryption > > + * implementations need to initialize their context of the given encryption > > + * algorithm. > > + */ > > +#define PG_CIPHER_AES_GCM 0 > > +#define PG_CIPHER_AES_KW 1 > > +#define PG_CIPHER_AES_KWP 2 > > +#define PG_MAX_CIPHER_ID 3 > > > > Are we in the process of experimenting which algorithms are better? If > > we support one algorithm that is actually used in the core, we would > > reduce the tests as well. > > I think we are only using KWP (Key Wrap with Padding) because that is > for wrapping keys: > > > https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/KWVS.pdf Yes. > I am not sure about KW. I think we are using GCM for the WAP/heap/index > pages. Stephen would know more. KW was more-or-less 'for free' and there were tests for it, which is why it was included. Yes, GCM would be for WAL/heap/index pages, it wouldn't be appropriate to use KW or KWP for that. Using KW/KWP for the key wrapping also makes the API simpler- and therefore easier for other implementations to be written which provide the same API. > > FWIW, I've written a PoC patch for buffer encryption to make sure the > > kms patch would be workable with other components using the encryption > > key managed by kmgr. > > Wow, it is a small patch --- nice. I agree that the actual encryption patch, for just the main heap/index, won't be too bad. The larger part will be dealing with all of the temporary files we create that have user data in them... I've been contemplating a way to try and make that part of the patch smaller though and hopefully that will bear fruit and we can avoid having to change a lof of, eg, reorderbuffer.c and pgstat.c. There's a few places where we need to be sure to be updating the LSN for both logged and unlogged relations properly, including dealing with things like the magic GIST "GistBuildLSN" fake-LSN too, and we will absolutely need to have a bit used in the IV to distinguish if it's a real LSN or an unlogged LSN. Although, another approach and one that I've discussed a bit with Bruce, is to have more keys- such as a key for temporary files, and perhaps even a key for logged relations and a different for unlogged.. Or perhaps sets of keys for each which automatically are rotating every X number of GB based on the LSN... Which is a big part of why key management is such an important part of this effort. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Mon, Jan 11, 2021 at 08:12:00PM +0900, Masahiko Sawada wrote: > On Sun, Jan 10, 2021 at 11:51 PM Bruce Momjian wrote: > > OK, here they are with numeric prefixes. It was actually tricky to > > figure out how to create a squashed format-patch based on another branch. > > Thank you for attaching the patches. It passes all cfbot tests, great. Yeah, I saw that. :-) I head to learn a lot about how to create squashed format-patches on non-master branches. I have now automated it so it will be easy going forward. > Looking at the patch, it supports three algorithms but only > PG_CIPHER_AES_KWP is used in the core for now: > > +/* > + * Supported symmetric encryption algorithm. These identifiers are passed > + * to pg_cipher_ctx_create() function, and then actual encryption > + * implementations need to initialize their context of the given encryption > + * algorithm. > + */ > +#define PG_CIPHER_AES_GCM 0 > +#define PG_CIPHER_AES_KW 1 > +#define PG_CIPHER_AES_KWP 2 > +#define PG_MAX_CIPHER_ID 3 > > Are we in the process of experimenting which algorithms are better? If > we support one algorithm that is actually used in the core, we would > reduce the tests as well. I think we are only using KWP (Key Wrap with Padding) because that is for wrapping keys: https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/KWVS.pdf I am not sure about KW. I think we are using GCM for the WAP/heap/index pages. Stephen would know more. > FWIW, I've written a PoC patch for buffer encryption to make sure the > kms patch would be workable with other components using the encryption > key managed by kmgr. Wow, it is a small patch --- nice. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Sun, Jan 10, 2021 at 11:51 PM Bruce Momjian wrote: > > On Sun, Jan 10, 2021 at 06:04:12PM +1300, Thomas Munro wrote: > > On Sun, Jan 10, 2021 at 3:45 PM Bruce Momjian wrote: > > > Does anyone know why the cfbot applied the patch listed second first > > > here? > > > > > > http://cfbot.cputube.org/patch_31_2925.log > > > > > > Specifically, it applied hex..key.diff.gz before hex.diff.gz. I assumed > > > it would apply attachments in the order they appear in the email. > > > > It sorts the filenames (in this case after decompressing step removes > > the .gz endings). That works pretty well for the patches that "git > > format-patch" spits out, but it's a bit hit and miss with cases like > > yours. > > OK, here they are with numeric prefixes. It was actually tricky to > figure out how to create a squashed format-patch based on another branch. > Thank you for attaching the patches. It passes all cfbot tests, great. Looking at the patch, it supports three algorithms but only PG_CIPHER_AES_KWP is used in the core for now: +/* + * Supported symmetric encryption algorithm. These identifiers are passed + * to pg_cipher_ctx_create() function, and then actual encryption + * implementations need to initialize their context of the given encryption + * algorithm. + */ +#define PG_CIPHER_AES_GCM 0 +#define PG_CIPHER_AES_KW 1 +#define PG_CIPHER_AES_KWP 2 +#define PG_MAX_CIPHER_ID 3 Are we in the process of experimenting which algorithms are better? If we support one algorithm that is actually used in the core, we would reduce the tests as well. FWIW, I've written a PoC patch for buffer encryption to make sure the kms patch would be workable with other components using the encryption key managed by kmgr. Overall it’s good. While the buffer encryption patch is still PoC quality and there are some problems regarding nonce generation we need to deal with, it easily can use the relation key managed by the kmgr to encrypt/decrypt buffers. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/ 0003-Poc-buffer-encryption.patch Description: Binary data
Re: Key management with tests
On Sun, Jan 10, 2021 at 3:45 PM Bruce Momjian wrote: > Does anyone know why the cfbot applied the patch listed second first > here? > > http://cfbot.cputube.org/patch_31_2925.log > > Specifically, it applied hex..key.diff.gz before hex.diff.gz. I assumed > it would apply attachments in the order they appear in the email. It sorts the filenames (in this case after decompressing step removes the .gz endings). That works pretty well for the patches that "git format-patch" spits out, but it's a bit hit and miss with cases like yours.
Re: Key management with tests
On Sat, Jan 9, 2021 at 08:08:16PM -0500, Bruce Momjian wrote: > On Sat, Jan 9, 2021 at 01:17:36PM -0500, Bruce Momjian wrote: > > I know we are still working on the hex patch (dest-len) and the crypto > > tests, but I wanted to post this so people can see where we are, and we > > can get some current cfbot testing. > > Here is an updated version that covers all the possible > testing/configuration options. Does anyone know why the cfbot applied the patch listed second first here? http://cfbot.cputube.org/patch_31_2925.log Specifically, it applied hex..key.diff.gz before hex.diff.gz. I assumed it would apply attachments in the order they appear in the email. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Fri, Jan 8, 2021 at 03:33:44PM -0500, Stephen Frost wrote: > > > Anyway, I think we need to figure out how to trim. The first part would > > > be to figure out whether we need 128 _and_ 256-bit tests, and then see > > > what items are really useful. Stephen, do you have any ideas on that? > > > We currently have 10296 tests, and I think we could get away with 100. > > > > Yeah, it's probably still too much, but I don't have any particularly > > justifiable suggestions as to exactly what we should remove or what we > > should keep. > > > > Perhaps it'd make sense to try and cover the cases that are more likely > > to be issues between our wrapper functions and OpenSSL, and not stress > > too much about constantly testing cases that should really be up to > > OpenSSL. As such, I'd propose: > > > > - Add back in some 192-bit tests, so we cover all three bit lengths. > > - Add back in some additional authenticated test cases, just to make > > sure that, until/unless we implement support, the test code properly > > skips over those. > > - Keep tests for various length plaintext/ciphertext (including 0-byte > > cases, so we make sure those work, since they really should). > > - Keep at least one test for each length of tag that's included in the > > test suite. > > Makes sense. I did a simplistic trim-down to 90 tests but it still was > 40% of the patch; attached. The hex strings are very long. I don't think we actually need to stress over the size of the test data relative to the size of the patch- it's not like it's all that much perl code. I can appreciate that we don't want to add megabytes worth of test data to the git repo though. > > I'm not sure how many tests we'd end up with from that, but my swag / > > gut feeling is that it'd probably be on the order of 100ish and a small > > enough set that it won't dwarf the rest of the patch. > > > > Would be nice if we had a way for some buildfarm animal or something to > > pull in the entire suite and test it, imv.. If anyone wants to > > volunteer, I'd be happy to explain how to make that happen (it's not > > hard though- download/unzip the files, drop them in the directory, > > update the test script to add all the files into the array). > > Yes, do we have a place to store more comprehensive tests outside of our > git tree? Has this been done before? Not that I'm aware of. Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Fri, Jan 8, 2021 at 03:34:23PM -0500, Stephen Frost wrote: > > All the tests pass now. The current src/test directory is 19MB, and > > adding these tests takes it to 23MB, or a 20% increase. That seems like > > a lot. It is testing 128-bit and 256-bit keys --- should we do fewer > > tests, or just test 256, or use gzip to compress the tests by 50%? > > (Does every platform have gzip?) > > Thanks a lot for working on this and figuring out what the issue was and > fixing it! That's great that we got all those cases passing for you > too. Yes, I was relieved. The pattern of when zero-length strings fail in which modes is still very odd, but at least it reports an error, so it isn't returning incorrect data. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Fri, Jan 8, 2021 at 03:33:44PM -0500, Stephen Frost wrote: > > No, I don't think so. Stephen imported the entire NIST test suite. It > > was so comperhensive, it detected several OpenSSL bugs for zero-length > > strings, which I already reported, but we would never be encrypting > > zero-length strings, so there wasn't a lot of value to it. > > I ran the entire test suite locally to ensure everything worked, but I > didn't actually include all of it in the PR which you merged- I had > already reduced it quite a bit by removing all 'additional > authenticated data' test cases (which the tests will automatically skip > and which we haven't implemented support for in the common library > wrappers) and by removing the 192-bit cases. This reduced the overall > test set by about 2/3rd's or so, as I recall. Wow, so that was reduced! > > Anyway, I think we need to figure out how to trim. The first part would > > be to figure out whether we need 128 _and_ 256-bit tests, and then see > > what items are really useful. Stephen, do you have any ideas on that? > > We currently have 10296 tests, and I think we could get away with 100. > > Yeah, it's probably still too much, but I don't have any particularly > justifiable suggestions as to exactly what we should remove or what we > should keep. > > Perhaps it'd make sense to try and cover the cases that are more likely > to be issues between our wrapper functions and OpenSSL, and not stress > too much about constantly testing cases that should really be up to > OpenSSL. As such, I'd propose: > > - Add back in some 192-bit tests, so we cover all three bit lengths. > - Add back in some additional authenticated test cases, just to make > sure that, until/unless we implement support, the test code properly > skips over those. > - Keep tests for various length plaintext/ciphertext (including 0-byte > cases, so we make sure those work, since they really should). > - Keep at least one test for each length of tag that's included in the > test suite. Makes sense. I did a simplistic trim-down to 90 tests but it still was 40% of the patch; attached. The hex strings are very long. > I'm not sure how many tests we'd end up with from that, but my swag / > gut feeling is that it'd probably be on the order of 100ish and a small > enough set that it won't dwarf the rest of the patch. > > Would be nice if we had a way for some buildfarm animal or something to > pull in the entire suite and test it, imv.. If anyone wants to > volunteer, I'd be happy to explain how to make that happen (it's not > hard though- download/unzip the files, drop them in the directory, > update the test script to add all the files into the array). Yes, do we have a place to store more comprehensive tests outside of our git tree? Has this been done before? -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee cryptotest.tgz Description: application/gtar-compressed
Re: Key management with tests
Greetings Bruce,
* Bruce Momjian (br...@momjian.us) wrote:
> On Fri, Jan 1, 2021 at 01:07:50AM -0500, Bruce Momjian wrote:
> > On Thu, Dec 31, 2020 at 11:50:47PM -0500, Bruce Momjian wrote:
> > > I have completed the key management patch with tests created by Stephen
> > > Frost. Original patch by Masahiko Sawada. It requires the hex
> > > reorganization patch first. The key patch is now 2.1MB because of the
> > > tests, so attaching it here seems unwise:
> > >
> > > https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff
> > > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
> > >
> > > I will add it to the commitfest. I think we need to figure out how much
> > > of the tests we want to add.
> >
> > I am getting regression test errors using OpenSSL 1.1.1d 10 Sep 2019
> > with zero-length input data (no -p), while Stephen is able for those
> > tests to pass. This needs more research, plus I think higher-level
> > tests.
>
> I have found the cause of the failure, which I added as a C comment:
>
> /*
> * OpenSSL 1.1.1d and earlier crashes on some zero-length plaintext
> * and ciphertext strings. It crashes on an encryption call to
> * EVP_EncryptFinal_ex(() in GCM mode of zero-length strings if
> * plaintext is NULL, even though plaintext_len is zero. Setting
> * plaintext to non-NULL allows it to work. In KW/KWP mode,
> * zero-length strings fail if plaintext_len = 0 and plaintext is
> * non-NULL (the opposite). OpenSSL 1.1.1e+ is fine with all options.
> */
> else if (cipher == PG_CIPHER_AES_GCM)
> {
> plaintext_len = 0;
> plaintext = pg_malloc0(1);
> }
>
> All the tests pass now. The current src/test directory is 19MB, and
> adding these tests takes it to 23MB, or a 20% increase. That seems like
> a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
> tests, or just test 256, or use gzip to compress the tests by 50%?
> (Does every platform have gzip?)
Thanks a lot for working on this and figuring out what the issue was and
fixing it! That's great that we got all those cases passing for you
too.
Thanks again,
Stephen
signature.asc
Description: PGP signature
Re: Key management with tests
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Thu, Jan 7, 2021 at 04:08:49PM -0300, Álvaro Herrera wrote: > > On 2021-Jan-07, Bruce Momjian wrote: > > > > > All the tests pass now. The current src/test directory is 19MB, and > > > adding these tests takes it to 23MB, or a 20% increase. That seems like > > > a lot. It is testing 128-bit and 256-bit keys --- should we do fewer > > > tests, or just test 256, or use gzip to compress the tests by 50%? > > > (Does every platform have gzip?) > > > > So the tests are about 95% of the patch ... do we really need that many > > tests? > > No, I don't think so. Stephen imported the entire NIST test suite. It > was so comperhensive, it detected several OpenSSL bugs for zero-length > strings, which I already reported, but we would never be encrypting > zero-length strings, so there wasn't a lot of value to it. I ran the entire test suite locally to ensure everything worked, but I didn't actually include all of it in the PR which you merged- I had already reduced it quite a bit by removing all 'additional authenticated data' test cases (which the tests will automatically skip and which we haven't implemented support for in the common library wrappers) and by removing the 192-bit cases. This reduced the overall test set by about 2/3rd's or so, as I recall. > Anyway, I think we need to figure out how to trim. The first part would > be to figure out whether we need 128 _and_ 256-bit tests, and then see > what items are really useful. Stephen, do you have any ideas on that? > We currently have 10296 tests, and I think we could get away with 100. Yeah, it's probably still too much, but I don't have any particularly justifiable suggestions as to exactly what we should remove or what we should keep. Perhaps it'd make sense to try and cover the cases that are more likely to be issues between our wrapper functions and OpenSSL, and not stress too much about constantly testing cases that should really be up to OpenSSL. As such, I'd propose: - Add back in some 192-bit tests, so we cover all three bit lengths. - Add back in some additional authenticated test cases, just to make sure that, until/unless we implement support, the test code properly skips over those. - Keep tests for various length plaintext/ciphertext (including 0-byte cases, so we make sure those work, since they really should). - Keep at least one test for each length of tag that's included in the test suite. I'm not sure how many tests we'd end up with from that, but my swag / gut feeling is that it'd probably be on the order of 100ish and a small enough set that it won't dwarf the rest of the patch. Would be nice if we had a way for some buildfarm animal or something to pull in the entire suite and test it, imv.. If anyone wants to volunteer, I'd be happy to explain how to make that happen (it's not hard though- download/unzip the files, drop them in the directory, update the test script to add all the files into the array). Thanks, Stephen signature.asc Description: PGP signature
Re: Key management with tests
On Thu, Jan 7, 2021 at 10:02:14AM -0500, Bruce Momjian wrote: > My next step is to add the high-level tests. Here is the high-level script, and the log output. I used the pg_upgrade test.sh as a model. It uses "CFE DEBUG" lines that are already in the code to compare the initdb encryption with the other initdb decryption and pg_ctl decryption. It was easier than I thought. What it does not do is to test the file descriptor passing from /dev/tty, or the sample scripts. This seems acceptable to me since I test them and they rarely change. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee test.sh Description: Bourne shell script make[1]: Leaving directory '/usr/local/src/gen/pgsql/postgresql/src/common' MAKE= PATH="/pgtop/tmp_install/usr/local/pgsql/bin:$PATH" LD_LIBRARY_PATH="/pgtop/tmp_install/usr/local/pgsql/lib" bindir=/pgtop/tmp_install//usr/local/pgsql/bin /bin/sh test.sh The files belonging to this database system will be owned by user "postgres". This user must also own the server process. The database cluster will be initialized with locale "en_US.UTF-8". The default database encoding has accordingly been set to "UTF8". The default text search configuration will be set to "english". Data page checksums are disabled. Cluster file encryption is enabled. creating directory /pgtop/src/test/crypto/tmp_check/data ... ok creating subdirectories ... ok selecting dynamic shared memory implementation ... posix selecting default max_connections ... 100 selecting default shared_buffers ... 128MB selecting default time zone ... America/New_York creating configuration files ... ok running bootstrap script ... ok performing post-bootstrap initialization ... ok Sync to disk skipped. The data directory might become corrupt if the operating system crashes. Success. You can now start the database server using: /pgtop/tmp_install/usr/local/pgsql/bin/pg_ctl -D /pgtop/src/test/crypto/tmp_check/data -l logfile start waiting for server to start done server started waiting for server to shut down done server stopped PASSED rm -rf '/usr/local/src/gen/pgsql/postgresql/src/test/crypto'/tmp_check /bin/mkdir -p '/usr/local/src/gen/pgsql/postgresql/src/test/crypto'/tmp_check cd . && TESTDIR='/usr/local/src/gen/pgsql/postgresql/src/test/crypto' PATH="/pgtop/tmp_install/usr/local/pgsql/bin:$PATH" LD_LIBRARY_PATH="/pgtop/tmp_install/usr/local/pgsql/lib" PGPORT='65432' PG_REGRESS='/usr/local/src/gen/pgsql/postgresql/src/test/crypto/../../../src/test/regress/pg_regress' REGRESS_SHLIB='/pgtop/src/test/regress/regress.so' /usr/bin/prove -I ../../../src/test/perl/ -I . t/*.pl t/001_testcrypto.pl .. ok t/002_testkw.pl .. ok All tests successful. Files=2, Tests=10296, 138 wallclock secs ( 1.81 usr 0.22 sys + 26.55 cusr 24.02 csys = 52.60 CPU) Result: PASS
Re: Key management with tests
On Thu, Jan 7, 2021 at 04:08:49PM -0300, Álvaro Herrera wrote: > On 2021-Jan-07, Bruce Momjian wrote: > > > All the tests pass now. The current src/test directory is 19MB, and > > adding these tests takes it to 23MB, or a 20% increase. That seems like > > a lot. It is testing 128-bit and 256-bit keys --- should we do fewer > > tests, or just test 256, or use gzip to compress the tests by 50%? > > (Does every platform have gzip?) > > So the tests are about 95% of the patch ... do we really need that many > tests? No, I don't think so. Stephen imported the entire NIST test suite. It was so comperhensive, it detected several OpenSSL bugs for zero-length strings, which I already reported, but we would never be encrypting zero-length strings, so there wasn't a lot of value to it. Anyway, I think we need to figure out how to trim. The first part would be to figure out whether we need 128 _and_ 256-bit tests, and then see what items are really useful. Stephen, do you have any ideas on that? We currently have 10296 tests, and I think we could get away with 100. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On 2021-Jan-07, Bruce Momjian wrote: > All the tests pass now. The current src/test directory is 19MB, and > adding these tests takes it to 23MB, or a 20% increase. That seems like > a lot. It is testing 128-bit and 256-bit keys --- should we do fewer > tests, or just test 256, or use gzip to compress the tests by 50%? > (Does every platform have gzip?) So the tests are about 95% of the patch ... do we really need that many tests? -- Álvaro Herrera
Re: Key management with tests
On Fri, Jan 1, 2021 at 01:07:50AM -0500, Bruce Momjian wrote:
> On Thu, Dec 31, 2020 at 11:50:47PM -0500, Bruce Momjian wrote:
> > I have completed the key management patch with tests created by Stephen
> > Frost. Original patch by Masahiko Sawada. It requires the hex
> > reorganization patch first. The key patch is now 2.1MB because of the
> > tests, so attaching it here seems unwise:
> >
> > https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff
> > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
> >
> > I will add it to the commitfest. I think we need to figure out how much
> > of the tests we want to add.
>
> I am getting regression test errors using OpenSSL 1.1.1d 10 Sep 2019
> with zero-length input data (no -p), while Stephen is able for those
> tests to pass. This needs more research, plus I think higher-level
> tests.
I have found the cause of the failure, which I added as a C comment:
/*
* OpenSSL 1.1.1d and earlier crashes on some zero-length plaintext
* and ciphertext strings. It crashes on an encryption call to
* EVP_EncryptFinal_ex(() in GCM mode of zero-length strings if
* plaintext is NULL, even though plaintext_len is zero. Setting
* plaintext to non-NULL allows it to work. In KW/KWP mode,
* zero-length strings fail if plaintext_len = 0 and plaintext is
* non-NULL (the opposite). OpenSSL 1.1.1e+ is fine with all options.
*/
else if (cipher == PG_CIPHER_AES_GCM)
{
plaintext_len = 0;
plaintext = pg_malloc0(1);
}
All the tests pass now. The current src/test directory is 19MB, and
adding these tests takes it to 23MB, or a 20% increase. That seems like
a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
tests, or just test 256, or use gzip to compress the tests by 50%?
(Does every platform have gzip?)
My next step is to add the high-level tests.
--
Bruce Momjian https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Thu, Dec 31, 2020 at 11:50:47PM -0500, Bruce Momjian wrote: > I have completed the key management patch with tests created by Stephen > Frost. Original patch by Masahiko Sawada. It requires the hex > reorganization patch first. The key patch is now 2.1MB because of the > tests, so attaching it here seems unwise: > > https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff > > I will add it to the commitfest. I think we need to figure out how much > of the tests we want to add. I am getting regression test errors using OpenSSL 1.1.1d 10 Sep 2019 with zero-length input data (no -p), while Stephen is able for those tests to pass. This needs more research, plus I think higher-level tests. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Key management with tests
I have completed the key management patch with tests created by Stephen Frost. Original patch by Masahiko Sawada. It requires the hex reorganization patch first. The key patch is now 2.1MB because of the tests, so attaching it here seems unwise: https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff https://github.com/postgres/postgres/compare/master...bmomjian:key.diff I will add it to the commitfest. I think we need to figure out how much of the tests we want to add. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
