Re: Moving forward with TDE [PATCH v3]
On Mon, 22 Jan 2024 at 11:47, Peter Smith wrote: > > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > there were CFbot test failures last time it was run [2]. Please have a > look and post an updated version if necessary. The patch which you submitted has been awaiting your attention for quite some time now. As such, we have moved it to "Returned with Feedback" and removed it from the reviewing queue. Depending on timing, this may be reversible. Kindly address the feedback you have received, and resubmit the patch to the next CommitFest. Regards, Vignesh
Re: Moving forward with TDE [PATCH v3]
2024-01 Commitfest. Hi, This patch has a CF status of "Needs Review" [1], but it seems there were CFbot test failures last time it was run [2]. Please have a look and post an updated version if necessary. == [1] https://commitfest.postgresql.org/46/3985/ [2] https://cirrus-ci.com/task/5498215743619072 Kind Regards, Peter Smith.
Re: Moving forward with TDE
On Sun, Dec 17, 2023 at 06:30:50AM +, Chris Travers wrote: > Hi, > > I was re-reading the patches here and there was one thing I didn't > understand. > > There are provisions for a separation of data encryption keys for primary and > replica I see, and these share a single WAL key. > > But if I am setting up a replica from the primary, and the primary is already > encrypted, then do these forceably share the same data encrypting keys? Is > there a need to have (possibly in a follow-up patch) an ability to decrypt > and re-encrypt in pg_basebackup (which would need access to both keys) or is > this handled already and I just missed it? Yes, decrypt and re-encrypt in pg_basebackup would be necessary, or in the actual protocol stream. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: Moving forward with TDE
Hi, I was re-reading the patches here and there was one thing I didn't understand. There are provisions for a separation of data encryption keys for primary and replica I see, and these share a single WAL key. But if I am setting up a replica from the primary, and the primary is already encrypted, then do these forceably share the same data encrypting keys? Is there a need to have (possibly in a follow-up patch) an ability to decrypt and re-encrypt in pg_basebackup (which would need access to both keys) or is this handled already and I just missed it? Best Wishes, Chris Travers
Re: Moving forward with TDE [PATCH v3]
On Tue, Nov 7, 2023 at 5:49 PM Andres Freund wrote: > Hi, > > On 2023-11-06 09:56:37 -0500, Stephen Frost wrote: > > * Andres Freund (and...@anarazel.de) wrote: > > > I still am quite quite unconvinced that using the LSN as a nonce is a > good > > > design decision. > > > > This is a really important part of the overall path to moving this > > forward, so I wanted to jump to it and have a specific discussion > > around this. I agree that there are downsides to using the LSN, some of > > which we could possibly address (eg: include the timeline ID in the IV), > > but others that would be harder to deal with. > > > The question then is- what's the alternative? > > > > One approach would be to change the page format to include space for an > > explicit nonce. I don't see the community accepting such a new page > > format as the only format we support though as that would mean no > > pg_upgrade support along with wasted space if TDE isn't being used. > > Right. > Hmm, if we /were/ to introduce some sort of page format change, Couldn't that be a use case for modifying the pd_version field? Could v4 pages be read in and written out as v5 pages with different interpretations? > > Ideally, we'd instead be able to support multiple page formats where > > users could decide when they create their cluster what features they > > want- and luckily we even have such an effort underway with patches > > posted for review [1]. > > I think there are some details wrong with that patch - IMO the existing > macros > should just continue to work as-is and instead the places that want the > more > narrow definition should be moved to the new macros and it changes places > that > should continue to use compile time constants - but it doesn't seem like a > fundamentally bad idea to me. I certainly like it much better than making > the > page size runtime configurable. > There had been some discussion about this WRT renaming macros and the like (constants, etc)—I think a new pass eliminating the variable blocksize pieces and seeing if we can minimize churn here is worthwhile, will take a look and see what the minimally-viable set of changes is here. > (I'll try to reply with the above points to [1]) > > > > Certainly, with the base page-special-feature patch, we could have an > option > > for users to choose that they want a better nonce than the LSN, or we > could > > bundle that assumption in with, say, the authenticated-encryption feature > > (if you want authenticated encryption, then you want more from the > > encryption system than the basics, and therefore we presume you also > want a > > better nonce than the LSN). > > I don't think we should support using the LSN as a nonce if we have an > alternative. The cost and complexity overhead is just not worth it. Yes, > it'll be harder for users to migrate to encryption, but adding complexity > elsewhere in the system to get an inferior result isn't worth it. > >From my read, XTS (which I'd see as inferior to authenticated encryption, but better than some other options) could use LSN as an IV without leakage concerns, perhaps mixing in the BlockNumber as well. If we are going to allow multiple encryption types, I think we may need to consider that needs for IVs may be different, so this may need to be something that is selectable per encryption type. I am unclear how much of a requirement this is, but seems like having a design supporting this to be pluggable—even if a static lookup table internally for encryption type, block length, IV source, etc—seems the most future proof if we had to retire an encryption method or prevent creation of specific methods, say. > > Another approach would be a separate fork, but that then has a number of > > downsides too- every write has to touch that too, and a single page of > > nonces would cover a pretty large number of pages also. > > Yea, the costs of doing so is nontrivial. If you were trying to implement > encryption on the smgr level - which I doubt you should but am not certain > about - my suggestion would be to interleave pages with metadata like > nonces > and AEAD with the data pages. I.e. one metadata page would be followed by > (BLCKSZ - SizeOfPageHeaderData) / (sizeof(nonce) + sizeof(AEAD)) > pages containing actual relation data. That way you still get decent > locality > during scans and writes. > Hmm, this is actually an interesting idea, I will think about this a bit. > Relation forks were a mistake, we shouldn't use them in more places. > > > I think it'd be much better if we also encrypted forks, rather than just > the > main fork... > I believe the existing code should just work by modifying the PageNeedsToBeEncrypted macro; I will test that and see if anything blows up. David
Re: Moving forward with TDE [PATCH v3]
On Tue, Nov 7, 2023 at 6:47 PM Andres Freund wrote: > Hi, > > On 2023-11-06 11:26:44 +0100, Matthias van de Meent wrote: > > On Sat, 4 Nov 2023 at 03:38, Andres Freund wrote: > > > On 2023-11-02 22:09:40 +0100, Matthias van de Meent wrote: > > > > I'm quite surprised at the significant number of changes being made > > > > outside the core storage manager files. I thought that changing out > > > > mdsmgr with an encrypted smgr (that could wrap mdsmgr if so desired) > > > > would be the most obvious change to implement cluster-wide encryption > > > > with the least code touched, as relations don't need to know whether > > > > the files they're writing are encrypted, right? Is there a reason to > > > > not implement this at the smgr level that I overlooked in the > > > > documentation of these patches? > > > > > > You can't really implement encryption transparently inside an smgr > without > > > significant downsides. You need a way to store an initialization vector > > > associated with the page (or you can store that elsewhere, but then > you've > > > doubled the worst cse amount of random reads/writes). The patch uses > the LSN > > > as the IV (which I doubt is a good idea). For authenticated encryption > further > > > additional storage space is required. > > > > I am unaware of any user of the smgr API that doesn't also use the > > buffer cache, and thus implicitly the Page layout with PageHeader > > [^1] > > Everything indeed uses a PageHeader - but there are a number of places > that do > *not* utilize pd_lower/upper/special. E.g. visibilitymap.c just assumes > that > those fields are zero - and changing that wouldn't be trivial / free, > because > we do a lot of bitmasking/shifting with constants derived from > > #define MAPSIZE (BLCKSZ - MAXALIGN(SizeOfPageHeaderData)) > > which obviously wouldn't be constant anymore if you could reserve space on > the > page. > While not constants, I was able to get this working with variable values here in a way that did not have the overhead of the original patch for the vismap specifically using Montgomery Multiplication for division/mod. This was actually the heaviest of the changes from moving to runtime-calculated, so we might be able to use this approach in this specific case even if only this change is required for this specific fork. > > The API of smgr is also tailored to page-sized quanta of data > > with mostly relation-level information. I don't see why there would be > > a veil covering the layout of Page for smgr when all other information > > already points to the use of PageHeader and Page layouts. In my view, > > it would even make sense to allow the smgr to get exclusive access to > > some part of the page in the current Page layout. > > > > Yes, I agree that there will be an impact on usable page size if you > > want authenticated encryption, and that AMs will indeed need to > > account for storage space now being used by the smgr - inconvenient, > > but it serves a purpose. That would happen regardless of whether smgr > > or some higher system decides where to store the data for encryption - > > as long as it is on the page, the AM effectively can't use those > > bytes. > > But I'd say that's best solved by making the Page documentation and > > PageInit API explicit about the potential use of that space by the > > chosen storage method (encrypted, plain, ...) instead of requiring the > > various AMs to manually consider encryption when using Postgres' APIs > > for writing data to disk without hitting shared buffers; page space > > management is already a task of AMs, but handling the actual > > encryption is not. > > I don't particularly disagree with any detail here - but to me reserving > space > for nonces etc at PageInit() time pretty much is the opposite of handling > encryption inside smgr. > Originally, I was anticipating that we might want different space amounts reserved on different classes of pages (apart from encryption), so while we'd be storing the default page reserved size in pg_control we'd not be limited to this in the structure of the page calls. We could presumably just move the logic into PageInit() itself if every reserved allocation is the same and individual call sites wouldn't need to know about it. The call sites do have more context as to the requirements of the page or the "type" of page in play, which if we made it dependent on page type would need to get passed in somehow, which was where the reserved_page_size parameter came in to the current patch. > > > Should the AM really care whether the data on disk is encrypted or > > not? I don't think so. When the disk contains encrypted bytes, but > > smgrread() and smgrwrite() both produce and accept plaintext data, > > who's going to complain? Requiring AMs to be mindful about encryption > > on all common paths only adds pitfalls where encryption would be > > forgotten by the developer of AMs in one path or another. > > I agree with that - I think the way the
Re: Moving forward with TDE [PATCH v3]
Hi, On 2023-11-06 11:26:44 +0100, Matthias van de Meent wrote: > On Sat, 4 Nov 2023 at 03:38, Andres Freund wrote: > > On 2023-11-02 22:09:40 +0100, Matthias van de Meent wrote: > > > I'm quite surprised at the significant number of changes being made > > > outside the core storage manager files. I thought that changing out > > > mdsmgr with an encrypted smgr (that could wrap mdsmgr if so desired) > > > would be the most obvious change to implement cluster-wide encryption > > > with the least code touched, as relations don't need to know whether > > > the files they're writing are encrypted, right? Is there a reason to > > > not implement this at the smgr level that I overlooked in the > > > documentation of these patches? > > > > You can't really implement encryption transparently inside an smgr without > > significant downsides. You need a way to store an initialization vector > > associated with the page (or you can store that elsewhere, but then you've > > doubled the worst cse amount of random reads/writes). The patch uses the LSN > > as the IV (which I doubt is a good idea). For authenticated encryption > > further > > additional storage space is required. > > I am unaware of any user of the smgr API that doesn't also use the > buffer cache, and thus implicitly the Page layout with PageHeader > [^1] Everything indeed uses a PageHeader - but there are a number of places that do *not* utilize pd_lower/upper/special. E.g. visibilitymap.c just assumes that those fields are zero - and changing that wouldn't be trivial / free, because we do a lot of bitmasking/shifting with constants derived from #define MAPSIZE (BLCKSZ - MAXALIGN(SizeOfPageHeaderData)) which obviously wouldn't be constant anymore if you could reserve space on the page. > The API of smgr is also tailored to page-sized quanta of data > with mostly relation-level information. I don't see why there would be > a veil covering the layout of Page for smgr when all other information > already points to the use of PageHeader and Page layouts. In my view, > it would even make sense to allow the smgr to get exclusive access to > some part of the page in the current Page layout. > > Yes, I agree that there will be an impact on usable page size if you > want authenticated encryption, and that AMs will indeed need to > account for storage space now being used by the smgr - inconvenient, > but it serves a purpose. That would happen regardless of whether smgr > or some higher system decides where to store the data for encryption - > as long as it is on the page, the AM effectively can't use those > bytes. > But I'd say that's best solved by making the Page documentation and > PageInit API explicit about the potential use of that space by the > chosen storage method (encrypted, plain, ...) instead of requiring the > various AMs to manually consider encryption when using Postgres' APIs > for writing data to disk without hitting shared buffers; page space > management is already a task of AMs, but handling the actual > encryption is not. I don't particularly disagree with any detail here - but to me reserving space for nonces etc at PageInit() time pretty much is the opposite of handling encryption inside smgr. > Should the AM really care whether the data on disk is encrypted or > not? I don't think so. When the disk contains encrypted bytes, but > smgrread() and smgrwrite() both produce and accept plaintext data, > who's going to complain? Requiring AMs to be mindful about encryption > on all common paths only adds pitfalls where encryption would be > forgotten by the developer of AMs in one path or another. I agree with that - I think the way the patch currently is designed is not right. There's other stuff you can't trivially do at the smgr level. E.g. if checksums or encryption is enabled, you need to copy the buffer to compute checksums / do IO if in shared buffers, because somebody could set a hint bit even with just a shared content lock. But you don't need that when coming from private buffers during index builds. > I think that getting PageInit to allocate the smgr-specific area would > take some effort, too (which would potentially require adding some > relational context to PageInit, so that it knows which page of which > relation it is going to initialize), but IMHO that would be more > natural than requiring all index and table AMs to be aware the actual > encryption of its pages and require manual handling of that encryption > when the page needs to be written to disk, when it otherwise already > conforms to the various buffer management and file extension APIs > currently in use in PostgreSQL. I would expect "transparent" data > encryption to be handled at the file write layer (i.e. smgr), not > inside the AMs. As mentioned above - I agree that the relevant code shouldn't be in index AMs. But I somewhat doubt that smgr is the right level either. For one, the place computing checksums needs awareness of locking / sharing
Re: Moving forward with TDE [PATCH v3]
Hi, On 2023-11-06 09:56:37 -0500, Stephen Frost wrote: > * Andres Freund (and...@anarazel.de) wrote: > > I still am quite quite unconvinced that using the LSN as a nonce is a good > > design decision. > > This is a really important part of the overall path to moving this > forward, so I wanted to jump to it and have a specific discussion > around this. I agree that there are downsides to using the LSN, some of > which we could possibly address (eg: include the timeline ID in the IV), > but others that would be harder to deal with. > The question then is- what's the alternative? > > One approach would be to change the page format to include space for an > explicit nonce. I don't see the community accepting such a new page > format as the only format we support though as that would mean no > pg_upgrade support along with wasted space if TDE isn't being used. Right. > Ideally, we'd instead be able to support multiple page formats where > users could decide when they create their cluster what features they > want- and luckily we even have such an effort underway with patches > posted for review [1]. I think there are some details wrong with that patch - IMO the existing macros should just continue to work as-is and instead the places that want the more narrow definition should be moved to the new macros and it changes places that should continue to use compile time constants - but it doesn't seem like a fundamentally bad idea to me. I certainly like it much better than making the page size runtime configurable. (I'll try to reply with the above points to [1]) > Certainly, with the base page-special-feature patch, we could have an option > for users to choose that they want a better nonce than the LSN, or we could > bundle that assumption in with, say, the authenticated-encryption feature > (if you want authenticated encryption, then you want more from the > encryption system than the basics, and therefore we presume you also want a > better nonce than the LSN). I don't think we should support using the LSN as a nonce if we have an alternative. The cost and complexity overhead is just not worth it. Yes, it'll be harder for users to migrate to encryption, but adding complexity elsewhere in the system to get an inferior result isn't worth it. > Another approach would be a separate fork, but that then has a number of > downsides too- every write has to touch that too, and a single page of > nonces would cover a pretty large number of pages also. Yea, the costs of doing so is nontrivial. If you were trying to implement encryption on the smgr level - which I doubt you should but am not certain about - my suggestion would be to interleave pages with metadata like nonces and AEAD with the data pages. I.e. one metadata page would be followed by (BLCKSZ - SizeOfPageHeaderData) / (sizeof(nonce) + sizeof(AEAD)) pages containing actual relation data. That way you still get decent locality during scans and writes. Relation forks were a mistake, we shouldn't use them in more places. I think it'd be much better if we also encrypted forks, rather than just the main fork... Greetings, Andres Freund
Re: Moving forward with TDE [PATCH v3]
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Mon, Nov 6, 2023 at 09:56:37AM -0500, Stephen Frost wrote: > > The gist is, without a suggestion of things to try, we're left > > to our own devices to try and figure out things which might be > > successful, only to have those turned down too when we come back with > > them, see [1] for what feels like an example of this. Given your > > feedback overall, which I'm very thankful for, I'm hopeful that you see > > that this is, indeed, a useful feature that people are asking for and > > therefore are willing to spend some time on it, but if the feedback is > > that nothing on the page is acceptable to use for the nonce, we can't > > put the nonce somewhere else, and we can't change the page format, then > > everything else is just moving deck chairs around on the titanic that > > has been this effort. > > Yeah, I know the feeling, though I thought XTS was immune enough to > nonce/LSN reuse that it was acceptable. Ultimately it depends on the attack vector you're trying to address, but generally, I think you're right about the XTS tweak reuse not being that big of a deal. XTS isn't CTR or GCM. With FDE (full disk encryption) you're expecting the attacker to steal the physical laptop, hard drive, etc, generally, and so the downside of using the same tweak with XTS over and over again isn't that bad (and is what most FDE solutions do, aiui, by simply using the sector number; we could do something similar to that by using the relfilenode + block number) because that re-use is a problem if the attacker is able to see multiple copies of the same block over time where the block has been encrypted with different data but the same key and tweak. Using the LSN actually is better than what the FDE solutions do because the LSN varies much more often than the sector number. Sure, it doesn't change with every write and maybe an attacker could glean something from that, but that's quite narrow. The downside from the LSN based approach with XTS is probably more that using the LSN means that we can't encrypt the LSN itself and that is a leak too- but then again, we leak that through the simple WAL filenames too, to some extent, so it doesn't strike me as a huge issue. XTS as a block cipher doesn't suffer from the IV-reuse issue that you have with streaming ciphers where the same key+IV and different data leads to being able to trivally retrive the plaintext though and I worry that maybe that's what people were thinking. The README and comments I don't think were terribly clear on this and I think may have even been from back when CTR was being considered, where IV reuse would have resulted in plaintext being trivially available. > What got me sunk on the feature was the complexity of adding temporary > file encryption support and that tipped the scales in the negative for > me in community value of the feature vs. added complexity. (Yeah, I used > a Titanic reference in the last sentence. ;-) ) However, I am open to > the community value and complexity values changing over time. My blog > post on the topic: We do need to address the temporary file situation too and we do have a bit of an issue that how we deal with temporary files today in PG isn't very consistent and there's too many ways to do that. There's a patch that works on that, though it has some bitrot that we're working on addressing currently. There is value in simply fixing that situation wrt temporary file management independent of encryption, though of course then encryption of those temporary files becomes much simpler. Thanks, Stephen signature.asc Description: PGP signature
Re: Moving forward with TDE [PATCH v3]
On Fri, Nov 3, 2023 at 9:53 PM Andres Freund wrote: > On 2023-11-02 19:32:28 -0700, Andres Freund wrote: > > > From 327e86d52be1df8de9c3a324cb06b85ba5db9604 Mon Sep 17 00:00:00 2001 > > > From: David Christensen > > > Date: Fri, 29 Sep 2023 15:16:00 -0400 > > > Subject: [PATCH v3 5/5] Add encrypted/authenticated WAL > > > > > > When using an encrypted cluster, we need to ensure that the WAL is also > > > encrypted. While we could go with an page-based approach, we use > instead a > > > per-record approach, using GCM for the encryption method and storing > the AuthTag > > > in the xl_crc field. > > What was the reason for this decision? > This was mainly to prevent IV reuse by using a per-record encryption rather than per-page, since partial writes out on the WAL buffer would result in reuse there. This was somewhat of an experiment since authenticated data per record was basically equivalent in function to the CRC. There was a switch here so normal clusters use the crc field with the existing CRC implementation, only encrypted clusters use this alternate approach.
Re: Moving forward with TDE [PATCH v3]
Hi, thanks for the detailed feedback here. I do think it's worth addressing the question Stephen raised as far as what we use for the IV[1]; whether LSN or something else entirely, and if so what. The choice of LSN here is fairly fundamental to the existing implementation, so if we decide to do something different some of this might be moot. Best, David [1] https://www.mail-archive.com/pgsql-hackers@lists.postgresql.org/msg154613.html
Re: Moving forward with TDE [PATCH v3]
On Mon, Nov 6, 2023 at 09:56:37AM -0500, Stephen Frost wrote: > The gist is, without a suggestion of things to try, we're left > to our own devices to try and figure out things which might be > successful, only to have those turned down too when we come back with > them, see [1] for what feels like an example of this. Given your > feedback overall, which I'm very thankful for, I'm hopeful that you see > that this is, indeed, a useful feature that people are asking for and > therefore are willing to spend some time on it, but if the feedback is > that nothing on the page is acceptable to use for the nonce, we can't > put the nonce somewhere else, and we can't change the page format, then > everything else is just moving deck chairs around on the titanic that > has been this effort. Yeah, I know the feeling, though I thought XTS was immune enough to nonce/LSN reuse that it was acceptable. What got me sunk on the feature was the complexity of adding temporary file encryption support and that tipped the scales in the negative for me in community value of the feature vs. added complexity. (Yeah, I used a Titanic reference in the last sentence. ;-) ) However, I am open to the community value and complexity values changing over time. My blog post on the topic: https://momjian.us/main/blogs/pgblog/2023.html#October_19_2023 -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: Moving forward with TDE [PATCH v3]
On Thu, Nov 2, 2023 at 07:32:28PM -0700, Andres Freund wrote: > On 2023-10-31 16:23:17 -0500, David Christensen wrote: > > +Implementation > > +-- > > + > > +To enable cluster file encryption, the initdb option > > +--cluster-key-command must be used, which specifies a command to > > +retrieve the KEK. > > FWIW, I think "cluster file encryption" is somewhat ambiguous. That could also > mean encrypting on the file system level or such. We could call it: * cluster data file encryption * cluster data encryption * database cluster encryption -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: Moving forward with TDE [PATCH v3]
Greetings, Thanks for your feedback on this. * Andres Freund (and...@anarazel.de) wrote: > I still am quite quite unconvinced that using the LSN as a nonce is a good > design decision. This is a really important part of the overall path to moving this forward, so I wanted to jump to it and have a specific discussion around this. I agree that there are downsides to using the LSN, some of which we could possibly address (eg: include the timeline ID in the IV), but others that would be harder to deal with. The question then is- what's the alternative? One approach would be to change the page format to include space for an explicit nonce. I don't see the community accepting such a new page format as the only format we support though as that would mean no pg_upgrade support along with wasted space if TDE isn't being used. Ideally, we'd instead be able to support multiple page formats where users could decide when they create their cluster what features they want- and luckily we even have such an effort underway with patches posted for review [1]. Certainly, with the base page-special-feature patch, we could have an option for users to choose that they want a better nonce than the LSN, or we could bundle that assumption in with, say, the authenticated-encryption feature (if you want authenticated encryption, then you want more from the encryption system than the basics, and therefore we presume you also want a better nonce than the LSN). Another approach would be a separate fork, but that then has a number of downsides too- every write has to touch that too, and a single page of nonces would cover a pretty large number of pages also. Ultimately, I agree with you that the LSN isn't perfect and we really shouldn't be calling it 'ideal' as it isn't, and we can work to fix that language in the patch, but the lack of any alternative being proposed that might be acceptable makes this feel a bit like rock management [2]. My apologies if there's something good that's already been specifically pushed and I just missed it; if so, a link to that suggestion and discussion would be greatly appreciated. Thanks again! Stephen [1]: https://commitfest.postgresql.org/45/3986/ [2]: https://en.wikipedia.org/wiki/Wikipedia:Bring_me_a_rock ; though that isn't great for a quick summary (which I tried to find on an isolated page somewhere and didn't). The gist is, without a suggestion of things to try, we're left to our own devices to try and figure out things which might be successful, only to have those turned down too when we come back with them, see [1] for what feels like an example of this. Given your feedback overall, which I'm very thankful for, I'm hopeful that you see that this is, indeed, a useful feature that people are asking for and therefore are willing to spend some time on it, but if the feedback is that nothing on the page is acceptable to use for the nonce, we can't put the nonce somewhere else, and we can't change the page format, then everything else is just moving deck chairs around on the titanic that has been this effort. signature.asc Description: PGP signature
Re: Moving forward with TDE [PATCH v3]
On Sat, 4 Nov 2023 at 03:38, Andres Freund wrote: > > Hi, > > On 2023-11-02 22:09:40 +0100, Matthias van de Meent wrote: > > I'm quite surprised at the significant number of changes being made > > outside the core storage manager files. I thought that changing out > > mdsmgr with an encrypted smgr (that could wrap mdsmgr if so desired) > > would be the most obvious change to implement cluster-wide encryption > > with the least code touched, as relations don't need to know whether > > the files they're writing are encrypted, right? Is there a reason to > > not implement this at the smgr level that I overlooked in the > > documentation of these patches? > > You can't really implement encryption transparently inside an smgr without > significant downsides. You need a way to store an initialization vector > associated with the page (or you can store that elsewhere, but then you've > doubled the worst cse amount of random reads/writes). The patch uses the LSN > as the IV (which I doubt is a good idea). For authenticated encryption further > additional storage space is required. I am unaware of any user of the smgr API that doesn't also use the buffer cache, and thus implicitly the Page layout with PageHeader [^1]. The API of smgr is also tailored to page-sized quanta of data with mostly relation-level information. I don't see why there would be a veil covering the layout of Page for smgr when all other information already points to the use of PageHeader and Page layouts. In my view, it would even make sense to allow the smgr to get exclusive access to some part of the page in the current Page layout. Yes, I agree that there will be an impact on usable page size if you want authenticated encryption, and that AMs will indeed need to account for storage space now being used by the smgr - inconvenient, but it serves a purpose. That would happen regardless of whether smgr or some higher system decides where to store the data for encryption - as long as it is on the page, the AM effectively can't use those bytes. But I'd say that's best solved by making the Page documentation and PageInit API explicit about the potential use of that space by the chosen storage method (encrypted, plain, ...) instead of requiring the various AMs to manually consider encryption when using Postgres' APIs for writing data to disk without hitting shared buffers; page space management is already a task of AMs, but handling the actual encryption is not. Should the AM really care whether the data on disk is encrypted or not? I don't think so. When the disk contains encrypted bytes, but smgrread() and smgrwrite() both produce and accept plaintext data, who's going to complain? Requiring AMs to be mindful about encryption on all common paths only adds pitfalls where encryption would be forgotten by the developer of AMs in one path or another. > To be able to to use the LSN as the IV, the patch needs to ensure that the LSN > increases in additional situations (a more aggressive version of > wal_log_hint_bits) - which can't be done below smgr, where we don't know about > what WAL logging was done. Nor can you easily just add space on the page below > md.c, for the purpose of storing an LSN independent IV and the authentication > data. I think that getting PageInit to allocate the smgr-specific area would take some effort, too (which would potentially require adding some relational context to PageInit, so that it knows which page of which relation it is going to initialize), but IMHO that would be more natural than requiring all index and table AMs to be aware the actual encryption of its pages and require manual handling of that encryption when the page needs to be written to disk, when it otherwise already conforms to the various buffer management and file extension APIs currently in use in PostgreSQL. I would expect "transparent" data encryption to be handled at the file write layer (i.e. smgr), not inside the AMs. Kind regards, Matthias van de Meent [^1] ReadBuffer_common uses PageIsVerifiedExtended which verifies that a page conforms with Postgres' Page layout if checksums are enabled. Furthermore, all builtin index AMs utilize pd_special, further implying the use of a PageInit/PageHeader-based page layout. Additionally, the heap tableAM also complies, and both FSM and VM also use postgres' Page layout. As for other AMs that I could check: bloom, rum, and pgvector's ivfflat and hnsw all use page layouts.
Re: Moving forward with TDE [PATCH v3]
On 2023-11-02 19:32:28 -0700, Andres Freund wrote: > > From 327e86d52be1df8de9c3a324cb06b85ba5db9604 Mon Sep 17 00:00:00 2001 > > From: David Christensen > > Date: Fri, 29 Sep 2023 15:16:00 -0400 > > Subject: [PATCH v3 5/5] Add encrypted/authenticated WAL > > > > When using an encrypted cluster, we need to ensure that the WAL is also > > encrypted. While we could go with an page-based approach, we use instead a > > per-record approach, using GCM for the encryption method and storing the > > AuthTag > > in the xl_crc field. What was the reason for this decision? ?
Re: Moving forward with TDE [PATCH v3]
Hi, On 2023-11-02 22:09:40 +0100, Matthias van de Meent wrote: > I'm quite surprised at the significant number of changes being made > outside the core storage manager files. I thought that changing out > mdsmgr with an encrypted smgr (that could wrap mdsmgr if so desired) > would be the most obvious change to implement cluster-wide encryption > with the least code touched, as relations don't need to know whether > the files they're writing are encrypted, right? Is there a reason to > not implement this at the smgr level that I overlooked in the > documentation of these patches? You can't really implement encryption transparently inside an smgr without significant downsides. You need a way to store an initialization vector associated with the page (or you can store that elsewhere, but then you've doubled the worst cse amount of random reads/writes). The patch uses the LSN as the IV (which I doubt is a good idea). For authenticated encryption further additional storage space is required. To be able to to use the LSN as the IV, the patch needs to ensure that the LSN increases in additional situations (a more aggressive version of wal_log_hint_bits) - which can't be done below smgr, where we don't know about what WAL logging was done. Nor can you easily just add space on the page below md.c, for the purpose of storing an LSN independent IV and the authentication data. You could decide that the security that XTS provides is sufficient (XTS could be used without a real IV, with some downsides), but we'd pretty much prevent ourselves from ever implementing authenticated encryption. Greetings, Andres Freund
Re: Moving forward with TDE [PATCH v3]
Hi,
On 2023-10-31 16:23:17 -0500, David Christensen wrote:
> The patches are as follows:
>
> 0001 - doc updates
> 0002 - Basic key management and cipher support
> 0003 - Backend-related changes to support heap encryption
> 0004 - modifications to bin tools and programs to manage key rotation and
> add other knowledge
> 0005 - Encrypted/authenticated WAL
>
> These are very broad strokes at this point and should be split up a bit
> more to make things more granular and easier to review, but I wanted to get
> this update out.
Yes, particularly 0003 really needs to be split - as is it's not easily
reviewable.
> From 327e86d52be1df8de9c3a324cb06b85ba5db9604 Mon Sep 17 00:00:00 2001
> From: David Christensen
> Date: Fri, 29 Sep 2023 15:16:00 -0400
> Subject: [PATCH v3 5/5] Add encrypted/authenticated WAL
>
> When using an encrypted cluster, we need to ensure that the WAL is also
> encrypted. While we could go with an page-based approach, we use instead a
> per-record approach, using GCM for the encryption method and storing the
> AuthTag
> in the xl_crc field.
>
> We change the xl_crc field to instead be a union struct, with a compile-time
> adjustable size to allow us to customize the number of bytes allocated to the
> GCM authtag. This allows us to easily adjust the size of bytes needed to
> support our authentication. (Testing has included up to 12, but leaving at
> this
> point to 4 due to keeping the size of the WAL records the same.)
Ugh, that'll be quite a bit of overhead in some workloads... You can't really
use such a build for non-encrypted workloads, making this a not very
deployable path...
> @@ -905,20 +905,28 @@ XLogInsertRecord(XLogRecData *rdata,
> {
> /*
>* Now that xl_prev has been filled in, calculate CRC of the
> record
> - * header.
> + * header. If we are using encrypted WAL, this CRC is
> overwritten by
> + * the authentication tag, so just zero
>*/
> - rdata_crc = rechdr->xl_crc;
> - COMP_CRC32C(rdata_crc, rechdr, offsetof(XLogRecord, xl_crc));
> - FIN_CRC32C(rdata_crc);
> - rechdr->xl_crc = rdata_crc;
> + if (!encrypt_wal)
> + {
> + rdata_crc = rechdr->xl_integrity.crc;
> + COMP_CRC32C(rdata_crc, rechdr, offsetof(XLogRecord,
> xl_integrity.crc));
> + FIN_CRC32C(rdata_crc);
> + rechdr->xl_integrity.crc = rdata_crc;
> + }
> + else
> + memset(>xl_integrity, 0,
> sizeof(rechdr->xl_integrity));
Why aren't you encrypting most of the data here? Just as for CRC computation,
encrypting a large record in XLOG_BLCKSZ
> * XLogRecordDataHeaderLong structs all begin with a single 'id' byte. It's
> * used to distinguish between block references, and the main data structs.
> */
> +
> +#define XL_AUTHTAG_SIZE 4
> +#define XL_HEADER_PAD 2
> +
> typedef struct XLogRecord
> {
> uint32 xl_tot_len; /* total len of entire record */
> @@ -45,14 +49,16 @@ typedef struct XLogRecord
> XLogRecPtr xl_prev;/* ptr to previous record in
> log */
> uint8 xl_info;/* flag bits, see below */
> RmgrId xl_rmid;/* resource manager for this
> record */
> - /* 2 bytes of padding here, initialize to zero */
> - pg_crc32c xl_crc; /* CRC for this record */
> -
> + uint8 xl_pad[XL_HEADER_PAD]; /* required alignment
> padding */
What does "required" mean here? And why is this defined in a separate define?
And why do we need the explicit field here at all? The union will still
provide sufficient alignment for a uint32.
It also doesn't seem right to remove the comment about needing to zero out the
space.
> From 7557acf60f52da4a86fd9f902bab4804c028dd4b Mon Sep 17 00:00:00 2001
> From: David Christensen
> Date: Tue, 31 Oct 2023 15:24:02 -0400
> Subject: [PATCH v3 3/5] Backend-related changes
> --- a/src/backend/access/heap/rewriteheap.c
> +++ b/src/backend/access/heap/rewriteheap.c
> @@ -323,6 +323,11 @@ end_heap_rewrite(RewriteState state)
> state->rs_buffer,
> true);
>
> + PageEncryptInplace(state->rs_buffer, MAIN_FORKNUM,
> +
> RelationIsPermanent(state->rs_new_rel),
> +state->rs_blockno,
> +
> RelationGetSmgr(state->rs_new_rel)->smgr_rlocator.locator.relNumber
> + );
> PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
>
> smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,
I don't think it's ok to have to make such changes in a
Re: Moving forward with TDE [PATCH v3]
On Tue, 31 Oct 2023 at 22:23, David Christensen wrote: > > Greetings, > > I am including an updated version of this patch series; it has been rebased > onto 6ec62b7799 and reworked somewhat. > > The patches are as follows: > > 0001 - doc updates > 0002 - Basic key management and cipher support > 0003 - Backend-related changes to support heap encryption I'm quite surprised at the significant number of changes being made outside the core storage manager files. I thought that changing out mdsmgr with an encrypted smgr (that could wrap mdsmgr if so desired) would be the most obvious change to implement cluster-wide encryption with the least code touched, as relations don't need to know whether the files they're writing are encrypted, right? Is there a reason to not implement this at the smgr level that I overlooked in the documentation of these patches? Kind regards, Matthias van de Meent Neon (https://neon.tech)
Re: Moving forward with TDE [PATCH v3]
On Tue, Oct 31, 2023 at 04:32:38PM -0500, David Christensen wrote: > On Tue, Oct 31, 2023 at 4:30 PM Bruce Momjian wrote: > Temporary /files/ are handled in a different patch set and are not included > here (not sure of the status of integrating at this point). I believe that > this patch should handle temporary heap files just fine since they go through > the Page API. Yes, that's what I thought, thanks. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: Moving forward with TDE [PATCH v3]
On Tue, Oct 31, 2023 at 4:30 PM Bruce Momjian wrote: > On Tue, Oct 31, 2023 at 04:23:17PM -0500, David Christensen wrote: > > Greetings, > > > > I am including an updated version of this patch series; it has been > rebased > > onto 6ec62b7799 and reworked somewhat. > > > > The patches are as follows: > > > > 0001 - doc updates > > 0002 - Basic key management and cipher support > > 0003 - Backend-related changes to support heap encryption > > 0004 - modifications to bin tools and programs to manage key rotation > and add > > other knowledge > > 0005 - Encrypted/authenticated WAL > > > > These are very broad strokes at this point and should be split up a bit > more to > > make things more granular and easier to review, but I wanted to get this > update > > out. > > This lacks temp table file encryption, right? Temporary /files/ are handled in a different patch set and are not included here (not sure of the status of integrating at this point). I believe that this patch should handle temporary heap files just fine since they go through the Page API.
Re: Moving forward with TDE [PATCH v3]
On Tue, Oct 31, 2023 at 04:23:17PM -0500, David Christensen wrote: > Greetings, > > I am including an updated version of this patch series; it has been rebased > onto 6ec62b7799 and reworked somewhat. > > The patches are as follows: > > 0001 - doc updates > 0002 - Basic key management and cipher support > 0003 - Backend-related changes to support heap encryption > 0004 - modifications to bin tools and programs to manage key rotation and add > other knowledge > 0005 - Encrypted/authenticated WAL > > These are very broad strokes at this point and should be split up a bit more > to > make things more granular and easier to review, but I wanted to get this > update > out. This lacks temp table file encryption, right? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: Moving forward with TDE
On Tue, Mar 28, 2023 at 8:35 AM Bruce Momjian wrote: > On Tue, Mar 28, 2023 at 02:03:50AM +0200, Stephen Frost wrote: > > The remote storage is certainly an independent system. Multi-mount LUNs > are > > entirely possible in a SAN (and absolutely with NFS, or just the NFS > server > > itself is compromised..), so while the attacker may not have any access > to the > > database server itself, they may have access to these other systems, and > that’s > > not even considering in-transit attacks which are also absolutely > possible, > > especially with iSCSI or NFS. > > > > I don’t understand what is being claimed that the remote storage is “not > an > > independent system” based on my understanding of, eg, NFS. With NFS, a > > directory on the NFS server is exported and the client mounts that > directory as > > NFS locally, all over a network which may or may not be secured against > > manipulation. A user on the NFS server with root access is absolutely > able to > > access and modify files on the NFS server trivially, even if they have no > > access to the PG server. Would you explain what you mean? > > The point is that someone could change values in the storage, pg_xact, > encryption settings, binaries, that would allow the attacker to learn > the encryption key. This is not possible for two secure endpoints and > someone changing data in transit. Yeah, it took me a while to > understand these boundaries too. > > > So the idea is that the backup user can be compromised without the > data > > being vulnerable --- makes sense, though that use-case seems narrow. > > > > That’s perhaps a fair consideration- but it’s clearly of enough value > that many > > of our users are asking for it and not using PG because we don’t have it > today. > > Ultimately though, this clearly makes it more than a “checkbox” feature. > I hope > > we are able to agree on that now. > > It is more than a check box feature, yes, but I am guessing few people > are wanting the this for the actual features beyond check box. > > > Yes, there is value beyond the check-box, but in most cases those > > values are limited considering the complexity of the features, and > the > > check-box is what most people are asking for, I think. > > > > For the users who ask on the lists for this feature, regularly, how many > don’t > > ask because they google or find prior responses on the list to the > question of > > if we have this capability? How do we know that their cases are > “checkbox”? > > Because I have rarely heard people articulate the value beyond check > box. > I think there is value. I am going to try to articulate a case for this here. The first is that if people just want a "checkbox" then they can implement PostgreSQL in ways that have encryption at rest today. This includes using LUKS and the encryption options in pgbackrest. That's good enough for a checkbox. It isn't good enough for a real, secured instance however. There are a few problems with trying to do this for a secured instance. The first is that you have multiple links in the encryption chain, and the failure of any one of them ill lead to cleartext exposure of data files. This is not a problem for those who just want to tick a checkbox. Also the fact that backups and main systems are separately encrypted there (if the backups are encrypted at all) means that people have to choose between complicating a restore process and simply ditching encryption on the backup, which makes the checkbox somewhat pointless. Where I have usually seen this come up is in the question of "how do you prevent the problem of someone pulling storage devices from your servers and taking them away to compromise your data?" Physical security comes into it but often times people want more than that as an answer. I saw questions like that from external auditors when I was at Adjust. If you want to actually address that problem, then the current tooling is quite cumbersome. Yes you can do it, but it is very hard to make sure it has been fully secured and also very hard to monitor. TDE would make the setup and verification of this much easier. And in particular it solves a number of other issues that I can see arising from LUKS and similar approaches since it doesn't rely on the kernel to be able to translate plain text to and from cypher text. I have actually worked with folks who have PII and need to protect it and who currently use LUKS and pg_backrest to do so. I would be extremely happy to see TDE replace those for their needs. I can imagine that those who hold high value data would use it as well instead of these other more error prone and less secure setups. > > > Consider that there are standards groups which explicitly consider these > attack > > vectors and consider them important enough to require mitigations to > address > > those vectors. Do the end users of PG understand the attack vectors or > why they > > matter? Perhaps not, but just
Re: Moving forward with TDE
On Tue, Mar 28, 2023 at 5:02 AM Stephen Frost wrote: > > > There's clearly user demand for it as there's a number of organizations >> > who have forks which are providing it in one shape or another. This >> > kind of splintering of the community is actually an actively bad thing >> > for the project and is part of what killed Unix, by at least some pretty >> > reputable accounts, in my view. >> >> Yes, the number of commercial implementations of this is a concern. Of >> course, it is also possible that those commercial implementations are >> meeting checkbox requirements rather than technical ones, and the >> community has been hostile to check box-only features. > > > I’ve grown weary of this argument as the other major piece of work it was > routinely applied to was RLS and yet that has certainly been seen broadly > as a beneficial feature with users clearly leveraging it and in more than > some “checkbox” way. > > Indeed, it’s similar also in that commercial implementations were done of > RLS while there were arguments made about it being a checkbox feature which > were used to discourage it from being implemented in core. Were it truly > checkbox, I don’t feel we would have the regular and ongoing discussion > about it on the lists that we do, nor see other tools built on top of PG > which specifically leverage it. Perhaps there are truly checkbox features > out there which we will never implement, but I’m (perhaps due to what my > dad would call selective listening on my part, perhaps not) having trouble > coming up with any presently. Features that exist in other systems that we > don’t want? Certainly. We don’t characterize those as simply “checkbox” > though. Perhaps that’s in part because we provide alternatives- but that’s > not the case here. We have no comparable way to have this capability as > part of the core system. > > We, as a community, are clearly losing value by lack of this capability, > if by no other measure than simply the numerous users of the commercial > implementations feeling that they simply can’t use PG without this feature, > for whatever their reasoning. > I also think this is something of a problem because very few requirements are actually purely technical requirements, and I think the issue is that in many cases there are ways around the lack of the feature. So I would phrase this differently. What is the value of doing this in core? This dramatically simplifies the question of setting up a PostgreSQL environment that is properly protected with encryption at rest. That in itself is valuable. Today you can accomplish something similar with encrypted filesystems and encryption options in things like pgbackrest. However these are many different pieces of a solution and missing up the setup of any one of them can compromise the data. Having a single point of encryption and decryption means fewer opportunities to mess it up and that means less risk. This in turn makes it easier to settle on using PostgreSQL. There are certainly going to be those who approach encryption at rest as a checkbox item and who don't really care if there are holes in it. But there are others who really should be concerned (and this is becoming a bigger issue where data privacy, PCI-DSS, and other requirements may come into play), and those need better tooling than we have. I also think that as data privacy becomes a larger issue, this will become a larger topic. Anyway, my contribution to that question. Best Wishes, Chris Travers > > Thanks, > > Stephen > -- Best Wishes, Chris Travers Efficito: Hosted Accounting and ERP. Robust and Flexible. No vendor lock-in. http://www.efficito.com/learn_more
Re: Moving forward with TDE
Greetings, On Mon, Mar 27, 2023 at 21:35 Bruce Momjian wrote: > On Tue, Mar 28, 2023 at 02:03:50AM +0200, Stephen Frost wrote: > > The remote storage is certainly an independent system. Multi-mount LUNs > are > > entirely possible in a SAN (and absolutely with NFS, or just the NFS > server > > itself is compromised..), so while the attacker may not have any access > to the > > database server itself, they may have access to these other systems, and > that’s > > not even considering in-transit attacks which are also absolutely > possible, > > especially with iSCSI or NFS. > > > > I don’t understand what is being claimed that the remote storage is “not > an > > independent system” based on my understanding of, eg, NFS. With NFS, a > > directory on the NFS server is exported and the client mounts that > directory as > > NFS locally, all over a network which may or may not be secured against > > manipulation. A user on the NFS server with root access is absolutely > able to > > access and modify files on the NFS server trivially, even if they have no > > access to the PG server. Would you explain what you mean? > > The point is that someone could change values in the storage, pg_xact, > encryption settings, binaries, that would allow the attacker to learn > the encryption key. This is not possible for two secure endpoints and > someone changing data in transit. Yeah, it took me a while to > understand these boundaries too. This depends on the specific configuration of the systems, clearly. Being able to change values in other parts of the system isn’t great and we should work to improve on that, but clearly that isn’t so much of an issue that people aren’t willing to accept a partial solution or existing commercial solutions wouldn’t be accepted or considered viable. Indeed, using GCM is objectively an improvement over what’s being offered commonly today. I also generally object to the idea that being able to manipulate the PGDATA directory necessarily means being able to gain access to the KEK. In trivial solutions, sure, it’s possible, but the NFS server should never be asking some external KMS for the key to a given DB server and a reasonable implementation won’t allow this, and instead would flag and log such an attempt for someone to review, leading to a much faster realization of a compromised system. Certainly it’s much simpler to reason about an attacker with no knowledge of either system and only network access to see if they can penetrate the communications between the two end-points, but that is not the only case where authenticated encryption is useful. > So the idea is that the backup user can be compromised without the > data > > being vulnerable --- makes sense, though that use-case seems narrow. > > > > That’s perhaps a fair consideration- but it’s clearly of enough value > that many > > of our users are asking for it and not using PG because we don’t have it > today. > > Ultimately though, this clearly makes it more than a “checkbox” feature. > I hope > > we are able to agree on that now. > > It is more than a check box feature, yes, but I am guessing few people > are wanting the this for the actual features beyond check box. As I explained previously, perhaps the people asking are doing so for only the “checkbox”, but that doesn’t mean it isn’t a useful feature or that it isn’t valuable in its own right. Those checklists were compiled and enforced for a reason, which the end users might not understand but is still absolutely valuable. Sad to say, but frankly this is becoming more and more common but we shouldn’t be faulting the users asking for it- if it were truly useless then eventually it would be removed from the standard, but it hasn’t and it won’t be because, while not every end user has a depth of understanding to explain it, it is actually a useful and important capability to have and one that is important to implement. > Yes, there is value beyond the check-box, but in most cases those > > values are limited considering the complexity of the features, and > the > > check-box is what most people are asking for, I think. > > > > For the users who ask on the lists for this feature, regularly, how many > don’t > > ask because they google or find prior responses on the list to the > question of > > if we have this capability? How do we know that their cases are > “checkbox”? > > Because I have rarely heard people articulate the value beyond check > box. Have I done so sufficiently then that we can agree that calling it “checkbox” is inappropriate and detrimental to our user base? > Consider that there are standards groups which explicitly consider these > attack > > vectors and consider them important enough to require mitigations to > address > > those vectors. Do the end users of PG understand the attack vectors or > why they > > matter? Perhaps not, but just because they can’t articulate the > reasoning does > > NOT mean that the attack vector
Re: Moving forward with TDE
On Tue, Mar 28, 2023 at 02:03:50AM +0200, Stephen Frost wrote: > The remote storage is certainly an independent system. Multi-mount LUNs are > entirely possible in a SAN (and absolutely with NFS, or just the NFS server > itself is compromised..), so while the attacker may not have any access to the > database server itself, they may have access to these other systems, and > that’s > not even considering in-transit attacks which are also absolutely possible, > especially with iSCSI or NFS. > > I don’t understand what is being claimed that the remote storage is “not an > independent system” based on my understanding of, eg, NFS. With NFS, a > directory on the NFS server is exported and the client mounts that directory > as > NFS locally, all over a network which may or may not be secured against > manipulation. A user on the NFS server with root access is absolutely able to > access and modify files on the NFS server trivially, even if they have no > access to the PG server. Would you explain what you mean? The point is that someone could change values in the storage, pg_xact, encryption settings, binaries, that would allow the attacker to learn the encryption key. This is not possible for two secure endpoints and someone changing data in transit. Yeah, it took me a while to understand these boundaries too. > So the idea is that the backup user can be compromised without the data > being vulnerable --- makes sense, though that use-case seems narrow. > > That’s perhaps a fair consideration- but it’s clearly of enough value that > many > of our users are asking for it and not using PG because we don’t have it > today. > Ultimately though, this clearly makes it more than a “checkbox” feature. I > hope > we are able to agree on that now. It is more than a check box feature, yes, but I am guessing few people are wanting the this for the actual features beyond check box. > Yes, there is value beyond the check-box, but in most cases those > values are limited considering the complexity of the features, and the > check-box is what most people are asking for, I think. > > For the users who ask on the lists for this feature, regularly, how many don’t > ask because they google or find prior responses on the list to the question of > if we have this capability? How do we know that their cases are “checkbox”? Because I have rarely heard people articulate the value beyond check box. > Consider that there are standards groups which explicitly consider these > attack > vectors and consider them important enough to require mitigations to address > those vectors. Do the end users of PG understand the attack vectors or why > they > matter? Perhaps not, but just because they can’t articulate the reasoning > does > NOT mean that the attack vector doesn’t exist or that their environment is > somehow immune to it- indeed, as the standards bodies surely know, the > opposite > is true- they’re almost certainly at risk of those attack vectors and > therefore > the standards bodies are absolutely justified in requiring them to provide a > solution. Treating these users as unimportant because they don’t have the > depth > of understanding that we do or that the standards body does is not helping > them- it’s actively driving them away from PG. Well, then who is going to explain them here, because I have not heard them yet. > The RLS arguments were that queries could expoose some of the underlying > data, but in summary, that was considered acceptable. > > This is an excellent point- and dovetails very nicely into my argument that > protecting primary data (what is provided by users and ends up in indexes and > heaps) is valuable even if we don’t (yet..) have protection for other parts of > the system. Reducing the size of the attack vector is absolutely useful, > especially when it’s such a large amount of the data in the system. Yes, we > should, and will, continue to improve- as we do with many features, but we > don’t need to wait for perfection to include this feature, just as with RLS > and > numerous other features we have. The issue is that you needed a certain type of user with a certain type of access to break RLS, while for this, writing to PGDATA is the simple case for all the breakage, and the thing we are protecting with authentication. > > > We, as a community, are clearly losing value by lack of this > capability, > > if by > > > no other measure than simply the numerous users of the commercial > > > implementations feeling that they simply can’t use PG without this > > feature, for > > > whatever their reasoning. > > > > That is true, but I go back to my concern over useful feature vs. > check > > box. > > > > While it’s easy to label something as checkbox, I don’t feel we have > been > fair > > No, actually, it isn't. I am not sure why you are saying that. > >
Re: Moving forward with TDE
Greetings, On Mon, Mar 27, 2023 at 19:19 Bruce Momjian wrote: > On Tue, Mar 28, 2023 at 12:57:42AM +0200, Stephen Frost wrote: > > I consider the operating system and its processes as much more of a > > single entity than TLS over a network. > > > > This may be the case sometimes but there’s absolutely no shortage of > other > > cases and it’s almost more the rule these days, that there is some kind > of > > network between the OS processes and the storage- a SAN, an iSCSI > network, NFS, > > are all quite common. > > Yes, but consider that the database cluster is having to get its data > from that remote storage --- the remote storage is not an independent > entity that can be corrupted without the databaes server being > compromised. If everything in PGDATA was GCM-verified, it would be > secure, but because some parts are not, I don't think it would be. The remote storage is certainly an independent system. Multi-mount LUNs are entirely possible in a SAN (and absolutely with NFS, or just the NFS server itself is compromised..), so while the attacker may not have any access to the database server itself, they may have access to these other systems, and that’s not even considering in-transit attacks which are also absolutely possible, especially with iSCSI or NFS. I don’t understand what is being claimed that the remote storage is “not an independent system” based on my understanding of, eg, NFS. With NFS, a directory on the NFS server is exported and the client mounts that directory as NFS locally, all over a network which may or may not be secured against manipulation. A user on the NFS server with root access is absolutely able to access and modify files on the NFS server trivially, even if they have no access to the PG server. Would you explain what you mean? I do agree that the ideal case would be that we encrypt everything we can (not everything can be for various reasons, but we don’t actually need to either) in the PGDATA directory is encrypted and authenticated, just like it would be ideal if everything was checksum’d and isn’t today. We are progressing in that direction thanks to efforts such as reworking the other subsystems to used shared buffers and a consistent page format, but just like with checksums we do not need to have the perfect solution for us to provide a lot of value here- and our users know that as the same is true of the unauthenticated encryption approaches being offered by the commercial solutions. > > As specific examples, consider: > > > > > > An attack against the database system where the database server is > shut > > down, > > > or a backup, and the encryption key isn’t available on the system. > > > > > > The backup system itself, not running as the PG user (an option > supported > > by PG > > > and at least pgbackrest) being compromised, thus allowing for > injection > > of > > > changes into a backup or into a restore. > > > > I then question why we are not adding encryption to pg_basebackup or > > pgbackrest rather than the database system. > > > > Pgbackrest has encryption and authentication of it … but that doesn’t > actually > > address the attack vector that I outlined. If the backup user is > compromised > > then they can change the data before it gets to the storage. If the > backup > > user is compromised then they have access to whatever key is used to > encrypt > > and authenticate the backup and therefore can trivially manipulate the > data. > > So the idea is that the backup user can be compromised without the data > being vulnerable --- makes sense, though that use-case seems narrow. That’s perhaps a fair consideration- but it’s clearly of enough value that many of our users are asking for it and not using PG because we don’t have it today. Ultimately though, this clearly makes it more than a “checkbox” feature. I hope we are able to agree on that now. > What were the _technical_ reasons for those objections? > > > > I believe largely the ones I’m bringing up here and which I outline > above… I > > don’t mean to pretend that any of this is of my own independent > construction. I > > don’t believe it is and my apologies if it came across that way. > > Yes, there is value beyond the check-box, but in most cases those > values are limited considering the complexity of the features, and the > check-box is what most people are asking for, I think. For the users who ask on the lists for this feature, regularly, how many don’t ask because they google or find prior responses on the list to the question of if we have this capability? How do we know that their cases are “checkbox”? Consider that there are standards groups which explicitly consider these attack vectors and consider them important enough to require mitigations to address those vectors. Do the end users of PG understand the attack vectors or why they matter? Perhaps not, but just because they can’t articulate the
Re: Moving forward with TDE
On Tue, Mar 28, 2023 at 12:57:42AM +0200, Stephen Frost wrote: > I consider the operating system and its processes as much more of a > single entity than TLS over a network. > > This may be the case sometimes but there’s absolutely no shortage of other > cases and it’s almost more the rule these days, that there is some kind of > network between the OS processes and the storage- a SAN, an iSCSI network, > NFS, > are all quite common. Yes, but consider that the database cluster is having to get its data from that remote storage --- the remote storage is not an independent entity that can be corrupted without the databaes server being compromised. If everything in PGDATA was GCM-verified, it would be secure, but because some parts are not, I don't think it would be. > > As specific examples, consider: > > > > An attack against the database system where the database server is shut > down, > > or a backup, and the encryption key isn’t available on the system. > > > > The backup system itself, not running as the PG user (an option > supported > by PG > > and at least pgbackrest) being compromised, thus allowing for injection > of > > changes into a backup or into a restore. > > I then question why we are not adding encryption to pg_basebackup or > pgbackrest rather than the database system. > > Pgbackrest has encryption and authentication of it … but that doesn’t actually > address the attack vector that I outlined. If the backup user is compromised > then they can change the data before it gets to the storage. If the backup > user is compromised then they have access to whatever key is used to encrypt > and authenticate the backup and therefore can trivially manipulate the data. So the idea is that the backup user can be compromised without the data being vulnerable --- makes sense, though that use-case seems narrow. > What were the _technical_ reasons for those objections? > > I believe largely the ones I’m bringing up here and which I outline above… I > don’t mean to pretend that any of this is of my own independent construction. > I > don’t believe it is and my apologies if it came across that way. Yes, there is value beyond the check-box, but in most cases those values are limited considering the complexity of the features, and the check-box is what most people are asking for, I think. > > I’ve grown weary of this argument as the other major piece of work it > was > > routinely applied to was RLS and yet that has certainly been seen > broadly > as a > > beneficial feature with users clearly leveraging it and in more than > some > > “checkbox” way. > > RLS has to overcome that objection, and I think it did, as was better > for doing that. > > Beyond it being called a checkbox - what were the arguments against it? I The RLS arguments were that queries could expoose some of the underlying data, but in summary, that was considered acceptable. > > We, as a community, are clearly losing value by lack of this capability, > if by > > no other measure than simply the numerous users of the commercial > > implementations feeling that they simply can’t use PG without this > feature, for > > whatever their reasoning. > > That is true, but I go back to my concern over useful feature vs. check > box. > > While it’s easy to label something as checkbox, I don’t feel we have been fair No, actually, it isn't. I am not sure why you are saying that. > to our users in doing so as it has historically prevented features which our > users are demanding and end up getting from commercial providers until we > implement them ultimately anyway. This particular argument simply doesn’t > seem > to actually hold the value that proponents of it claim, for us at least, and > we > have clear counter-examples which we can point to and I hope we learn from > those. I don't think you are addressing actual issues above. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
Re: Moving forward with TDE
Greetings, On Mon, Mar 27, 2023 at 18:17 Bruce Momjian wrote: > On Tue, Mar 28, 2023 at 12:01:56AM +0200, Stephen Frost wrote: > > Greetings, > > > > On Mon, Mar 27, 2023 at 12:38 Bruce Momjian wrote: > > > > On Wed, Mar 8, 2023 at 04:25:04PM -0500, Stephen Frost wrote: > > > Agreed, though the latest efforts include an option for > *authenticated* > > > encryption as well as unauthenticated. That makes it much more > > > difficult to make undetected changes to the data that's protected > by > > > the authenticated encryption being used. > > > > I thought some more about this. GCM-style authentication of > encrypted > > data has value because it assumes the two end points are secure but > that > > a malicious actor could modify data during transfer. In the Postgres > > case, it seems the two end points and the transfer are all in the > same > > place. Therefore, it is unclear to me the value of using GCM-style > > authentication because if the GCM-level can be modified, so can the > end > > points, and the encryption key exposed. > > > > > > What are the two end points you are referring to and why don’t you feel > there > > is an opportunity between them for a malicious actor to attack the > system? > > Uh, TLS can use GCM and in this case you assume the sender and receiver > are secure, no? TLS does use GCM.. pretty much exclusively as far as I can recall. So do a lot of other things though.. > There are simpler cases to consider than an online attack on a single > > independent system where an attacker having access to modify the data in > > transit between PG and the storage would imply the attacker also having > access > > to read keys out of PG’s memory. > > I consider the operating system and its processes as much more of a > single entity than TLS over a network. This may be the case sometimes but there’s absolutely no shortage of other cases and it’s almost more the rule these days, that there is some kind of network between the OS processes and the storage- a SAN, an iSCSI network, NFS, are all quite common. > As specific examples, consider: > > > > An attack against the database system where the database server is shut > down, > > or a backup, and the encryption key isn’t available on the system. > > > > The backup system itself, not running as the PG user (an option > supported by PG > > and at least pgbackrest) being compromised, thus allowing for injection > of > > changes into a backup or into a restore. > > I then question why we are not adding encryption to pg_basebackup or > pgbackrest rather than the database system. Pgbackrest has encryption and authentication of it … but that doesn’t actually address the attack vector that I outlined. If the backup user is compromised then they can change the data before it gets to the storage. If the backup user is compromised then they have access to whatever key is used to encrypt and authenticate the backup and therefore can trivially manipulate the data. Encryption of backups by the backup tool serves to protect the data after it leaves the backup system and is stored in cloud storage or in whatever format the repository takes. This is beneficial, particularly when the data itself offers no protection, but simply not the same. > The beginning of this discussion also very clearly had individuals voicing > > strong opinions that unauthenticated encryption methods were not > acceptable as > > an end-state for PG due to the clear issue of there then being no > protection > > against modification of data. The approach we are working towards > provides > > What were the _technical_ reasons for those objections? I believe largely the ones I’m bringing up here and which I outline above… I don’t mean to pretend that any of this is of my own independent construction. I don’t believe it is and my apologies if it came across that way. > both the unauthenticated option, which clearly has value to a large > number of > > our collective user base considering the number of commercial > implementations > > which have now arisen, and the authenticated solution which goes further > and > > provides the level clearly expected of the PG community. This gets us a > win-win > > situation. > > > > > There's clearly user demand for it as there's a number of > organizations > > > who have forks which are providing it in one shape or another. > This > > > kind of splintering of the community is actually an actively bad > thing > > > for the project and is part of what killed Unix, by at least some > pretty > > > reputable accounts, in my view. > > > > Yes, the number of commercial implementations of this is a concern. > Of > > course, it is also possible that those commercial implementations are > > meeting checkbox requirements rather than technical ones, and the > > community has been hostile to check box-only features. > > > > > > I’ve grown weary of this argument
Re: Moving forward with TDE
On Tue, Mar 28, 2023 at 12:01:56AM +0200, Stephen Frost wrote: > Greetings, > > On Mon, Mar 27, 2023 at 12:38 Bruce Momjian wrote: > > On Wed, Mar 8, 2023 at 04:25:04PM -0500, Stephen Frost wrote: > > Agreed, though the latest efforts include an option for *authenticated* > > encryption as well as unauthenticated. That makes it much more > > difficult to make undetected changes to the data that's protected by > > the authenticated encryption being used. > > I thought some more about this. GCM-style authentication of encrypted > data has value because it assumes the two end points are secure but that > a malicious actor could modify data during transfer. In the Postgres > case, it seems the two end points and the transfer are all in the same > place. Therefore, it is unclear to me the value of using GCM-style > authentication because if the GCM-level can be modified, so can the end > points, and the encryption key exposed. > > > What are the two end points you are referring to and why don’t you feel there > is an opportunity between them for a malicious actor to attack the system? Uh, TLS can use GCM and in this case you assume the sender and receiver are secure, no? > There are simpler cases to consider than an online attack on a single > independent system where an attacker having access to modify the data in > transit between PG and the storage would imply the attacker also having access > to read keys out of PG’s memory. I consider the operating system and its processes as much more of a single entity than TLS over a network. > As specific examples, consider: > > An attack against the database system where the database server is shut down, > or a backup, and the encryption key isn’t available on the system. > > The backup system itself, not running as the PG user (an option supported by > PG > and at least pgbackrest) being compromised, thus allowing for injection of > changes into a backup or into a restore. I then question why we are not adding encryption to pg_basebackup or pgbackrest rather than the database system. > The beginning of this discussion also very clearly had individuals voicing > strong opinions that unauthenticated encryption methods were not acceptable as > an end-state for PG due to the clear issue of there then being no protection > against modification of data. The approach we are working towards provides What were the _technical_ reasons for those objections? > both the unauthenticated option, which clearly has value to a large number of > our collective user base considering the number of commercial implementations > which have now arisen, and the authenticated solution which goes further and > provides the level clearly expected of the PG community. This gets us a > win-win > situation. > > > There's clearly user demand for it as there's a number of organizations > > who have forks which are providing it in one shape or another. This > > kind of splintering of the community is actually an actively bad thing > > for the project and is part of what killed Unix, by at least some pretty > > reputable accounts, in my view. > > Yes, the number of commercial implementations of this is a concern. Of > course, it is also possible that those commercial implementations are > meeting checkbox requirements rather than technical ones, and the > community has been hostile to check box-only features. > > > I’ve grown weary of this argument as the other major piece of work it was > routinely applied to was RLS and yet that has certainly been seen broadly as a > beneficial feature with users clearly leveraging it and in more than some > “checkbox” way. RLS has to overcome that objection, and I think it did, as was better for doing that. > We, as a community, are clearly losing value by lack of this capability, if by > no other measure than simply the numerous users of the commercial > implementations feeling that they simply can’t use PG without this feature, > for > whatever their reasoning. That is true, but I go back to my concern over useful feature vs. check box. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
Re: Moving forward with TDE
Greetings, On Mon, Mar 27, 2023 at 12:38 Bruce Momjian wrote: > On Wed, Mar 8, 2023 at 04:25:04PM -0500, Stephen Frost wrote: > > Agreed, though the latest efforts include an option for *authenticated* > > encryption as well as unauthenticated. That makes it much more > > difficult to make undetected changes to the data that's protected by > > the authenticated encryption being used. > > I thought some more about this. GCM-style authentication of encrypted > data has value because it assumes the two end points are secure but that > a malicious actor could modify data during transfer. In the Postgres > case, it seems the two end points and the transfer are all in the same > place. Therefore, it is unclear to me the value of using GCM-style > authentication because if the GCM-level can be modified, so can the end > points, and the encryption key exposed. What are the two end points you are referring to and why don’t you feel there is an opportunity between them for a malicious actor to attack the system? There are simpler cases to consider than an online attack on a single independent system where an attacker having access to modify the data in transit between PG and the storage would imply the attacker also having access to read keys out of PG’s memory. As specific examples, consider: An attack against the database system where the database server is shut down, or a backup, and the encryption key isn’t available on the system. The backup system itself, not running as the PG user (an option supported by PG and at least pgbackrest) being compromised, thus allowing for injection of changes into a backup or into a restore. The beginning of this discussion also very clearly had individuals voicing strong opinions that unauthenticated encryption methods were not acceptable as an end-state for PG due to the clear issue of there then being no protection against modification of data. The approach we are working towards provides both the unauthenticated option, which clearly has value to a large number of our collective user base considering the number of commercial implementations which have now arisen, and the authenticated solution which goes further and provides the level clearly expected of the PG community. This gets us a win-win situation. > There's clearly user demand for it as there's a number of organizations > > who have forks which are providing it in one shape or another. This > > kind of splintering of the community is actually an actively bad thing > > for the project and is part of what killed Unix, by at least some pretty > > reputable accounts, in my view. > > Yes, the number of commercial implementations of this is a concern. Of > course, it is also possible that those commercial implementations are > meeting checkbox requirements rather than technical ones, and the > community has been hostile to check box-only features. I’ve grown weary of this argument as the other major piece of work it was routinely applied to was RLS and yet that has certainly been seen broadly as a beneficial feature with users clearly leveraging it and in more than some “checkbox” way. Indeed, it’s similar also in that commercial implementations were done of RLS while there were arguments made about it being a checkbox feature which were used to discourage it from being implemented in core. Were it truly checkbox, I don’t feel we would have the regular and ongoing discussion about it on the lists that we do, nor see other tools built on top of PG which specifically leverage it. Perhaps there are truly checkbox features out there which we will never implement, but I’m (perhaps due to what my dad would call selective listening on my part, perhaps not) having trouble coming up with any presently. Features that exist in other systems that we don’t want? Certainly. We don’t characterize those as simply “checkbox” though. Perhaps that’s in part because we provide alternatives- but that’s not the case here. We have no comparable way to have this capability as part of the core system. We, as a community, are clearly losing value by lack of this capability, if by no other measure than simply the numerous users of the commercial implementations feeling that they simply can’t use PG without this feature, for whatever their reasoning. Thanks, Stephen
Re: Moving forward with TDE
On Wed, Mar 8, 2023 at 04:25:04PM -0500, Stephen Frost wrote: > Agreed, though the latest efforts include an option for *authenticated* > encryption as well as unauthenticated. That makes it much more > difficult to make undetected changes to the data that's protected by > the authenticated encryption being used. I thought some more about this. GCM-style authentication of encrypted data has value because it assumes the two end points are secure but that a malicious actor could modify data during transfer. In the Postgres case, it seems the two end points and the transfer are all in the same place. Therefore, it is unclear to me the value of using GCM-style authentication because if the GCM-level can be modified, so can the end points, and the encryption key exposed. > There's clearly user demand for it as there's a number of organizations > who have forks which are providing it in one shape or another. This > kind of splintering of the community is actually an actively bad thing > for the project and is part of what killed Unix, by at least some pretty > reputable accounts, in my view. Yes, the number of commercial implementations of this is a concern. Of course, it is also possible that those commercial implementations are meeting checkbox requirements rather than technical ones, and the community has been hostile to check box-only features. > Certainly agree with you there though there's an overall trajectory of > patches involved in all of this that's a bit deep. The plan is to > discuss that at PGCon (On the Road to TDE) and at the PGCon > Unconference after. I certainly hope those interested will be there. > I'm also happy to have a call with anyone interested in this effort > independent of that, of course. I will not be attending Ottawa. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
Re: Moving forward with TDE
Greetings, * Chris Travers (chris.trav...@gmail.com) wrote: > From the documentation, the primary threat model of TDE is to prevent > decryption of data from archived wal segments (and data files), for example > on a backup system. While there are other methods around this problem to > date, I think that this feature is worth pursuing for that reason. I want to > address a couple of reasons for this and then go into some reservations I > have about how some of this is documented. Agreed, though the latest efforts include an option for *authenticated* encryption as well as unauthenticated. That makes it much more difficult to make undetected changes to the data that's protected by the authenticated encryption being used. > There are current workarounds to ensuring encryption at rest, but these have > a number of problems. Encryption passphrases end up lying around the system > in various places. Key rotation is often difficult. And one mistake can > easily render all efforts ineffective. TDE solves these problems. The > overall design from the internal docs looks solid. This definitely is > something I would recommend for many users. There's clearly user demand for it as there's a number of organizations who have forks which are providing it in one shape or another. This kind of splintering of the community is actually an actively bad thing for the project and is part of what killed Unix, by at least some pretty reputable accounts, in my view. > I have a couple small caveats though. Encryption of data is a large topic > and there isn't a one-size-fits-all solution to industrial or state > requirements. Having all this key management available in PostgreSQL is a > very good thing. Long run it is likely to end up being extensible, and > therefore both more powerful and offering a wider range of choices for > solution architects. Implementing encryption is also something that is easy > to mess up. For this reason I think it would be great if we had a > standardized format for discussing encryption options that we could use going > forward. I don't think that should be held against this patch but I think we > need to start discussing it now because it will be a bigger problem later. Do you have a suggestion as to the format to use? > A second caveat I have is that key management is a topic where you really > need a good overview of internals in order to implement effectively. If you > don't know how an SSL handshake works or what is in a certificate, you can > easily make mistakes in setting up SSL. I can see the same thing happening > here. For example, I don't think it would be safe to leave the KEK on an > encrypted filesystem that is decrypted at runtime (or at least I wouldn't > consider that safe -- your appetite for risk may vary). Agreed that we should document this and make clear that the KEK is necessary for server start but absolutely should be kept as safe as possible and certainly not stored on disk somewhere nearby the encrypted cluster. > My proposal would be to have build a template for encryption options in the > documentation. This could include topics like SSL as well. In such a > template we'd have sections like "Threat model," "How it works," > "Implementation Requirements" and so forth. Again I don't think this needs > to be part of the current patch but I think it is something we need to start > thinking about now. Maybe after this goes in, I can present a proposed > documentation patch. I'm not entirely sure that it makes sense to lump this and TLS in the same place as they end up being rather independent at the end of the day. If you have ideas for how to improve the documentation, I'd certainly encourage you to go ahead and work on that and submit it as a patch rather than waiting for this to actually land in core. Having good and solid documentation is something that will help this get in, after all, and to the extent that it's covering existing topics like TLS, those could likely be included independently and that would be of benefit to everyone. > I will also note that I don't consider myself to be very qualified on topics > like encryption. I can reason about key management to some extent but some > implementation details may be beyond me. I would hope we could get some > extra review on this patch set soon. Certainly agree with you there though there's an overall trajectory of patches involved in all of this that's a bit deep. The plan is to discuss that at PGCon (On the Road to TDE) and at the PGCon Unconference after. I certainly hope those interested will be there. I'm also happy to have a call with anyone interested in this effort independent of that, of course. Thanks! Stephen signature.asc Description: PGP signature
Re: Moving forward with TDE
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested I have decided to write a review here in terms of whether we want this feature, and perhaps the way we should look at encryption as a project down the road, since I think this is only the beginning. I am hoping to run some full tests of the feature sometime in coming weeks. Right now this review is limited to the documentation and documented feature. From the documentation, the primary threat model of TDE is to prevent decryption of data from archived wal segments (and data files), for example on a backup system. While there are other methods around this problem to date, I think that this feature is worth pursuing for that reason. I want to address a couple of reasons for this and then go into some reservations I have about how some of this is documented. There are current workarounds to ensuring encryption at rest, but these have a number of problems. Encryption passphrases end up lying around the system in various places. Key rotation is often difficult. And one mistake can easily render all efforts ineffective. TDE solves these problems. The overall design from the internal docs looks solid. This definitely is something I would recommend for many users. I have a couple small caveats though. Encryption of data is a large topic and there isn't a one-size-fits-all solution to industrial or state requirements. Having all this key management available in PostgreSQL is a very good thing. Long run it is likely to end up being extensible, and therefore both more powerful and offering a wider range of choices for solution architects. Implementing encryption is also something that is easy to mess up. For this reason I think it would be great if we had a standardized format for discussing encryption options that we could use going forward. I don't think that should be held against this patch but I think we need to start discussing it now because it will be a bigger problem later. A second caveat I have is that key management is a topic where you really need a good overview of internals in order to implement effectively. If you don't know how an SSL handshake works or what is in a certificate, you can easily make mistakes in setting up SSL. I can see the same thing happening here. For example, I don't think it would be safe to leave the KEK on an encrypted filesystem that is decrypted at runtime (or at least I wouldn't consider that safe -- your appetite for risk may vary). My proposal would be to have build a template for encryption options in the documentation. This could include topics like SSL as well. In such a template we'd have sections like "Threat model," "How it works," "Implementation Requirements" and so forth. Again I don't think this needs to be part of the current patch but I think it is something we need to start thinking about now. Maybe after this goes in, I can present a proposed documentation patch. I will also note that I don't consider myself to be very qualified on topics like encryption. I can reason about key management to some extent but some implementation details may be beyond me. I would hope we could get some extra review on this patch set soon.
Re: Moving forward with TDE
On Fri, 4 Nov 2022 at 03:36, David Christensen wrote: > > > Unless somebody in the community remembers open questions/issues with > > TDE that were never addressed I suggest simply iterating with our > > usual testing/reviewing process. For now I'm going to change the > > status of the CF entry [1] to "Waiting for Author" since the patchset > > doesn't pass the CI [2]. > > Thanks, enclosed is a new version that is rebased on HEAD and fixes a > bug that the new pg_control_init() test picked up. The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID b82557ecc2ebbf649142740a1c5ce8d19089f620 === === applying patch ./v2-0004-cfe-04-common_over_cfe-03-scripts-squash-commit.patch patching file src/common/Makefile Hunk #2 FAILED at 84. 1 out of 2 hunks FAILED -- saving rejects to file src/common/Makefile.rej [1] - http://cfbot.cputube.org/patch_41_3985.log Regards, Vignesh
Re: Moving forward with TDE
Hi Dilip, Thanks for the feedback here. I will review the docs changes and add to my tree. Best, David
Re: Moving forward with TDE
Hi Jacob, Thanks, I've added this patch in my tree [1]. (For now, just adding fixes and the like atop the original separate patches, but will eventually get things winnowed down into probably the same 12 parts the originals were reviewed in. Best, David [1] https://github.com/pgguru/postgres/tree/tde
Re: Moving forward with TDE
On Tue, Nov 15, 2022 at 11:39 AM David Christensen
wrote:
> Good to know about the next steps, thanks.
You're welcome!
> This was just a refresh of the old patches on the wiki to work as written on
> HEAD. If there are known TODOs here this then that work is still needing to
> be done.
>
> I was going to take 2) and Stephen was going to work on 3); I am not sure
> about the other two but will review the thread you pointed to. Thanks for
> pointing that out.
I've attached the diffs I'm carrying to build this under meson (as
well as -Wshadow; my removal of the two variables probably needs some
scrutiny). It looks like the testcrypto executable will need
substantial changes after the common/hex.h revert.
--Jacob
From eb4b55f5d03e362cf340f322c0cefbf95f53657a Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Mon, 14 Nov 2022 16:02:57 -0800
Subject: [PATCH] WIP: fix meson
Note that the testcrypto executable needs to be rewritten.
---
src/backend/bootstrap/bootstrap.c | 2 --
src/backend/crypto/kmgr.c | 1 -
src/backend/crypto/meson.build| 14 ++
src/backend/meson.build | 1 +
src/bin/meson.build | 1 +
src/bin/pg_alterckey/meson.build | 16
src/common/meson.build| 3 +++
7 files changed, 35 insertions(+), 3 deletions(-)
create mode 100644 src/backend/crypto/meson.build
create mode 100644 src/bin/pg_alterckey/meson.build
diff --git a/src/backend/bootstrap/bootstrap.c
b/src/backend/bootstrap/bootstrap.c
index ffaf9c7cc4..07b2f15343 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -256,8 +256,6 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
break;
case 'K':
{
- int i;
-
/* method 0/disabled cannot be
specified */
for (i = DISABLED_ENCRYPTION_METHOD + 1;
i < NUM_ENCRYPTION_METHODS;
i++)
diff --git a/src/backend/crypto/kmgr.c b/src/backend/crypto/kmgr.c
index a83effbca8..b4248965f8 100644
--- a/src/backend/crypto/kmgr.c
+++ b/src/backend/crypto/kmgr.c
@@ -141,7 +141,6 @@ BootStrapKmgr(void)
if (bootstrap_old_key_datadir == NULL)
{
unsigned char *bootstrap_keys_wrap[KMGR_NUM_DATA_KEYS];
- int key_lens[KMGR_NUM_DATA_KEYS];
PgCipherCtx *cluster_key_ctx;
/* Create KEK encryption context */
diff --git a/src/backend/crypto/meson.build b/src/backend/crypto/meson.build
new file mode 100644
index 00..2b72b26ebf
--- /dev/null
+++ b/src/backend/crypto/meson.build
@@ -0,0 +1,14 @@
+backend_sources += files(
+ 'bufenc.c',
+ 'kmgr.c',
+)
+
+install_data(
+ 'ckey_aws.sh.sample',
+ 'ckey_direct.sh.sample',
+ 'ckey_passphrase.sh.sample',
+ 'ckey_piv_nopin.sh.sample',
+ 'ckey_piv_pin.sh.sample',
+ 'ssl_passphrase.sh.sample',
+ install_dir: dir_data / 'auth_commands',
+)
diff --git a/src/backend/meson.build b/src/backend/meson.build
index 37562bae13..2162aef554 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -9,6 +9,7 @@ subdir('backup')
subdir('bootstrap')
subdir('catalog')
subdir('commands')
+subdir('crypto')
subdir('executor')
subdir('foreign')
subdir('jit')
diff --git a/src/bin/meson.build b/src/bin/meson.build
index 5fd5a9d2f9..8dd9133213 100644
--- a/src/bin/meson.build
+++ b/src/bin/meson.build
@@ -1,4 +1,5 @@
subdir('initdb')
+subdir('pg_alterckey')
subdir('pg_amcheck')
subdir('pg_archivecleanup')
subdir('pg_basebackup')
diff --git a/src/bin/pg_alterckey/meson.build b/src/bin/pg_alterckey/meson.build
new file mode 100644
index 00..72ec8274c3
--- /dev/null
+++ b/src/bin/pg_alterckey/meson.build
@@ -0,0 +1,16 @@
+pg_alterckey_sources = files(
+ 'pg_alterckey.c',
+)
+
+if host_system == 'windows'
+ pg_alterckey_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
+'--NAME', 'pg_alterckey',
+'--FILEDESC', 'pg_alterckey - alter the cluster key',])
+endif
+
+pg_alterckey = executable('pg_alterckey',
+ pg_alterckey_sources,
+ dependencies: [frontend_code, libpq],
+ kwargs: default_bin_args,
+)
+bin_targets += pg_alterckey
diff --git a/src/common/meson.build b/src/common/meson.build
index 1c9b8a3a01..102b4d7b23 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -12,6 +12,7 @@ common_sources = files(
'ip.c',
'jsonapi.c',
'keywords.c',
+ 'kmgr_utils.c',
'kwlookup.c',
'link-canary.c',
'md5_common.c',
@@ -34,12 +35,14 @@ common_sources = files(
if ssl.found()
common_sources += files(
+'cipher_openssl.c',
'cryptohash_openssl.c',
'hmac_openssl.c',
'protocol_openssl.c',
)
else
common_sources += files(
+'cipher.c',
'cryptohash.c',
'hmac.c',
'md5.c',
--
2.25.1
Re: Moving forward with TDE
> On Nov 15, 2022, at 1:08 PM, Jacob Champion wrote: > > On Mon, Oct 24, 2022 at 9:29 AM David Christensen > wrote: >> I would love to open a discussion about how to move forward and get >> some of these features built out. The historical threads here are >> quite long and complicated; is there a "current state" other than the >> wiki that reflects the general thinking on this feature? Any major >> developments in direction that would not be reflected in the code from >> May 2021? > > I don't think the patchset here has incorporated the results of the > discussion [1] that happened at the end of 2021. For example, it looks > like AES-CTR is still in use for the pages, which I thought was > already determined to be insufficient. Good to know about the next steps, thanks. > The following next steps were proposed in that thread: > >> 1. modify temporary file I/O to use a more centralized API >> 2. modify the existing cluster file encryption patch to use XTS with a >> IV that uses more than the LSN >> 3. add XTS regression test code like CTR >> 4. create WAL encryption code using CTR > > Does this patchset need review before those steps are taken (or was > there additional conversation/work that I missed)? This was just a refresh of the old patches on the wiki to work as written on HEAD. If there are known TODOs here this then that work is still needing to be done. I was going to take 2) and Stephen was going to work on 3); I am not sure about the other two but will review the thread you pointed to. Thanks for pointing that out. David
Re: Moving forward with TDE
On Mon, Oct 24, 2022 at 9:29 AM David Christensen wrote: > I would love to open a discussion about how to move forward and get > some of these features built out. The historical threads here are > quite long and complicated; is there a "current state" other than the > wiki that reflects the general thinking on this feature? Any major > developments in direction that would not be reflected in the code from > May 2021? I don't think the patchset here has incorporated the results of the discussion [1] that happened at the end of 2021. For example, it looks like AES-CTR is still in use for the pages, which I thought was already determined to be insufficient. The following next steps were proposed in that thread: > 1. modify temporary file I/O to use a more centralized API > 2. modify the existing cluster file encryption patch to use XTS with a >IV that uses more than the LSN > 3. add XTS regression test code like CTR > 4. create WAL encryption code using CTR Does this patchset need review before those steps are taken (or was there additional conversation/work that I missed)? Thanks, --Jacob [1] https://www.postgresql.org/message-id/flat/20211013222648.GA373%40momjian.us
Re: Moving forward with TDE
On Fri, Nov 4, 2022 at 3:36 AM David Christensen wrote: > > > Unless somebody in the community remembers open questions/issues with > > TDE that were never addressed I suggest simply iterating with our > > usual testing/reviewing process. For now I'm going to change the > > status of the CF entry [1] to "Waiting for Author" since the patchset > > doesn't pass the CI [2]. > > Thanks, enclosed is a new version that is rebased on HEAD and fixes a > bug that the new pg_control_init() test picked up. I was looking into the documentation patches 0001 and 0002, I think the explanation is very clear. I have a few questions/comments +By not using the database id in the IV, CREATE DATABASE can copy the +heap/index files from the old database to a new one without +decryption/encryption. Both page copies are valid. Once a database +changes its pages, it gets new LSNs, and hence new IV. How about the WAL_LOG method for creating a database? because in that we get the new LSN for the pages in the new database, so do we reencrypt, if yes then this documentation needs to be updated otherwise we might need to add that code. +changes its pages, it gets new LSNs, and hence new IV. Using only the +LSN and page number also avoids requiring pg_upgrade to preserve +database oids, tablespace oids, and relfilenodes. I think this line needs to be changed, because now we are already preserving dbid/tbsid/relfilenode. So even though we are not using those in IV there is no point in saying we are avoiding that requirement. I will review the remaining patches soon. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Moving forward with TDE
Hi David, > Working with Stephen, I am attempting to pick up some of the work that > was left off with TDE and the key management infrastructure. I have > rebased Bruce's KMS/TDE patches as they existed on the > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption wiki > page, which are enclosed in this email. I'm happy to see that the TDE effort was picked up. > I would love to open a discussion about how to move forward and get > some of these features built out. The historical threads here are > quite long and complicated; is there a "current state" other than the > wiki that reflects the general thinking on this feature? Any major > developments in direction that would not be reflected in the code from > May 2021? The patches seem to be well documented and decomposed in small pieces. That's good. Unless somebody in the community remembers open questions/issues with TDE that were never addressed I suggest simply iterating with our usual testing/reviewing process. For now I'm going to change the status of the CF entry [1] to "Waiting for Author" since the patchset doesn't pass the CI [2]. One limitation of the design described on the wiki I see is that it seems to heavily rely on AES: > We will use Advanced Encryption Standard (AES) [4]. We will offer three key > length options (128, 192, and 256-bits) selected at initdb time with > --file-encryption-method (there doesn't seem to be any mention of the hash/MAC algorithms, that's odd). In the future we should be able to add the support of alternative algorithms. The reason is that the algorithms can become weak every 20 years or so, and the preferred algorithms may also depend on the region. This should NOT be implemented in this particular patchset, but the design shouldn't prevent from implementing this in the future. [1]: https://commitfest.postgresql.org/40/3985/ [2]: http://cfbot.cputube.org/ -- Best regards, Aleksander Alekseev
