Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Robert Haas writes: > On Thu, Apr 21, 2022 at 1:53 PM Tom Lane wrote: >> 0001 attached is a revised patch that does it that way. This seems >> like a clearly better answer. >> 0002 contains the perhaps-slightly-more-controversial changes of >> changing the macro names and explicitly pinning no databases. > Both patches look good to me. Pushed, thanks for looking. regards, tom lane
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Thu, Apr 21, 2022 at 1:53 PM Tom Lane wrote: > Robert Haas writes: > > On Wed, Apr 20, 2022 at 4:56 PM Tom Lane wrote: > >> Having just had to bury my nose in renumber_oids.pl, I thought of a > >> different approach we could take to expose these OIDs to Catalog.pm. > >> That's to invent a new macro that Catalog.pm recognizes, and write > >> something about like this in pg_database.h: > >> DECLARE_OID_DEFINING_MACRO(Template0ObjectId, 4); > >> DECLARE_OID_DEFINING_MACRO(PostgresObjectId, 5); > > > I like it! > > 0001 attached is a revised patch that does it that way. This seems > like a clearly better answer. > > 0002 contains the perhaps-slightly-more-controversial changes of > changing the macro names and explicitly pinning no databases. Both patches look good to me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Robert Haas writes:
> On Wed, Apr 20, 2022 at 4:56 PM Tom Lane wrote:
>> Having just had to bury my nose in renumber_oids.pl, I thought of a
>> different approach we could take to expose these OIDs to Catalog.pm.
>> That's to invent a new macro that Catalog.pm recognizes, and write
>> something about like this in pg_database.h:
>> DECLARE_OID_DEFINING_MACRO(Template0ObjectId, 4);
>> DECLARE_OID_DEFINING_MACRO(PostgresObjectId, 5);
> I like it!
0001 attached is a revised patch that does it that way. This seems
like a clearly better answer.
0002 contains the perhaps-slightly-more-controversial changes of
changing the macro names and explicitly pinning no databases.
regards, tom lane
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 0275795dea..ece0a934f0 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -44,6 +44,8 @@ sub ParseHeader
$catalog{columns} = [];
$catalog{toasting}= [];
$catalog{indexing}= [];
+ $catalog{other_oids} = [];
+ $catalog{foreign_keys} = [];
$catalog{client_code} = [];
open(my $ifh, '<', $input_file) || die "$input_file: $!";
@@ -118,6 +120,14 @@ sub ParseHeader
index_decl => $6
};
}
+ elsif (/^DECLARE_OID_DEFINING_MACRO\(\s*(\w+),\s*(\d+)\)/)
+ {
+ push @{ $catalog{other_oids} },
+ {
+other_name => $1,
+other_oid => $2
+ };
+ }
elsif (
/^DECLARE_(ARRAY_)?FOREIGN_KEY(_OPT)?\(\s*\(([^)]+)\),\s*(\w+),\s*\(([^)]+)\)\)/
)
@@ -572,6 +582,10 @@ sub FindAllOidsFromHeaders
{
push @oids, $index->{index_oid};
}
+ foreach my $other (@{ $catalog->{other_oids} })
+ {
+ push @oids, $other->{other_oid};
+ }
}
return \@oids;
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 520f77971b..d7e5c02f95 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -339,9 +339,11 @@ IsPinnedObject(Oid classId, Oid objectId)
* robustness.
*/
- /* template1 is not pinned */
+ /* template1, template0, postgres are not pinned */
if (classId == DatabaseRelationId &&
- objectId == TemplateDbOid)
+ (objectId == TemplateDbOid ||
+ objectId == Template0ObjectId ||
+ objectId == PostgresObjectId))
return false;
/* the public namespace is not pinned */
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 2d02b02267..f4ec6d6d40 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -472,7 +472,7 @@ EOM
$catalog->{rowtype_oid_macro}, $catalog->{rowtype_oid}
if $catalog->{rowtype_oid_macro};
- # Likewise for macros for toast and index OIDs
+ # Likewise for macros for toast, index, and other OIDs
foreach my $toast (@{ $catalog->{toasting} })
{
printf $def "#define %s %s\n",
@@ -488,6 +488,12 @@ EOM
$index->{index_oid_macro}, $index->{index_oid}
if $index->{index_oid_macro};
}
+ foreach my $other (@{ $catalog->{other_oids} })
+ {
+ printf $def "#define %s %s\n",
+ $other->{other_name}, $other->{other_oid}
+ if $other->{other_name};
+ }
print $def "\n";
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 1cb4a5b0d2..5e2eeefc4c 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -59,11 +59,11 @@
#include "sys/mman.h"
#endif
-#include "access/transam.h"
#include "access/xlog_internal.h"
#include "catalog/pg_authid_d.h"
#include "catalog/pg_class_d.h" /* pgrminclude ignore */
#include "catalog/pg_collation_d.h"
+#include "catalog/pg_database_d.h" /* pgrminclude ignore */
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d3588607e7..786d592e2b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2901,10 +2901,11 @@ dumpDatabase(Archive *fout)
qdatname = pg_strdup(fmtId(datname));
/*
- * Prepare the CREATE DATABASE command. We must specify encoding, locale,
- * and tablespace since those can't be altered later. Other DB properties
- * are left to the DATABASE PROPERTIES entry, so that they can be applied
- * after reconnecting to the target DB.
+ * Prepare the CREATE DATABASE command. We must specify OID (if we want
+ * to preserve that), as well as the encoding, locale, and tablespace
+ * since those can't be altered later. Other DB properties are left to
+ * the DATABASE PROPERTIES entry, so that they can be applied after
+ * reconnecting to the target DB.
*/
if (dopt->binary_upgrade)
{
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 9a2816de51..338dfca5a0 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -196,10 +196,6 @@ FullTransactionIdAdvance(FullTransactionId *dest)
#define FirstUnpinnedObjectId 12000
#define FirstNormalObjectId 16384
-/* OIDs of Template0 and Postgres database are fixed */
-#define
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Wed, Apr 20, 2022 at 4:56 PM Tom Lane wrote: > Having just had to bury my nose in renumber_oids.pl, I thought of a > different approach we could take to expose these OIDs to Catalog.pm. > That's to invent a new macro that Catalog.pm recognizes, and write > something about like this in pg_database.h: > > DECLARE_OID_DEFINING_MACRO(Template0ObjectId, 4); > DECLARE_OID_DEFINING_MACRO(PostgresObjectId, 5); > > If that seems more plausible to you I'll set about preparing a patch. I like it! -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Robert Haas writes: > On Wed, Apr 20, 2022 at 2:34 PM Tom Lane wrote: >> The attached draft patch attempts to improve this situation. >> It reserves these OIDs, and creates the associated macros, through >> the normal BKI infrastructure by adding entries in pg_database.dat. >> We have to delete those rows again during initdb, which is slightly >> ugly but surely no more so than initdb's other direct manipulations >> of pg_database. > I'm not sure I really like this approach, but if you're firmly > convinced that it's better than cleaning up the loose ends in some > other way, I'm not going to waste a lot of energy fighting about it. Having just had to bury my nose in renumber_oids.pl, I thought of a different approach we could take to expose these OIDs to Catalog.pm. That's to invent a new macro that Catalog.pm recognizes, and write something about like this in pg_database.h: DECLARE_OID_DEFINING_MACRO(Template0ObjectId, 4); DECLARE_OID_DEFINING_MACRO(PostgresObjectId, 5); That would result in (a) the OIDs becoming known to Catalog.pm as reserved, though it wouldn't have any great clue about their semantics; and (b) this getting emitted into pg_database_d.h: #define Template0ObjectId 4 #define PostgresObjectId 5 Then we'd not need the dummy entries in pg_database.dat, which does seem cleaner now that I think about it. A downside is that with no context, Catalog.pm could not provide name translation services during postgres.bki generation for such OIDs --- but at least for these entries, we don't need that. If that seems more plausible to you I'll set about preparing a patch. regards, tom lane
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Wed, Apr 20, 2022 at 2:34 PM Tom Lane wrote: > The attached draft patch attempts to improve this situation. > It reserves these OIDs, and creates the associated macros, through > the normal BKI infrastructure by adding entries in pg_database.dat. > We have to delete those rows again during initdb, which is slightly > ugly but surely no more so than initdb's other direct manipulations > of pg_database. I'm not sure I really like this approach, but if you're firmly convinced that it's better than cleaning up the loose ends in some other way, I'm not going to waste a lot of energy fighting about it. It doesn't seem horrible. > There are a few loose ends: > > * I'm a bit inclined to simplify IsPinnedObject by just teaching > it that *no* entries of pg_database are pinned, which would correspond > to the evident lack of enforcement in dropdb(). Can anyone see a > reason why we might pin some database in future? It's kind of curious that we don't pin anything now. There's kind of nothing to stop you from hosing the system by dropping template0 and/or template1, or mutating them into some crazy form that doesn't work. But having said that, if as a matter of policy we don't even pin template0 or template1 or postgres, then it seems unlikely that we would suddenly decide to pin some new database in the future. > * I had to set up the additional pg_database entries with nonzero > datfrozenxid to avoid an assertion failure during initdb's first VACUUM. > (That VACUUM will overwrite template1's datfrozenxid before computing > the global minimum frozen XID, but not these others; and it doesn't like > finding that the minimum is zero.) This feels klugy. An alternative is > to delete the extra pg_database rows before that VACUUM, which would > mean taking those deletes out of make_template0 and make_postgres and > putting them somewhere seemingly unrelated, so that's a bit crufty too. > Anybody have a preference? Not really. If anything that's an argument against this entire approach, but I already commented on that point above. > * The new macro names seem ill-chosen. Template0ObjectId is spelled > randomly differently from the longstanding TemplateDbOid, and surely > PostgresObjectId is about as vague a name as could possibly have > been thought of (please name an object that it couldn't apply to). > I'm a little inclined to rename TemplateDbOid to Template1DbOid and > use Template0DbOid and PostgresDbOid for the others, but I didn't pull > that trigger here. Seems totally reasonable. I don't find the current naming horrible or I'd not have committed it that way, but putting Db in there makes sense, too. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Robert Haas writes:
> On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda wrote:
>> Agree. In the latest patch, the template0 and postgres OIDs are fixed
>> to unused manually assigned OIDs 4 and 5 respectively. These OIDs are
>> no more listed as unused OIDs.
> Thanks. Committed with a few more cosmetic changes.
I happened to take a closer look at this patch today, and I'm pretty
unhappy with the way that the assignment of those OIDs was managed.
There are two big problems:
1. IsPinnedObject() will now report that template0 and postgres are
pinned. This seems not to prevent one from dropping them (I guess
dropdb() doesn't consult IsPinnedObject), but it would probably bollix
any pg_shdepend management that should happen for them.
2. The Catalog.pm infrastructure knows nothing about these OIDs.
While the unused_oids script was hack-n-slashed to claim that the
OIDs are used, other scripts won't know about them; for example
duplicate_oids won't report conflicts if someone tries to reuse
those OIDs.
The attached draft patch attempts to improve this situation.
It reserves these OIDs, and creates the associated macros, through
the normal BKI infrastructure by adding entries in pg_database.dat.
We have to delete those rows again during initdb, which is slightly
ugly but surely no more so than initdb's other direct manipulations
of pg_database.
There are a few loose ends:
* I'm a bit inclined to simplify IsPinnedObject by just teaching
it that *no* entries of pg_database are pinned, which would correspond
to the evident lack of enforcement in dropdb(). Can anyone see a
reason why we might pin some database in future?
* I had to set up the additional pg_database entries with nonzero
datfrozenxid to avoid an assertion failure during initdb's first VACUUM.
(That VACUUM will overwrite template1's datfrozenxid before computing
the global minimum frozen XID, but not these others; and it doesn't like
finding that the minimum is zero.) This feels klugy. An alternative is
to delete the extra pg_database rows before that VACUUM, which would
mean taking those deletes out of make_template0 and make_postgres and
putting them somewhere seemingly unrelated, so that's a bit crufty too.
Anybody have a preference?
* The new macro names seem ill-chosen. Template0ObjectId is spelled
randomly differently from the longstanding TemplateDbOid, and surely
PostgresObjectId is about as vague a name as could possibly have
been thought of (please name an object that it couldn't apply to).
I'm a little inclined to rename TemplateDbOid to Template1DbOid and
use Template0DbOid and PostgresDbOid for the others, but I didn't pull
that trigger here.
Comments?
regards, tom lane
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 520f77971b..d7e5c02f95 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -339,9 +339,11 @@ IsPinnedObject(Oid classId, Oid objectId)
* robustness.
*/
- /* template1 is not pinned */
+ /* template1, template0, postgres are not pinned */
if (classId == DatabaseRelationId &&
- objectId == TemplateDbOid)
+ (objectId == TemplateDbOid ||
+ objectId == Template0ObjectId ||
+ objectId == PostgresObjectId))
return false;
/* the public namespace is not pinned */
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 1cb4a5b0d2..04454b3d7c 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -59,11 +59,11 @@
#include "sys/mman.h"
#endif
-#include "access/transam.h"
#include "access/xlog_internal.h"
#include "catalog/pg_authid_d.h"
#include "catalog/pg_class_d.h" /* pgrminclude ignore */
#include "catalog/pg_collation_d.h"
+#include "catalog/pg_database_d.h" /* pgrminclude ignore */
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
@@ -1806,14 +1806,24 @@ make_template0(FILE *cmdfd)
* objects in the old cluster, the problem scenario only exists if the OID
* that is in use in the old cluster is also used in the new cluster - and
* the new cluster should be the result of a fresh initdb.)
- *
- * We use "STRATEGY = file_copy" here because checkpoints during initdb
- * are cheap. "STRATEGY = wal_log" would generate more WAL, which would
- * be a little bit slower and make the new cluster a little bit bigger.
*/
static const char *const template0_setup[] = {
- "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID = "
- CppAsString2(Template0ObjectId)
+ /*
+ * Since pg_database.dat includes a dummy row for template0, we have
+ * to remove that before creating the database for real.
+ */
+ "DELETE FROM pg_database WHERE datname = 'template0';\n\n",
+
+ /*
+ * Clone template1 to make template0.
+ *
+ * We use "STRATEGY = file_copy" here because checkpoints during
+ * initdb are cheap. "STRATEGY = wal_log" would generate more WAL,
+ * which would be a little bit slower and make the
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Mon, Mar 21, 2022 at 8:52 PM Andres Freund wrote: > I noticed this still has an open CF entry: > https://commitfest.postgresql.org/37/3296/ > I assume it can be marked as committed? Yeah, done now. But don't forget that we still need to do something on the "wrong fds used for refilenodes after pg_upgrade relfilenode changes Reply-To:" thread, and I think the ball is in your court there. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On 2022-01-24 14:44:10 -0500, Robert Haas wrote: > On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda wrote: > > Agree. In the latest patch, the template0 and postgres OIDs are fixed > > to unused manually assigned OIDs 4 and 5 respectively. These OIDs are > > no more listed as unused OIDs. > > Thanks. Committed with a few more cosmetic changes. I noticed this still has an open CF entry: https://commitfest.postgresql.org/37/3296/ I assume it can be marked as committed? - Andres
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Jan 25, 2022 at 10:19:53AM +0530, Shruthi Gowda wrote: > On Tue, Jan 25, 2022 at 1:14 AM Robert Haas wrote: > > On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda wrote: > > > Agree. In the latest patch, the template0 and postgres OIDs are fixed > > > to unused manually assigned OIDs 4 and 5 respectively. These OIDs are > > > no more listed as unused OIDs. > > > > Thanks. Committed with a few more cosmetic changes. > > Thanks, Robert for committing this patch. If I'm not wrong, this can be closed. https://commitfest.postgresql.org/37/3296/
Re: storing an explicit nonce
Stephen Frost wrote: > Perhaps this is all too meta and we need to work through some specific > ideas around just what this would look like. In particular, thinking > about what this API would look like and how it would be used by > reorderbuffer.c, which builds up changes in memory and then does a bare > write() call, seems like a main use-case to consider. The gist there > being "can we come up with an API to do all these things that doesn't > require entirely rewriting ReorderBufferSerializeChange()?" > > Seems like it'd be easier to achieve that by having something that looks > very close to how write() looks, but just happens to have the option to > run the data through a stream cipher and maybe does better error > handling for us. Making that layer also do block-based access to the > files underneath seems like a much larger effort that, sure, may make > some things better too but if we could do that with the same API then it > could also be done later if someone's interested in that. My initial proposal is in this new thread: https://www.postgresql.org/message-id/4987.1644323098%40antos -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Jan 25, 2022 at 1:14 AM Robert Haas wrote: > > On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda wrote: > > Agree. In the latest patch, the template0 and postgres OIDs are fixed > > to unused manually assigned OIDs 4 and 5 respectively. These OIDs are > > no more listed as unused OIDs. > > Thanks. Committed with a few more cosmetic changes. Thanks, Robert for committing this patch.
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda wrote: > Agree. In the latest patch, the template0 and postgres OIDs are fixed > to unused manually assigned OIDs 4 and 5 respectively. These OIDs are > no more listed as unused OIDs. Thanks. Committed with a few more cosmetic changes. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Sat, Jan 22, 2022 at 12:47:35PM +0530, Shruthi Gowda wrote: > On Sat, Jan 22, 2022 at 12:27 AM Tom Lane wrote: > > > > Robert Haas writes: > > > It seems to me that what this comment is saying is that OIDs in the > > > second and third categories are doled out by counters. Therefore, we > > > can't know which of those OIDs will get used, or how many of them will > > > get used, or which objects will get which OIDs. Therefore, I think we > > > should go back to the approach that you were using for template0 and > > > handle both that database and postgres using that method. That is, > > > assign an OID manually, and make sure unused_oids knows that it should > > > be counted as already used. > > > > Indeed. If you're going to manually assign OIDs to these databases, > > do it honestly, and put them into the range intended for that purpose. > > Trying to take short-cuts is just going to cause trouble down the road. > > Understood. I will rework the patch accordingly. Thanks Thanks. To get the rsync update reduction we need to preserve database oids. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Sat, Jan 22, 2022 at 12:17 AM Robert Haas wrote: > > On Fri, Jan 21, 2022 at 8:40 AM Shruthi Gowda wrote: > > From what I see in the code, template0 and postgres are the last > > things that get created in initdb phase. The system OIDs that get > > assigned to these DBs vary from release to release. At present, the > > system assigned OIDs of template0 and postgres are 13679 and 13680 > > respectively. I feel it would be safe to assign 16000 and 16001 for > > template0 and postgres respectively from the unpinned object OID range > > 12000 - 16383. In the future, even if the initdb unpinned objects > > reach the range of 16000 issues can only arise if initdb() creates > > another system-created database for which the system assigns these > > reserved OIDs (16000, 16001). > > It doesn't seem safe to me to rely on that. We don't know what could > happen in the future if the number of built-in objects increases. > Looking at the lengthy comment on this topic in transam.h, I see that > there are three ranges: > > 1- manually assigned OIDs > 1-11999 OIDs assigned by genbki.pl > 12000-16384 OIDs assigned to unpinned objects post-bootstrap > > It seems to me that what this comment is saying is that OIDs in the > second and third categories are doled out by counters. Therefore, we > can't know which of those OIDs will get used, or how many of them will > get used, or which objects will get which OIDs. Therefore, I think we > should go back to the approach that you were using for template0 and > handle both that database and postgres using that method. That is, > assign an OID manually, and make sure unused_oids knows that it should > be counted as already used. Agree. In the latest patch, the template0 and postgres OIDs are fixed to unused manually assigned OIDs 4 and 5 respectively. These OIDs are no more listed as unused OIDs. Regards, Shruthi KC EnterpriseDB: http://www.enterprisedb.com v13-0001-pg_upgrade-perserve-database-OID-patch.patch Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Sat, Jan 22, 2022 at 12:27 AM Tom Lane wrote: > > Robert Haas writes: > > It seems to me that what this comment is saying is that OIDs in the > > second and third categories are doled out by counters. Therefore, we > > can't know which of those OIDs will get used, or how many of them will > > get used, or which objects will get which OIDs. Therefore, I think we > > should go back to the approach that you were using for template0 and > > handle both that database and postgres using that method. That is, > > assign an OID manually, and make sure unused_oids knows that it should > > be counted as already used. > > Indeed. If you're going to manually assign OIDs to these databases, > do it honestly, and put them into the range intended for that purpose. > Trying to take short-cuts is just going to cause trouble down the road. Understood. I will rework the patch accordingly. Thanks Regards, Shruthi KC EnterpriseDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Robert Haas writes: > It seems to me that what this comment is saying is that OIDs in the > second and third categories are doled out by counters. Therefore, we > can't know which of those OIDs will get used, or how many of them will > get used, or which objects will get which OIDs. Therefore, I think we > should go back to the approach that you were using for template0 and > handle both that database and postgres using that method. That is, > assign an OID manually, and make sure unused_oids knows that it should > be counted as already used. Indeed. If you're going to manually assign OIDs to these databases, do it honestly, and put them into the range intended for that purpose. Trying to take short-cuts is just going to cause trouble down the road. regards, tom lane
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Fri, Jan 21, 2022 at 8:40 AM Shruthi Gowda wrote: > From what I see in the code, template0 and postgres are the last > things that get created in initdb phase. The system OIDs that get > assigned to these DBs vary from release to release. At present, the > system assigned OIDs of template0 and postgres are 13679 and 13680 > respectively. I feel it would be safe to assign 16000 and 16001 for > template0 and postgres respectively from the unpinned object OID range > 12000 - 16383. In the future, even if the initdb unpinned objects > reach the range of 16000 issues can only arise if initdb() creates > another system-created database for which the system assigns these > reserved OIDs (16000, 16001). It doesn't seem safe to me to rely on that. We don't know what could happen in the future if the number of built-in objects increases. Looking at the lengthy comment on this topic in transam.h, I see that there are three ranges: 1- manually assigned OIDs 1-11999 OIDs assigned by genbki.pl 12000-16384 OIDs assigned to unpinned objects post-bootstrap It seems to me that what this comment is saying is that OIDs in the second and third categories are doled out by counters. Therefore, we can't know which of those OIDs will get used, or how many of them will get used, or which objects will get which OIDs. Therefore, I think we should go back to the approach that you were using for template0 and handle both that database and postgres using that method. That is, assign an OID manually, and make sure unused_oids knows that it should be counted as already used. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Fri, Jan 21, 2022 at 1:08 AM Robert Haas wrote: > > On Thu, Jan 20, 2022 at 11:03 AM Shruthi Gowda wrote: > > It is not required for PostgresObjectId. The unused_oids script > > provides a list of unused oids in the manually-assignable OIDs range > > (1-). > > Well, so ... why are we not treating the OIDs for these two databases > the same? If there's a range from which we can assign OIDs without > risk of duplication and without needing to update this script, perhaps > we ought to assign both of them from that range and leave the script > alone. >From what I see in the code, template0 and postgres are the last things that get created in initdb phase. The system OIDs that get assigned to these DBs vary from release to release. At present, the system assigned OIDs of template0 and postgres are 13679 and 13680 respectively. I feel it would be safe to assign 16000 and 16001 for template0 and postgres respectively from the unpinned object OID range 12000 - 16383. In the future, even if the initdb unpinned objects reach the range of 16000 issues can only arise if initdb() creates another system-created database for which the system assigns these reserved OIDs (16000, 16001). > + * that is in use in the old cluster is also used in th new cluster - and > > th -> the. Fixed > +preserves the DB, tablespace, relfilenode OIDs so TOAST and other references > > Insert "and" before "relfilenode". Fixed Attached is the latest patch for review. Shruthi KC EnterpriseDB: http://www.enterprisedb.com v12-0001-pg_upgrade-perserve-database-OID-patch.patch Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Thu, Jan 20, 2022 at 11:03 AM Shruthi Gowda wrote: > It is not required for PostgresObjectId. The unused_oids script > provides a list of unused oids in the manually-assignable OIDs range > (1-). Well, so ... why are we not treating the OIDs for these two databases the same? If there's a range from which we can assign OIDs without risk of duplication and without needing to update this script, perhaps we ought to assign both of them from that range and leave the script alone. + * that is in use in the old cluster is also used in th new cluster - and th -> the. +preserves the DB, tablespace, relfilenode OIDs so TOAST and other references Insert "and" before "relfilenode". I think this is in pretty good shape now. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Thu, Jan 20, 2022 at 7:57 PM Robert Haas wrote:
>
> On Thu, Jan 20, 2022 at 7:09 AM Shruthi Gowda wrote:
> > > Here's an updated version in which I've reverted the changes to gram.y
> > > and tried to improve the comments and documentation. Could you have a
> > > look at implementing (2) above?
> >
> > Attached is the patch that implements comment (2).
>
> This probably needs minor rebasing on account of the fact that I just
> pushed the patch to remove datlastsysoid. I intended to do that before
> you posted a new version to save you the trouble, but I was too slow
> (or you were too fast, however you want to look at it).
I have rebased the patch. Kindly have a look at it.
> + errmsg("Invalid value for option \"%s\"", defel->defname),
>
> Per the "error message style" section of the documentation, primary
> error messages neither begin with a capital letter nor end with a
> period, while errdetail() messages are complete sentences and thus
> both begin with a capital letter and end with a period. But what I
> think you should really do here is get rid of the error detail and
> convey all the information in a primary error message. e.g. "OID %u is
> a system OID", or maybe better, "OIDs less than %u are reserved for
> system objects".
Fixed
> + errmsg("database oid %u is already used by database %s",
> + errmsg("data directory exists for database oid %u", dboid));
>
> Usually we write "OID" rather than "oid" in error messages. I think
> maybe it would be best to change the text slightly too. I suggest:
>
> database OID %u is already in use by database \"%s\"
> data directory already exists for database with OID %u
The second error message will be reported when the data directory with
the given OID exists in the data path but the corresponding DB does
not exist. This could happen only if the user creates directories in
the data folder which is indeed not an invalid usage. I have updated
the error message to "data directory with the specified OID %u already
exists" because the error message you recommended gives a slightly
different meaning.
> + * it would fail. To avoid that, assign a fixed OID to template0 and
> + * postgres rather than letting the server choose one.
>
> a fixed OID -> fixed OIDs
> one -> them
>
> Or maybe put this comment back the way I had it and just talk about
> postgres, and then change the comment in make_postgres to say "Assign
> a fixed OID to postgres, for the same reasons as template0."
Fixed
> + /*
> + * Make sure that binary upgrade propagate the database OID to the new
> + * cluster
> + */
>
> This comment doesn't really seem necessary. It's sort of self-explanatory.
Removed
> +# Push the OID that is reserved for template0 database.
> +my $Template0ObjectId =
> + Catalog::FindDefinedSymbol('access/transam.h', '..', 'Template0ObjectId');
> +push @{$oids}, $Template0ObjectId;
>
> Don't you need to do this for PostgresObjectId also?
It is not required for PostgresObjectId. The unused_oids script
provides a list of unused oids in the manually-assignable OIDs range
(1-).
Shruthi KC
EnterpriseDB: http://www.enterprisedb.com
v11-0001-pg_upgrade-perserve-database-OID-patch.patch
Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Thu, Jan 20, 2022 at 7:09 AM Shruthi Gowda wrote:
> > Here's an updated version in which I've reverted the changes to gram.y
> > and tried to improve the comments and documentation. Could you have a
> > look at implementing (2) above?
>
> Attached is the patch that implements comment (2).
This probably needs minor rebasing on account of the fact that I just
pushed the patch to remove datlastsysoid. I intended to do that before
you posted a new version to save you the trouble, but I was too slow
(or you were too fast, however you want to look at it).
+ errmsg("Invalid value for option \"%s\"", defel->defname),
Per the "error message style" section of the documentation, primary
error messages neither begin with a capital letter nor end with a
period, while errdetail() messages are complete sentences and thus
both begin with a capital letter and end with a period. But what I
think you should really do here is get rid of the error detail and
convey all the information in a primary error message. e.g. "OID %u is
a system OID", or maybe better, "OIDs less than %u are reserved for
system objects".
+ errmsg("database oid %u is already used by database %s",
+ errmsg("data directory exists for database oid %u", dboid));
Usually we write "OID" rather than "oid" in error messages. I think
maybe it would be best to change the text slightly too. I suggest:
database OID %u is already in use by database \"%s\"
data directory already exists for database with OID %u
+ * it would fail. To avoid that, assign a fixed OID to template0 and
+ * postgres rather than letting the server choose one.
a fixed OID -> fixed OIDs
one -> them
Or maybe put this comment back the way I had it and just talk about
postgres, and then change the comment in make_postgres to say "Assign
a fixed OID to postgres, for the same reasons as template0."
+ /*
+ * Make sure that binary upgrade propagate the database OID to the new
+ * cluster
+ */
This comment doesn't really seem necessary. It's sort of self-explanatory.
+# Push the OID that is reserved for template0 database.
+my $Template0ObjectId =
+ Catalog::FindDefinedSymbol('access/transam.h', '..', 'Template0ObjectId');
+push @{$oids}, $Template0ObjectId;
Don't you need to do this for PostgresObjectId also?
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Jan 18, 2022 at 2:34 AM Robert Haas wrote: > > On Mon, Jan 17, 2022 at 9:57 AM Shruthi Gowda wrote: > > I have rebased and generated the patches on top of PostgreSQL commit > > ID cf925936ecc031355cd56fbd392ec3180517a110. > > Kindly apply v8-0001-pg_upgrade-Preserve-relfilenodes-and-tablespace-O.patch > > first and then v8-0002-Preserve-database-OIDs-in-pg_upgrade.patch. > > OK, so looking over 0002, I noticed a few things: > > 1. datlastsysoid isn't being used for anything any more. That's not a > defect in your patch, but I've separately proposed to remove it. okay > 2. I realized that the whole idea here depends on not having initdb > create more than one database without a fixed OID. The patch solves > that by nailing down the OID of template0, which is a sufficient > solution. However, I think nailing down the (initial) OID of postgres > as well would be a good idea, just in case somebody in the future > decides to add another system-created database. I agree with your thought. In my latest patch, postgres db gets created with a fixed OID. I have chosen an arbitrary number 16000 as postgres OID from the unpinned object OID range1200 - 16383. > 3. The changes to gram.y don't do anything. Sure, you've added a new > "OID" token, but nothing generates that token, so it has no effect. > The reason the syntax works is that createdb_opt_name accepts "IDENT", > which means any string that's not in the keyword list (see kwlist.h). > But that's there already, so you don't need to do anything in this > file. okay > 4. I felt that the documentation and comments could be somewhat improved. The documentation and comment updates are more accurate with the required details. Thanks. > Here's an updated version in which I've reverted the changes to gram.y > and tried to improve the comments and documentation. Could you have a > look at implementing (2) above? Attached is the patch that implements comment (2). Shruthi KC EnterpriseDB: http://www.enterprisedb.com v10-0001-pg_upgrade-perserve-database-OID-patch.patch Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Jan 18, 2022 at 12:35 AM Robert Haas wrote: > > On Tue, Dec 14, 2021 at 1:21 PM Shruthi Gowda wrote: > > Thanks, Robert for the updated version. I reviewed the changes and it > > looks fine. > > I also tested the patch. The patch works as expected. > > Thanks. > > > > - I adjusted the function header comment for heap_create. Your > > > proposed comment seemed like it was pretty detailed but not 100% > > > correct. It also made one of the lines kind of long because you didn't > > > wrap the text in the surrounding style. I decided to make it simpler > > > and shorter instead of longer still and 100% correct. > > > > The comment update looks fine. However, I still feel it would be good to > > mention on which (rare) circumstance a valid relfilenode can get passed. > > In general, I think it's the job of a function parameter comment to > describe what the parameter does, not how the callers actually use it. > One problem with describing the latter is that, if someone later adds > another caller, there is a pretty good chance that they won't notice > that the comment needs to be changed. More fundamentally, the > parameter function comments should be like an instruction manual for > how to use the function. If you are trying to figure out how to use > this function, it is not helpful to know that "most callers like to > pass false" for this parameter. What you need to know is what value > your new call site should pass, and knowing what "most callers" do or > that something is "rare" doesn't really help. If we want to make this > comment more detailed, we should approach it from the point of view of > explaining how it ought to be set. It's clear now. Thanks for clarifying. > I've committed the v8-0001 patch you attached. I'll write separately > about v8-0002. Sure. Thank you.
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Mon, Jan 17, 2022 at 9:57 AM Shruthi Gowda wrote: > I have rebased and generated the patches on top of PostgreSQL commit > ID cf925936ecc031355cd56fbd392ec3180517a110. > Kindly apply v8-0001-pg_upgrade-Preserve-relfilenodes-and-tablespace-O.patch > first and then v8-0002-Preserve-database-OIDs-in-pg_upgrade.patch. OK, so looking over 0002, I noticed a few things: 1. datlastsysoid isn't being used for anything any more. That's not a defect in your patch, but I've separately proposed to remove it. 2. I realized that the whole idea here depends on not having initdb create more than one database without a fixed OID. The patch solves that by nailing down the OID of template0, which is a sufficient solution. However, I think nailing down the (initial) OID of postgres as well would be a good idea, just in case somebody in the future decides to add another system-created database. 3. The changes to gram.y don't do anything. Sure, you've added a new "OID" token, but nothing generates that token, so it has no effect. The reason the syntax works is that createdb_opt_name accepts "IDENT", which means any string that's not in the keyword list (see kwlist.h). But that's there already, so you don't need to do anything in this file. 4. I felt that the documentation and comments could be somewhat improved. Here's an updated version in which I've reverted the changes to gram.y and tried to improve the comments and documentation. Could you have a look at implementing (2) above? -- Robert Haas EDB: http://www.enterprisedb.com v9-0001-pg_upgrade-perserve-database-OID-patch-from-shrut.patch Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Dec 14, 2021 at 1:21 PM Shruthi Gowda wrote: > Thanks, Robert for the updated version. I reviewed the changes and it > looks fine. > I also tested the patch. The patch works as expected. Thanks. > > - I adjusted the function header comment for heap_create. Your > > proposed comment seemed like it was pretty detailed but not 100% > > correct. It also made one of the lines kind of long because you didn't > > wrap the text in the surrounding style. I decided to make it simpler > > and shorter instead of longer still and 100% correct. > > The comment update looks fine. However, I still feel it would be good to > mention on which (rare) circumstance a valid relfilenode can get passed. In general, I think it's the job of a function parameter comment to describe what the parameter does, not how the callers actually use it. One problem with describing the latter is that, if someone later adds another caller, there is a pretty good chance that they won't notice that the comment needs to be changed. More fundamentally, the parameter function comments should be like an instruction manual for how to use the function. If you are trying to figure out how to use this function, it is not helpful to know that "most callers like to pass false" for this parameter. What you need to know is what value your new call site should pass, and knowing what "most callers" do or that something is "rare" doesn't really help. If we want to make this comment more detailed, we should approach it from the point of view of explaining how it ought to be set. I've committed the v8-0001 patch you attached. I'll write separately about v8-0002. -- Robert Haas EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Sat, Jan 15, 2022 at 11:17 AM Julien Rouhaud wrote: > > Hi, > > On Fri, Dec 17, 2021 at 01:03:06PM +0530, Shruthi Gowda wrote: > > > > I have updated the DBOID preserve patch to handle this case and > > generated the latest patch on top of your v7-001-preserve-relfilenode > > patch. > > The cfbot reports that the patch doesn't apply anymore: > http://cfbot.cputube.org/patch_36_3296.log > === Applying patches on top of PostgreSQL commit ID > 5513dc6a304d8bda114004a3b906cc6fde5d6274 === > === applying patch ./v7-0002-Preserve-database-OIDs-in-pg_upgrade.patch > [...] > patching file src/bin/pg_upgrade/info.c > Hunk #1 FAILED at 190. > Hunk #2 succeeded at 351 (offset 27 lines). > 1 out of 2 hunks FAILED -- saving rejects to file > src/bin/pg_upgrade/info.c.rej > patching file src/bin/pg_upgrade/pg_upgrade.h > Hunk #1 FAILED at 145. > 1 out of 1 hunk FAILED -- saving rejects to file > src/bin/pg_upgrade/pg_upgrade.h.rej > patching file src/bin/pg_upgrade/relfilenode.c > Hunk #1 FAILED at 193. > 1 out of 1 hunk FAILED -- saving rejects to file > src/bin/pg_upgrade/relfilenode.c.rej > > Could you send a rebased version? In the meantime I willl switch the cf entry > to Waiting on Author. I have rebased and generated the patches on top of PostgreSQL commit ID cf925936ecc031355cd56fbd392ec3180517a110. Kindly apply v8-0001-pg_upgrade-Preserve-relfilenodes-and-tablespace-O.patch first and then v8-0002-Preserve-database-OIDs-in-pg_upgrade.patch. Thanks & Regards, Shruthi KC EnterpriseDB: http://www.enterprisedb.com v8-0002-Preserve-database-OIDs-in-pg_upgrade.patch Description: Binary data v8-0001-pg_upgrade-Preserve-relfilenodes-and-tablespace-O.patch Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Hi, On Fri, Dec 17, 2021 at 01:03:06PM +0530, Shruthi Gowda wrote: > > I have updated the DBOID preserve patch to handle this case and > generated the latest patch on top of your v7-001-preserve-relfilenode > patch. The cfbot reports that the patch doesn't apply anymore: http://cfbot.cputube.org/patch_36_3296.log === Applying patches on top of PostgreSQL commit ID 5513dc6a304d8bda114004a3b906cc6fde5d6274 === === applying patch ./v7-0002-Preserve-database-OIDs-in-pg_upgrade.patch [...] patching file src/bin/pg_upgrade/info.c Hunk #1 FAILED at 190. Hunk #2 succeeded at 351 (offset 27 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/bin/pg_upgrade/info.c.rej patching file src/bin/pg_upgrade/pg_upgrade.h Hunk #1 FAILED at 145. 1 out of 1 hunk FAILED -- saving rejects to file src/bin/pg_upgrade/pg_upgrade.h.rej patching file src/bin/pg_upgrade/relfilenode.c Hunk #1 FAILED at 193. 1 out of 1 hunk FAILED -- saving rejects to file src/bin/pg_upgrade/relfilenode.c.rej Could you send a rebased version? In the meantime I willl switch the cf entry to Waiting on Author.
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Mon, Dec 13, 2021 at 8:43 PM Shruthi Gowda wrote:
>
> On Mon, Dec 6, 2021 at 11:25 PM Robert Haas wrote:
> >
> > On Sun, Dec 5, 2021 at 11:44 PM Sadhuprasad Patro wrote:
> > > 3.
> > > @@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt
> > > *stmt)
> > > */
> > > pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
> > >
> > > - do
> > > + /* Select an OID for the new database if is not explicitly configured.
> > > */
> > > + if (!OidIsValid(dboid))
> > > {
> > > - dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
> > > -Anum_pg_database_oid);
> > > - } while (check_db_file_conflict(dboid));
> > >
> > > I think we need to do 'check_db_file_conflict' for the USER given OID
> > > also.. right? It may already be present.
> >
> > Hopefully, if that happens, we straight up fail later on.
>
> That's right. If a database with user-specified OID exists, the
> createdb fails with a "duplicate key value" error.
> If just a data directory with user-specified OID exists,
> MakePGDirectory() fails to create the directory and the cleanup
> callback createdb_failure_callback() removes the directory that was
> not created by 'createdb()' function.
> The subsequent create database call with the same OID will succeed.
> Should we handle the case where a data directory exists and the
> corresponding DB with that oid does not exist? I presume this
> situation doesn't arise unless the user tries to create directories in
> the data path. Any thoughts?
I have updated the DBOID preserve patch to handle this case and
generated the latest patch on top of your v7-001-preserve-relfilenode
patch.
Thanks & Regards
Shruthi KC
EnterpriseDB: http://www.enterprisedb.com
v7-0002-Preserve-database-OIDs-in-pg_upgrade.patch
Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On 12/15/21 12:09 AM, tushar wrote: I spent much of today reviewing 0001. Here's an updated version, so far only lightly tested. Please check whether I've broken anything. Thanks Robert, I tested from v96/12/13/v14 -> v15( with patch) things are working fine i.e table /index relfilenode is preserved, not changing after pg_upgrade. I covered tablespace OIDs testing scenarios and that is also preserved after pg_upgrade. -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On 12/14/21 2:35 AM, Robert Haas wrote: I spent much of today reviewing 0001. Here's an updated version, so far only lightly tested. Please check whether I've broken anything. Thanks Robert, I tested from v96/12/13/v14 -> v15( with patch) things are working fine i.e table /index relfilenode is preserved, not changing after pg_upgrade. -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Dec 14, 2021 at 2:35 AM Robert Haas wrote: > > On Mon, Dec 13, 2021 at 9:40 AM Shruthi Gowda wrote: > > > I am reviewing another patch > > > "v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well > > > and will provide the comments soon if any... > > I spent much of today reviewing 0001. Here's an updated version, so > far only lightly tested. Please check whether I've broken anything. > Here are the changes: Thanks, Robert for the updated version. I reviewed the changes and it looks fine. I also tested the patch. The patch works as expected. > - I adjusted the function header comment for heap_create. Your > proposed comment seemed like it was pretty detailed but not 100% > correct. It also made one of the lines kind of long because you didn't > wrap the text in the surrounding style. I decided to make it simpler > and shorter instead of longer still and 100% correct. The comment update looks fine. However, I still feel it would be good to mention on which (rare) circumstance a valid relfilenode can get passed. > - I removed a one-line comment that said /* Override the toast > relfilenode */ because it preceded an error check, not a line of code > that would have done what the comment claimed. > > - I removed a one-line comment that said /* Override the relfilenode > */ because the following code would only sometimes override the > relfilenode. The code didn't seem complex enough to justify a a longer > and more accurate comment, so I just took it out. Fine > - I changed a test for (relkind == RELKIND_RELATION || relkind == > RELKIND_SEQUENCE || relkind == RELKIND_MATVIEW) to use > RELKIND_HAS_STORAGE(). It's true that not all of the storage types > that RELKIND_HAS_STORAGE() tests are possible here, but that's not a > reason to avoiding using the macro. If somebody adds a new relkind > with storage in the future, they might miss the need to manually > update this place, but they will not likely miss the need to update > RELKIND_HAS_STORAGE() since, if they did, their code probably wouldn't > work at all. I agree. > - I changed the way that you were passing create_storage down to > heap_create. I think I said before that you should EITHER fix it so > that we set create_storage = true only when the relation actually has > storage OR ELSE have heap_create() itself override the value to false > when there is no storage. You did both. There are times when it's > reasonable to ensure the same thing in multiple places, but this > doesn't seem to be one of them. So I took that out. I chose to retain > the code in heap_create() that overrides the value to false, added a > comment explaining that it does that, and then adjusted the callers to > ignore the storage type. I then added comments, and in one place an > assertion, to make it clearer what is happening. The changes are fine. Thanks for the fine-tuning. > - In pg_dump.c, I adjusted the comment that says "Not every relation > has storage." and the test that immediately follows, to ignore the > relfilenode when relkind says it's a partitioned table. Really, > partitioned tables should never have had relfilenodes, but as it turns > out, they used to have them. > Fine. Understood. Thanks & Regards, Shruthi KC EnterpriseDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Mon, Dec 13, 2021 at 9:40 AM Shruthi Gowda wrote: > > I am reviewing another patch > > "v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well > > and will provide the comments soon if any... I spent much of today reviewing 0001. Here's an updated version, so far only lightly tested. Please check whether I've broken anything. Here are the changes: - I adjusted the function header comment for heap_create. Your proposed comment seemed like it was pretty detailed but not 100% correct. It also made one of the lines kind of long because you didn't wrap the text in the surrounding style. I decided to make it simpler and shorter instead of longer still and 100% correct. - I removed a one-line comment that said /* Override the toast relfilenode */ because it preceded an error check, not a line of code that would have done what the comment claimed. - I removed a one-line comment that said /* Override the relfilenode */ because the following code would only sometimes override the relfilenode. The code didn't seem complex enough to justify a a longer and more accurate comment, so I just took it out. - I changed a test for (relkind == RELKIND_RELATION || relkind == RELKIND_SEQUENCE || relkind == RELKIND_MATVIEW) to use RELKIND_HAS_STORAGE(). It's true that not all of the storage types that RELKIND_HAS_STORAGE() tests are possible here, but that's not a reason to avoiding using the macro. If somebody adds a new relkind with storage in the future, they might miss the need to manually update this place, but they will not likely miss the need to update RELKIND_HAS_STORAGE() since, if they did, their code probably wouldn't work at all. - I changed the way that you were passing create_storage down to heap_create. I think I said before that you should EITHER fix it so that we set create_storage = true only when the relation actually has storage OR ELSE have heap_create() itself override the value to false when there is no storage. You did both. There are times when it's reasonable to ensure the same thing in multiple places, but this doesn't seem to be one of them. So I took that out. I chose to retain the code in heap_create() that overrides the value to false, added a comment explaining that it does that, and then adjusted the callers to ignore the storage type. I then added comments, and in one place an assertion, to make it clearer what is happening. - In pg_dump.c, I adjusted the comment that says "Not every relation has storage." and the test that immediately follows, to ignore the relfilenode when relkind says it's a partitioned table. Really, partitioned tables should never have had relfilenodes, but as it turns out, they used to have them. Let me know your thoughts. -- Robert Haas EDB: http://www.enterprisedb.com v7-0001-pg_upgrade-Preserve-relfilenodes-and-tablespace-O.patch Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Mon, Dec 6, 2021 at 11:25 PM Robert Haas wrote:
>
> On Sun, Dec 5, 2021 at 11:44 PM Sadhuprasad Patro wrote:
> > 3.
> > @@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
> > */
> > pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
> >
> > - do
> > + /* Select an OID for the new database if is not explicitly configured. */
> > + if (!OidIsValid(dboid))
> > {
> > - dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
> > -Anum_pg_database_oid);
> > - } while (check_db_file_conflict(dboid));
> >
> > I think we need to do 'check_db_file_conflict' for the USER given OID
> > also.. right? It may already be present.
>
> Hopefully, if that happens, we straight up fail later on.
That's right. If a database with user-specified OID exists, the
createdb fails with a "duplicate key value" error.
If just a data directory with user-specified OID exists,
MakePGDirectory() fails to create the directory and the cleanup
callback createdb_failure_callback() removes the directory that was
not created by 'createdb()' function.
The subsequent create database call with the same OID will succeed.
Should we handle the case where a data directory exists and the
corresponding DB with that oid does not exist? I presume this
situation doesn't arise unless the user tries to create directories in
the data path. Any thoughts?
Thanks & Regards
Shruthi KC
EnterpriseDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Mon, Dec 6, 2021 at 10:14 AM Sadhuprasad Patro wrote:
>
> On Tue, Oct 26, 2021 at 6:55 PM Shruthi Gowda wrote:
> >
> >
> > I have revised the patch w.r.t the way 'create_storage' is interpreted
> > in heap_create() along with some minor changes to preserve the DBOID
> > patch.
> >
>
> Hi Shruthi,
>
> I am reviewing the attached patches and providing a few comments here
> below for patch "v5-0002-Preserve-database-OIDs-in-pg_upgrade.patch"
>
> 1.
> --- a/doc/src/sgml/ref/create_database.sgml
> +++ b/doc/src/sgml/ref/create_database.sgml
> @@ -31,7 +31,8 @@ CREATE DATABASE class="parameter">name
> - [ IS_TEMPLATE [=] class="parameter">istemplate ] ]
> + [ IS_TEMPLATE [=] class="parameter">istemplate ]
> + [ OID [=] class="parameter">db_oid ] ]
>
> Replace "db_oid" with 'oid'. Below in the listitem, we have mentioned 'oid'.
Replaced "db_oid" with "oid"
>
> 2.
> --- a/src/backend/commands/dbcommands.c
> +++ b/src/backend/commands/dbcommands.c
> + if ((dboid < FirstNormalObjectId) &&
> + (strcmp(dbname, "template0") != 0) &&
> + (!IsBinaryUpgrade))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
> + errmsg("Invalid value for option \"%s\"", defel->defname),
> + errhint("The specified OID %u is less than the minimum OID for user
> objects %u.",
> + dboid, FirstNormalObjectId));
> + }
>
> Are we sure that 'IsBinaryUpgrade' will be set properly, before the
> createdb function is called? Can we recheck once ?
I believe 'IsBinaryUpgrade' will be set to true when pg_upgrade is invoked.
pg_ugrade internally does pg_dump and pg_restore for every database in
the cluster.
> 3.
> @@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
> */
> pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
>
> - do
> + /* Select an OID for the new database if is not explicitly configured. */
> + if (!OidIsValid(dboid))
> {
> - dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
> -Anum_pg_database_oid);
> - } while (check_db_file_conflict(dboid));
>
> I think we need to do 'check_db_file_conflict' for the USER given OID
> also.. right? It may already be present.
If a datafile with user-specified OID exists, the create database
fails with the below error.
postgres=# create database d2 oid 16452;
ERROR: could not create directory "base/16452": File exists
> 4.
> --- a/src/bin/initdb/initdb.c
> +++ b/src/bin/initdb/initdb.c
>
> /*
> + * Create template0 database with oid Template0ObjectId i.e, 4
> + */
> +
>
> Better to mention here, why OID 4 is reserved for template0 database?.
The comment is updated to explain why template0 oid is fixed.
> 5.
> + /*
> + * Create template0 database with oid Template0ObjectId i.e, 4
> + */
> + static const char *const template0_setup[] = {
> + "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID
> "
> + CppAsString2(Template0ObjectId) ";\n\n",
>
> Can we write something like, 'OID = CppAsString2(Template0ObjectId)'?
> mention "=".
Fixed
> 6.
> +
> + /*
> + * We use the OID of postgres to determine datlastsysoid
> + */
> + "UPDATE pg_database SET datlastsysoid = "
> + "(SELECT oid FROM pg_database "
> + "WHERE datname = 'postgres');\n\n",
> +
>
> Make the above comment a single line comment.
As Robert confirmed, this part of the code is moved from a different place.
> 7.
> There are some spelling mistakes in the comments as below, please
> correct the same
> + /*
> + * Make sure that binary upgrade propogate the database OID to the
> new => correct spelling
> + * cluster
> + */
>
> +/* OID 4 is reserved for Templete0 database */
> > Correct spelling
> +#define Template0ObjectId 4
>
Fixed.
> I am reviewing another patch
> "v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well
> and will provide the comments soon if any...
Thanks. I have rebased relfilenode oid preserve patch. You may use the
rebased patch for review.
Thanks & Regards
Shruthi K C
EnterpriseDB: http://www.enterprisedb.com
v6-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg.patch
Description: Binary data
v6-0002-Preserve-database-OIDs-in-pg_upgrade.patch
Description: Binary data
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Sun, Dec 5, 2021 at 11:44 PM Sadhuprasad Patro wrote:
> 1.
> --- a/doc/src/sgml/ref/create_database.sgml
> +++ b/doc/src/sgml/ref/create_database.sgml
> @@ -31,7 +31,8 @@ CREATE DATABASE class="parameter">name
> - [ IS_TEMPLATE [=] class="parameter">istemplate ] ]
> + [ IS_TEMPLATE [=] class="parameter">istemplate ]
> + [ OID [=] class="parameter">db_oid ] ]
>
> Replace "db_oid" with 'oid'. Below in the listitem, we have mentioned 'oid'.
I agree that the listitem and the synopsis need to be consistent, but
it could be made consistent either by changing that one to db_oid or
this one to oid.
> 2.
> --- a/src/backend/commands/dbcommands.c
> +++ b/src/backend/commands/dbcommands.c
> + if ((dboid < FirstNormalObjectId) &&
> + (strcmp(dbname, "template0") != 0) &&
> + (!IsBinaryUpgrade))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
> + errmsg("Invalid value for option \"%s\"", defel->defname),
> + errhint("The specified OID %u is less than the minimum OID for user
> objects %u.",
> + dboid, FirstNormalObjectId));
> + }
>
> Are we sure that 'IsBinaryUpgrade' will be set properly, before the
> createdb function is called? Can we recheck once ?
How could it be set incorrectly, and how could we recheck this?
> 3.
> @@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
> */
> pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
>
> - do
> + /* Select an OID for the new database if is not explicitly configured. */
> + if (!OidIsValid(dboid))
> {
> - dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
> -Anum_pg_database_oid);
> - } while (check_db_file_conflict(dboid));
>
> I think we need to do 'check_db_file_conflict' for the USER given OID
> also.. right? It may already be present.
Hopefully, if that happens, we straight up fail later on.
> 4.
> --- a/src/bin/initdb/initdb.c
> +++ b/src/bin/initdb/initdb.c
>
> /*
> + * Create template0 database with oid Template0ObjectId i.e, 4
> + */
> +
>
> Better to mention here, why OID 4 is reserved for template0 database?.
I'm not sure how we would give a reason for selecting an arbitrary
constant? We could perhaps explain why we use a fixed OID. But there's
no reason it has to be 4, I think.
> 5.
> + /*
> + * Create template0 database with oid Template0ObjectId i.e, 4
> + */
> + static const char *const template0_setup[] = {
> + "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID
> "
> + CppAsString2(Template0ObjectId) ";\n\n",
>
> Can we write something like, 'OID = CppAsString2(Template0ObjectId)'?
> mention "=".
That seems like a good idea, because it would be more consistent.
> 6.
> +
> + /*
> + * We use the OID of postgres to determine datlastsysoid
> + */
> + "UPDATE pg_database SET datlastsysoid = "
> + "(SELECT oid FROM pg_database "
> + "WHERE datname = 'postgres');\n\n",
> +
>
> Make the above comment a single line comment.
I think what Shruthi did is more correct. It doesn't have to be done
as a single-line comment just because it can fit on one line. And
Shruthi didn't write this comment anyway, it's only moved slightly
from where it was before.
> 7.
> There are some spelling mistakes in the comments as below, please
> correct the same
> + /*
> + * Make sure that binary upgrade propogate the database OID to the
> new => correct spelling
> + * cluster
> + */
>
> +/* OID 4 is reserved for Templete0 database */
> > Correct spelling
> +#define Template0ObjectId 4
Yes, those would be good to fix.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Oct 26, 2021 at 6:55 PM Shruthi Gowda wrote:
>
>
> I have revised the patch w.r.t the way 'create_storage' is interpreted
> in heap_create() along with some minor changes to preserve the DBOID
> patch.
>
Hi Shruthi,
I am reviewing the attached patches and providing a few comments here
below for patch "v5-0002-Preserve-database-OIDs-in-pg_upgrade.patch"
1.
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -31,7 +31,8 @@ CREATE DATABASE name
- [ IS_TEMPLATE [=] istemplate ] ]
+ [ IS_TEMPLATE [=] istemplate ]
+ [ OID [=] db_oid ] ]
Replace "db_oid" with 'oid'. Below in the listitem, we have mentioned 'oid'.
2.
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
+ if ((dboid < FirstNormalObjectId) &&
+ (strcmp(dbname, "template0") != 0) &&
+ (!IsBinaryUpgrade))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("Invalid value for option \"%s\"", defel->defname),
+ errhint("The specified OID %u is less than the minimum OID for user
objects %u.",
+ dboid, FirstNormalObjectId));
+ }
Are we sure that 'IsBinaryUpgrade' will be set properly, before the
createdb function is called? Can we recheck once ?
3.
@@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
*/
pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
- do
+ /* Select an OID for the new database if is not explicitly configured. */
+ if (!OidIsValid(dboid))
{
- dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
-Anum_pg_database_oid);
- } while (check_db_file_conflict(dboid));
I think we need to do 'check_db_file_conflict' for the USER given OID
also.. right? It may already be present.
4.
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
/*
+ * Create template0 database with oid Template0ObjectId i.e, 4
+ */
+
Better to mention here, why OID 4 is reserved for template0 database?.
5.
+ /*
+ * Create template0 database with oid Template0ObjectId i.e, 4
+ */
+ static const char *const template0_setup[] = {
+ "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID "
+ CppAsString2(Template0ObjectId) ";\n\n",
Can we write something like, 'OID = CppAsString2(Template0ObjectId)'?
mention "=".
6.
+
+ /*
+ * We use the OID of postgres to determine datlastsysoid
+ */
+ "UPDATE pg_database SET datlastsysoid = "
+ "(SELECT oid FROM pg_database "
+ "WHERE datname = 'postgres');\n\n",
+
Make the above comment a single line comment.
7.
There are some spelling mistakes in the comments as below, please
correct the same
+ /*
+ * Make sure that binary upgrade propogate the database OID to the
new => correct spelling
+ * cluster
+ */
+/* OID 4 is reserved for Templete0 database */
> Correct spelling
+#define Template0ObjectId 4
I am reviewing another patch
"v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well
and will provide the comments soon if any...
Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Tue, Oct 12, 2021 at 10:26:54AM -0400, Robert Haas wrote: > > specifically: Appendix C: Tweaks > > > > Quoting a couple of paragraphs from that appendix: > > > > """ > > In general, if there is information that is available and statically > > associated with a plaintext, it is recommended to use that information > > as a tweak for the plaintext. Ideally, the non-secret tweak associated > > with a plaintext is associated only with that plaintext. > > > > Extensive tweaking means that fewer plaintexts are encrypted under any > > given tweak. This corresponds, in the security model that is described > > in [1], to fewer queries to the target instance of the encryption. > > """ > > > > The gist of this being- the more diverse the tweaking being used, the > > better. That's where I was going with my "limit the risk" comment. If > > we can make the tweak vary more for a given encryption invokation, > > that's going to be better, pretty much by definition, and as explained > > in publications by NIST. > > I mean I don't have anything against that appendix, but I think we > need to understand - with confidence - what the expectations are > specifically around XTS, and that appendix seems much more general > than that. Since there has not been activity on this thread for one month, I have updated the Postgres TDE wiki to include the conclusions and discussions from this thread: https://wiki.postgresql.org/wiki/Transparent_Data_Encryption -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Sasasu wrote: > On 2021/10/6 23:01, Robert Haas wrote: > > This seems wrong to me. CTR requires that you not reuse the IV. If you > > re-encrypt the page with a different IV, torn pages are a problem. If > > you re-encrypt it with the same IV, then it's not secure any more. > for CBC if the IV is predictable will case "dictionary attack". The following sounds like IV *uniqueness* is needed to defend against "known plaintext attack" ... > and for CBC and GCM reuse IV will case "known plaintext attack". ... but here you seem to say that *randomness* is also necessary: > XTS works like CBC but adds a tweak step. the tweak step does not add > randomness. It means XTS still has "known plaintext attack", (I suppose you mean "XTS with incorrect (e.g. non-random) IV", rather than XTS as such.) > due to the same reason from CBC. According to the Appendix C of https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a.pdf CBC requires *unpredictability* of the IV, but that does not necessarily mean randomness: the unpredictable IV can be obtained by applying the forward cipher function to an unique value. Can you please try to explain once again what you consider a requirement (uniqueness, randomness, etc.) on the IV for the XTS mode? Thanks. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Thu, Oct 7, 2021 at 7:33 PM Robert Haas wrote: > > On Thu, Oct 7, 2021 at 3:24 AM Shruthi Gowda wrote: > > Every other > > caller/flow passes false for 'create_storage' and we still need to > > create storage in heap_create() if relkind has storage. > > That seems surprising. I have revised the patch w.r.t the way 'create_storage' is interpreted in heap_create() along with some minor changes to preserve the DBOID patch. Regards, Shruthi KC EnterpriseDB: http://www.enterprisedb.com v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg.patch Description: Binary data v5-0002-Preserve-database-OIDs-in-pg_upgrade.patch Description: Binary data
Re: storing an explicit nonce
On Wed, Oct 13, 2021 at 09:16:37AM -0400, Stephen Frost wrote: > Greetings, > > * Ants Aasma (a...@cybertec.at) wrote: > > On Wed, 13 Oct 2021 at 02:20, Bruce Momjian wrote: > > > On Wed, Oct 13, 2021 at 12:48:51AM +0300, Ants Aasma wrote: > > > > On Wed, 13 Oct 2021 at 00:25, Bruce Momjian wrote: > > > > > > > > On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: > > > > > Page encrypting to all zeros is for all practical purposes > > > impossible to > > > > hit. > > > > > Basically an attacker would have to be able to arbitrarily set the > > > whole > > > > > contents of the page and they would then achieve that this page > > > gets > > > > ignored. > > > > > > > > Uh, how do we know that valid data can't produce an encrypted > > > all-zero > > > > page? > > > > > > > > > > > > Because the chances of that happening by accident are equivalent to > > > making a > > > > series of commits to postgres and ending up with the same git commit > > > hash 400 > > > > times in a row. > > > > > > Yes, 256^8192 is 1e+19728, but why not just assume a page LSN=0 is an > > > empty page, and if not, an error? Seems easier than checking if each > > > page contains all zeros every time. > > > > > > > We already check it anyway, see PageIsVerifiedExtended(). > > Right- we check the LSN along with the rest of the page there. Very good. I have not looked at the Cybertec patch recently. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Ants Aasma (a...@cybertec.at) wrote: > On Wed, 13 Oct 2021 at 02:20, Bruce Momjian wrote: > > On Wed, Oct 13, 2021 at 12:48:51AM +0300, Ants Aasma wrote: > > > On Wed, 13 Oct 2021 at 00:25, Bruce Momjian wrote: > > > > > > On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: > > > > Page encrypting to all zeros is for all practical purposes > > impossible to > > > hit. > > > > Basically an attacker would have to be able to arbitrarily set the > > whole > > > > contents of the page and they would then achieve that this page > > gets > > > ignored. > > > > > > Uh, how do we know that valid data can't produce an encrypted > > all-zero > > > page? > > > > > > > > > Because the chances of that happening by accident are equivalent to > > making a > > > series of commits to postgres and ending up with the same git commit > > hash 400 > > > times in a row. > > > > Yes, 256^8192 is 1e+19728, but why not just assume a page LSN=0 is an > > empty page, and if not, an error? Seems easier than checking if each > > page contains all zeros every time. > > > > We already check it anyway, see PageIsVerifiedExtended(). Right- we check the LSN along with the rest of the page there. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Wed, 13 Oct 2021 at 02:20, Bruce Momjian wrote: > On Wed, Oct 13, 2021 at 12:48:51AM +0300, Ants Aasma wrote: > > On Wed, 13 Oct 2021 at 00:25, Bruce Momjian wrote: > > > > On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: > > > Page encrypting to all zeros is for all practical purposes > impossible to > > hit. > > > Basically an attacker would have to be able to arbitrarily set the > whole > > > contents of the page and they would then achieve that this page > gets > > ignored. > > > > Uh, how do we know that valid data can't produce an encrypted > all-zero > > page? > > > > > > Because the chances of that happening by accident are equivalent to > making a > > series of commits to postgres and ending up with the same git commit > hash 400 > > times in a row. > > Yes, 256^8192 is 1e+19728, but why not just assume a page LSN=0 is an > empty page, and if not, an error? Seems easier than checking if each > page contains all zeros every time. > We already check it anyway, see PageIsVerifiedExtended(). -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com
Re: storing an explicit nonce
On Wed, Oct 13, 2021 at 12:48:51AM +0300, Ants Aasma wrote: > On Wed, 13 Oct 2021 at 00:25, Bruce Momjian wrote: > > On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: > > Page encrypting to all zeros is for all practical purposes impossible to > hit. > > Basically an attacker would have to be able to arbitrarily set the whole > > contents of the page and they would then achieve that this page gets > ignored. > > Uh, how do we know that valid data can't produce an encrypted all-zero > page? > > > Because the chances of that happening by accident are equivalent to making a > series of commits to postgres and ending up with the same git commit hash 400 > times in a row. Yes, 256^8192 is 1e+19728, but why not just assume a page LSN=0 is an empty page, and if not, an error? Seems easier than checking if each page contains all zeros every time. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, On Tue, Oct 12, 2021 at 17:49 Ants Aasma wrote: > > On Wed, 13 Oct 2021 at 00:25, Bruce Momjian wrote: > >> On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: >> > On Tue, 12 Oct 2021 at 16:14, Bruce Momjian wrote: >> > >> > Well, how do you detect an all-zero page vs a page that encrypted >> to all >> > zeros? >> > >> > Page encrypting to all zeros is for all practical purposes impossible >> to hit. >> > Basically an attacker would have to be able to arbitrarily set the whole >> > contents of the page and they would then achieve that this page gets >> ignored. >> >> Uh, how do we know that valid data can't produce an encrypted all-zero >> page? >> > > Because the chances of that happening by accident are equivalent to making > a series of commits to postgres and ending up with the same git commit hash > 400 times in a row. > And to then have a valid checksum … seems next to impossible. Thanks, Stephen >
Re: storing an explicit nonce
On Wed, 13 Oct 2021 at 00:25, Bruce Momjian wrote: > On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: > > On Tue, 12 Oct 2021 at 16:14, Bruce Momjian wrote: > > > > Well, how do you detect an all-zero page vs a page that encrypted to > all > > zeros? > > > > Page encrypting to all zeros is for all practical purposes impossible to > hit. > > Basically an attacker would have to be able to arbitrarily set the whole > > contents of the page and they would then achieve that this page gets > ignored. > > Uh, how do we know that valid data can't produce an encrypted all-zero > page? > Because the chances of that happening by accident are equivalent to making a series of commits to postgres and ending up with the same git commit hash 400 times in a row. -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com
Re: storing an explicit nonce
On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote: > On Tue, 12 Oct 2021 at 16:14, Bruce Momjian wrote: > > Well, how do you detect an all-zero page vs a page that encrypted to all > zeros? > > Page encrypting to all zeros is for all practical purposes impossible to hit. > Basically an attacker would have to be able to arbitrarily set the whole > contents of the page and they would then achieve that this page gets ignored. Uh, how do we know that valid data can't produce an encrypted all-zero page? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Tue, 12 Oct 2021 at 16:14, Bruce Momjian wrote: > Well, how do you detect an all-zero page vs a page that encrypted to all > zeros? > Page encrypting to all zeros is for all practical purposes impossible to hit. Basically an attacker would have to be able to arbitrarily set the whole contents of the page and they would then achieve that this page gets ignored. -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com
Re: storing an explicit nonce
On Tue, Oct 12, 2021 at 10:39 AM Stephen Frost wrote: > Using fake LSNs isn't new.. how is this not a concern already then? > > Also wondering why the buffer manager would care about the LSN on pages > which are not BM_PERMANENT..? > > I'll admit that I might certainly be missing something here. Oh, FlushBuffer has a guard against this case in it. I hadn't realized that. Sorry for the noise. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Oct 11, 2021 at 1:30 PM Stephen Frost wrote: > > Regarding unlogged LSNs at least, I would think that we'd want to > > actually use GetFakeLSNForUnloggedRel() instead of just having it zero'd > > out. The fixed value for GiST index pages is just during the index > > build process, as I recall, and that's perhaps less of a concern. Part > > of the point of using XTS is to avoid the issue of the LSN not being > > changed when hint bits are, or more generally not being unique in > > various cases. > > I don't believe there's anything to prevent the fake-LSN counter from > overtaking the real end-of-WAL, and if that should happen, then the > buffer manager would get confused. Maybe that can be fixed by doing > some sort of surgery on the buffer manager, but it doesn't seem to be > a trivial or ignorable problem. Using fake LSNs isn't new.. how is this not a concern already then? Also wondering why the buffer manager would care about the LSN on pages which are not BM_PERMANENT..? I'll admit that I might certainly be missing something here. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Mon, Oct 11, 2021 at 1:30 PM Stephen Frost wrote: > Regarding unlogged LSNs at least, I would think that we'd want to > actually use GetFakeLSNForUnloggedRel() instead of just having it zero'd > out. The fixed value for GiST index pages is just during the index > build process, as I recall, and that's perhaps less of a concern. Part > of the point of using XTS is to avoid the issue of the LSN not being > changed when hint bits are, or more generally not being unique in > various cases. I don't believe there's anything to prevent the fake-LSN counter from overtaking the real end-of-WAL, and if that should happen, then the buffer manager would get confused. Maybe that can be fixed by doing some sort of surgery on the buffer manager, but it doesn't seem to be a trivial or ignorable problem. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 11:05 PM Stephen Frost wrote: > Sure, I get that. Would be awesome if all these things were clearly > documented somewhere but I've never been able to find it quite as > explicitly laid out as one would like. :-( > specifically: Appendix C: Tweaks > > Quoting a couple of paragraphs from that appendix: > > """ > In general, if there is information that is available and statically > associated with a plaintext, it is recommended to use that information > as a tweak for the plaintext. Ideally, the non-secret tweak associated > with a plaintext is associated only with that plaintext. > > Extensive tweaking means that fewer plaintexts are encrypted under any > given tweak. This corresponds, in the security model that is described > in [1], to fewer queries to the target instance of the encryption. > """ > > The gist of this being- the more diverse the tweaking being used, the > better. That's where I was going with my "limit the risk" comment. If > we can make the tweak vary more for a given encryption invokation, > that's going to be better, pretty much by definition, and as explained > in publications by NIST. I mean I don't have anything against that appendix, but I think we need to understand - with confidence - what the expectations are specifically around XTS, and that appendix seems much more general than that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Tue, Oct 12, 2021 at 08:49:28AM -0400, Stephen Frost wrote: > * Bruce Momjian (br...@momjian.us) wrote: > > I thought he was saying that when you extend a file, you might have to > > extend it with all zeros, rather than being able to extend it with > > an actual encrypted page of zeros. For example, I think when a page is > > corrupt in storage, it reads back as a fully zero page, and we would > > need to handle that. Are you saying we already have logic to handle > > that so we don't need to change anything? > > When we extend a file, it gets extended with all zeros. PG already > handles that case, PG w/ TDE would need to also recognize that case > (which is what Ants was saying their patch does) and handle it. In > other words, we just need to realize when a page is all zeros and not > try to decrypt it when we're reading it. Ants' patch does that and my > recollection is that it wasn't very complicated to do, and that seems > much simpler than trying to figure out a way to ensure we do encrypt a > zero'd page as part of extending a file. Well, how do you detect an all-zero page vs a page that encrypted to all zeros? I am thinking a zero LSN (which is not encrypted) would be the only sure way, but we then have to make sure unlogged relations always get a fake LSN. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Oct 12, 2021 at 08:25:52AM -0400, Stephen Frost wrote: > > * Bruce Momjian (br...@momjian.us) wrote: > > > On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote: > > > > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian wrote: > > > > > > > > > Yes, that's the direction that I was thinking also and > > > > specifically with > > > > > XTS as the encryption algorithm to allow us to exclude the LSN > > > > but keep > > > > > everything else, and to address the concern around the > > > > nonce/tweak/etc > > > > > being the same sometimes across multiple writes. Another thing to > > > > > consider is if we want to encrypt zero'd page. There was a point > > > > > brought up that if we do then we are encrypting a fair bit of very > > > > > predictable bytes and that's not great (though there's a fair bit > > > > about > > > > > our pages that someone could quite possibly predict anyway based > > > > on > > > > > table structures and such...). I would think that if it's easy > > > > enough > > > > > to not encrypt zero'd pages that we should avoid doing so. Don't > > > > recall > > > > > offhand which way zero'd pages were being handled already but > > > > thought it > > > > > made sense to mention that as part of this discussion. > > > > > > > > Yeah, I wanted to mention that. I don't see any security difference > > > > between fully-zero pages, pages with headers and no tuples, and > > > > pages > > > > with headers and only a few tuples. If any of those are insecure, > > > > they > > > > all are. Therefore, I don't see any reason to treat them > > > > differently. > > > > > > > > > > > > We had to special case zero pages and not encrypt them because as far > > > > as I can > > > > tell, there is no atomic way to extend a file and initialize it to > > > > Enc(zero) in > > > > the same step. > > > > > > Oh, good point. Yeah, we will need to handle that. > > > > Not sure what's meant here by 'handle that', but I don't see any > > particular reason to avoid doing exactly the same for zero pages with > > TDE in core..? I don't think there's any reason we need to make things > > complicated to ensure that we encrypt entirely empty pages. > > I thought he was saying that when you extend a file, you might have to > extend it with all zeros, rather than being able to extend it with > an actual encrypted page of zeros. For example, I think when a page is > corrupt in storage, it reads back as a fully zero page, and we would > need to handle that. Are you saying we already have logic to handle > that so we don't need to change anything? When we extend a file, it gets extended with all zeros. PG already handles that case, PG w/ TDE would need to also recognize that case (which is what Ants was saying their patch does) and handle it. In other words, we just need to realize when a page is all zeros and not try to decrypt it when we're reading it. Ants' patch does that and my recollection is that it wasn't very complicated to do, and that seems much simpler than trying to figure out a way to ensure we do encrypt a zero'd page as part of extending a file. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Tue, Oct 12, 2021 at 08:25:52AM -0400, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote: > > > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian wrote: > > > > > > > Yes, that's the direction that I was thinking also and specifically > > > with > > > > XTS as the encryption algorithm to allow us to exclude the LSN but > > > keep > > > > everything else, and to address the concern around the > > > nonce/tweak/etc > > > > being the same sometimes across multiple writes. Another thing to > > > > consider is if we want to encrypt zero'd page. There was a point > > > > brought up that if we do then we are encrypting a fair bit of very > > > > predictable bytes and that's not great (though there's a fair bit > > > about > > > > our pages that someone could quite possibly predict anyway based on > > > > table structures and such...). I would think that if it's easy > > > enough > > > > to not encrypt zero'd pages that we should avoid doing so. Don't > > > recall > > > > offhand which way zero'd pages were being handled already but > > > thought it > > > > made sense to mention that as part of this discussion. > > > > > > Yeah, I wanted to mention that. I don't see any security difference > > > between fully-zero pages, pages with headers and no tuples, and pages > > > with headers and only a few tuples. If any of those are insecure, > > > they > > > all are. Therefore, I don't see any reason to treat them differently. > > > > > > > > > We had to special case zero pages and not encrypt them because as far as > > > I can > > > tell, there is no atomic way to extend a file and initialize it to > > > Enc(zero) in > > > the same step. > > > > Oh, good point. Yeah, we will need to handle that. > > Not sure what's meant here by 'handle that', but I don't see any > particular reason to avoid doing exactly the same for zero pages with > TDE in core..? I don't think there's any reason we need to make things > complicated to ensure that we encrypt entirely empty pages. I thought he was saying that when you extend a file, you might have to extend it with all zeros, rather than being able to extend it with an actual encrypted page of zeros. For example, I think when a page is corrupt in storage, it reads back as a fully zero page, and we would need to handle that. Are you saying we already have logic to handle that so we don't need to change anything? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote: > > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian wrote: > > > > > Yes, that's the direction that I was thinking also and specifically > > with > > > XTS as the encryption algorithm to allow us to exclude the LSN but > > keep > > > everything else, and to address the concern around the nonce/tweak/etc > > > being the same sometimes across multiple writes. Another thing to > > > consider is if we want to encrypt zero'd page. There was a point > > > brought up that if we do then we are encrypting a fair bit of very > > > predictable bytes and that's not great (though there's a fair bit > > about > > > our pages that someone could quite possibly predict anyway based on > > > table structures and such...). I would think that if it's easy enough > > > to not encrypt zero'd pages that we should avoid doing so. Don't > > recall > > > offhand which way zero'd pages were being handled already but thought > > it > > > made sense to mention that as part of this discussion. > > > > Yeah, I wanted to mention that. I don't see any security difference > > between fully-zero pages, pages with headers and no tuples, and pages > > with headers and only a few tuples. If any of those are insecure, they > > all are. Therefore, I don't see any reason to treat them differently. > > > > > > We had to special case zero pages and not encrypt them because as far as I > > can > > tell, there is no atomic way to extend a file and initialize it to > > Enc(zero) in > > the same step. > > Oh, good point. Yeah, we will need to handle that. Not sure what's meant here by 'handle that', but I don't see any particular reason to avoid doing exactly the same for zero pages with TDE in core..? I don't think there's any reason we need to make things complicated to ensure that we encrypt entirely empty pages. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote: > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian wrote: > > > Yes, that's the direction that I was thinking also and specifically with > > XTS as the encryption algorithm to allow us to exclude the LSN but keep > > everything else, and to address the concern around the nonce/tweak/etc > > being the same sometimes across multiple writes. Another thing to > > consider is if we want to encrypt zero'd page. There was a point > > brought up that if we do then we are encrypting a fair bit of very > > predictable bytes and that's not great (though there's a fair bit about > > our pages that someone could quite possibly predict anyway based on > > table structures and such...). I would think that if it's easy enough > > to not encrypt zero'd pages that we should avoid doing so. Don't recall > > offhand which way zero'd pages were being handled already but thought it > > made sense to mention that as part of this discussion. > > Yeah, I wanted to mention that. I don't see any security difference > between fully-zero pages, pages with headers and no tuples, and pages > with headers and only a few tuples. If any of those are insecure, they > all are. Therefore, I don't see any reason to treat them differently. > > > We had to special case zero pages and not encrypt them because as far as I can > tell, there is no atomic way to extend a file and initialize it to Enc(zero) > in > the same step. Oh, good point. Yeah, we will need to handle that. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Mon, 11 Oct 2021 at 22:15, Bruce Momjian wrote: > > Yes, that's the direction that I was thinking also and specifically with > > XTS as the encryption algorithm to allow us to exclude the LSN but keep > > everything else, and to address the concern around the nonce/tweak/etc > > being the same sometimes across multiple writes. Another thing to > > consider is if we want to encrypt zero'd page. There was a point > > brought up that if we do then we are encrypting a fair bit of very > > predictable bytes and that's not great (though there's a fair bit about > > our pages that someone could quite possibly predict anyway based on > > table structures and such...). I would think that if it's easy enough > > to not encrypt zero'd pages that we should avoid doing so. Don't recall > > offhand which way zero'd pages were being handled already but thought it > > made sense to mention that as part of this discussion. > > Yeah, I wanted to mention that. I don't see any security difference > between fully-zero pages, pages with headers and no tuples, and pages > with headers and only a few tuples. If any of those are insecure, they > all are. Therefore, I don't see any reason to treat them differently. > We had to special case zero pages and not encrypt them because as far as I can tell, there is no atomic way to extend a file and initialize it to Enc(zero) in the same step. -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com
Re: storing an explicit nonce
On Mon, Oct 11, 2021 at 01:30:38PM -0400, Stephen Frost wrote: > Greetings, > > > Keep in mind that in our existing code (not my patch), the LSN is zero > > for unlogged relations, a fixed value for some GiST index pages, and > > unchanged for some hint bit changes. Therefore, while we can include > > the LSN in the IV because it _might_ help, we can't rely on it. > > Regarding unlogged LSNs at least, I would think that we'd want to > actually use GetFakeLSNForUnloggedRel() instead of just having it zero'd > out. The fixed value for GiST index pages is just during the index Good idea. For my patch I had to use a WAL-logged dummy LSN, but for our use, re-using a fake LSN after a crash seems fine, so we can just use the existing GetFakeLSNForUnloggedRel(). However, we might need to use the part of my patch that removes the assumption that unlogged relations always have zero LSNs, because right now they are only used for GiST indexes --- I would have to research that more. > Yes, that's the direction that I was thinking also and specifically with > XTS as the encryption algorithm to allow us to exclude the LSN but keep > everything else, and to address the concern around the nonce/tweak/etc > being the same sometimes across multiple writes. Another thing to > consider is if we want to encrypt zero'd page. There was a point > brought up that if we do then we are encrypting a fair bit of very > predictable bytes and that's not great (though there's a fair bit about > our pages that someone could quite possibly predict anyway based on > table structures and such...). I would think that if it's easy enough > to not encrypt zero'd pages that we should avoid doing so. Don't recall > offhand which way zero'd pages were being handled already but thought it > made sense to mention that as part of this discussion. Yeah, I wanted to mention that. I don't see any security difference between fully-zero pages, pages with headers and no tuples, and pages with headers and only a few tuples. If any of those are insecure, they all are. Therefore, I don't see any reason to treat them differently. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Fri, Oct 8, 2021 at 02:34:20PM -0400, Stephen Frost wrote: > > What I think is missing from this discussion is the fact that, with XTS > > (and XEX, on which XTS is built), the IV *is* run through a forward > > cipher function, just as suggested above needs to be done for CBC. I > > don't see any reason to doubt that OpenSSL is correctly doing that. > > > > This article shows this pretty clearly: > > > > https://en.wikipedia.org/wiki/Disk_encryption_theory > > > > I don't think that changes the fact that, if we're able to, we should be > > varying the tweak/IV as often as we can, and including the LSN seems > > like a good way to do just that. > > Keep in mind that in our existiing code (not my patch), the LSN is zero > for unlogged relations, a fixed value for some GiST index pages, and > unchanged for some hint bit changes. Therefore, while we can include > the LSN in the IV because it _might_ help, we can't rely on it. Regarding unlogged LSNs at least, I would think that we'd want to actually use GetFakeLSNForUnloggedRel() instead of just having it zero'd out. The fixed value for GiST index pages is just during the index build process, as I recall, and that's perhaps less of a concern. Part of the point of using XTS is to avoid the issue of the LSN not being changed when hint bits are, or more generally not being unique in various cases. > We probably need to have a discussion about whether LSN and checksum > should be encrypted on the page. I think we are currently leaning to no > encryption for LSN because we can use it as part of the nonce (where is > it is variable) and encrypting the checksum for rudimenary integrity > checking. Yes, that's the direction that I was thinking also and specifically with XTS as the encryption algorithm to allow us to exclude the LSN but keep everything else, and to address the concern around the nonce/tweak/etc being the same sometimes across multiple writes. Another thing to consider is if we want to encrypt zero'd page. There was a point brought up that if we do then we are encrypting a fair bit of very predictable bytes and that's not great (though there's a fair bit about our pages that someone could quite possibly predict anyway based on table structures and such...). I would think that if it's easy enough to not encrypt zero'd pages that we should avoid doing so. Don't recall offhand which way zero'd pages were being handled already but thought it made sense to mention that as part of this discussion. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Mon, Oct 11, 2021 at 01:01:08PM -0400, Stephen Frost wrote: > > It is more than just the page format --- it would also be the added > > code, possible performance impact, and later code maintenance to allow > > for are a more complex or two different page formats. > > Yes, there is more to it than just the page format, I agree. I'm still > of the mind that it's something we're going to get to eventually, if for > no other reason than that our current page format is certainly not > perfect and it'd be pretty awesome if we could make improvements to it > (independently of TDE or anything else discussed currently). Yes, 100% agree on that. The good part is that TDE would not be paying the cost for that. ;-) -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Fri, Oct 8, 2021 at 02:34:20PM -0400, Stephen Frost wrote: > What I think is missing from this discussion is the fact that, with XTS > (and XEX, on which XTS is built), the IV *is* run through a forward > cipher function, just as suggested above needs to be done for CBC. I > don't see any reason to doubt that OpenSSL is correctly doing that. > > This article shows this pretty clearly: > > https://en.wikipedia.org/wiki/Disk_encryption_theory > > I don't think that changes the fact that, if we're able to, we should be > varying the tweak/IV as often as we can, and including the LSN seems > like a good way to do just that. Keep in mind that in our existiing code (not my patch), the LSN is zero for unlogged relations, a fixed value for some GiST index pages, and unchanged for some hint bit changes. Therefore, while we can include the LSN in the IV because it _might_ help, we can't rely on it. We probably need to have a discussion about whether LSN and checksum should be encrypted on the page. I think we are currently leaning to no encryption for LSN because we can use it as part of the nonce (where is it is variable) and encrypting the checksum for rudimenary integrity checking. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Thu, Oct 7, 2021 at 11:32:07PM -0400, Stephen Frost wrote: > > Part of the meeting was specifically about "why are we doing this?" and > > there were a few different answers- first and foremost was "because > > people are asking for it", from which followed that, yes, in many cases > > it's to satisfy an audit or similar requirement which any of the > > proposed methods would address. There was further discussion that we > > Yes, Cybertec's experience with their TDE patch's adoption supported > this. > > > could address *more* cases by providing something better, but the page > > format changes were weighed against that and the general consensus was > > that we should attack the simpler problem first and, potentially, gain > > a solution for 90% of the folks asking for it, and then later see if > > there's enough interest and desire to attack the remaining 10%. > > It is more than just the page format --- it would also be the added > code, possible performance impact, and later code maintenance to allow > for are a more complex or two different page formats. Yes, there is more to it than just the page format, I agree. I'm still of the mind that it's something we're going to get to eventually, if for no other reason than that our current page format is certainly not perfect and it'd be pretty awesome if we could make improvements to it (independently of TDE or anything else discussed currently). > As an example, I think the online checksum patch failed because it > wasn't happy with that 90% and went for the extra 10% of restartability, > but once you saw the 100% solution, the patch was too big and was > rejected. I'm, at least, still hopeful that we get the online checksum patch done. I'm not sure that I agree that this was 'the' reason it didn't make it in, but I don't think it'd be helpful to tangent this thread to discussing some other patch. > > As such, it's just not so simple as "what is 'secure enough'" because it > > depends on who you're talking to. Based on the collective discussion at > > the meeting, XTS is 'secure enough' for the needs of probably 90% of > > those asking, while the other 10% want better (an AEAD method such as > > GCM or GCM-SIV). Therefore, what should we do? Spend all of the extra > > resources and engineering effort to address the 10% and maybe not get > > anything because of the level of difficulty, or go the simpler route > > first and get the 90%? Through that lense, the choice seemed reasonably > > clear, at least to me, hence why I agreed that we should work on an XTS > > based approach first. > > Yes, that was the conclusion. I think it helped to have the discussion > verbally with everyone hearing every word, rather than via email where > people jump into the discussion not hearing earlier points. Yes, agreed. Certainly am hopeful that we are able to have more of those in the (relatively) near future too! Thanks! Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 11:32:07PM -0400, Stephen Frost wrote: > Part of the meeting was specifically about "why are we doing this?" and > there were a few different answers- first and foremost was "because > people are asking for it", from which followed that, yes, in many cases > it's to satisfy an audit or similar requirement which any of the > proposed methods would address. There was further discussion that we Yes, Cybertec's experience with their TDE patch's adoption supported this. > could address *more* cases by providing something better, but the page > format changes were weighed against that and the general consensus was > that we should attack the simpler problem first and, potentially, gain > a solution for 90% of the folks asking for it, and then later see if > there's enough interest and desire to attack the remaining 10%. It is more than just the page format --- it would also be the added code, possible performance impact, and later code maintenance to allow for are a more complex or two different page formats. As an example, I think the online checksum patch failed because it wasn't happy with that 90% and went for the extra 10% of restartability, but once you saw the 100% solution, the patch was too big and was rejected. > As such, it's just not so simple as "what is 'secure enough'" because it > depends on who you're talking to. Based on the collective discussion at > the meeting, XTS is 'secure enough' for the needs of probably 90% of > those asking, while the other 10% want better (an AEAD method such as > GCM or GCM-SIV). Therefore, what should we do? Spend all of the extra > resources and engineering effort to address the 10% and maybe not get > anything because of the level of difficulty, or go the simpler route > first and get the 90%? Through that lense, the choice seemed reasonably > clear, at least to me, hence why I agreed that we should work on an XTS > based approach first. Yes, that was the conclusion. I think it helped to have the discussion verbally with everyone hearing every word, rather than via email where people jump into the discussion not hearing earlier points. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On 2021/10/6 23:01, Robert Haas wrote: > This seems wrong to me. CTR requires that you not reuse the IV. If you > re-encrypt the page with a different IV, torn pages are a problem. If > you re-encrypt it with the same IV, then it's not secure any more. for CBC if the IV is predictable will case "dictionary attack". and for CBC and GCM reuse IV will case "known plaintext attack". XTS works like CBC but adds a tweak step. the tweak step does not add randomness. It means XTS still has "known plaintext attack", due to the same reason from CBC. many mails before this mail do a clear explanation, I just repeat. :> On 2021/10/7 22:28, Robert Haas wrote: > I'm a little concerned by the email from "Sasasu" saying that even in > XTS reusing the IV is not cryptographically weak. I don't know enough > about these different encryption modes to know if he's right, but if > he is then perhaps we need to consider his suggestion of using > AES-GCM. Or, uh, something else. a cryptography algorithm always lists some attack method (the scope), if the algorithm can defend against this attack, the algorithm is good. If software uses this algorithm but is attacked by the method not on that list. It is the software using this algorithm incorrectly, or should not use this algorithm. On 2021/10/8 03:38, Stephen Frost wrote: > I strongly suspect we'll have one of two > reactions- either we'll be more-or-less ignored and it'll be crickets > from the security folks, or we're going to get beat up by them for > $reasons, almost regardless of what we actually do. Best bet to > limit the risk (;) ) of the latter happening would be to try our best > to do what existing solutions already do- such as by using XTS. If using an existing wonderful algorithm but out of the design scope, cryptographers will laugh at you. On 2021/10/9 02:34, Stephen Frost wrote: Greetings, * Antonin Houska (a...@cybertec.at) wrote: Stephen Frost wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Thu, Oct 7, 2021 at 3:38 PM Stephen Frost wrote: While I certainly also appreciate that we want to get this as right as we possibly can from the start, I strongly suspect we'll have one of two reactions- either we'll be more-or-less ignored and it'll be crickets from the security folks, or we're going to get beat up by them for $reasons, almost regardless of what we actually do. Best bet to limit the risk ( ;) ) of the latter happening would be to try our best to do what existing solutions already do- such as by using XTS. There's things we can do to limit the risk of known-plaintext attacks, like simply not encrypting empty pages, or about possible known-IV risks, like using the LSN as part of the IV/tweak. Will we get everything? Probably not, but I don't think that we're going to really go wrong by using XTS as it's quite popularly used today and it's explicitly used for cases where you haven't got a place to store the extra nonce that you would need for AEAD encryption schemes. I agree that using a popular approach is a good way to go. If we do what other people do, then hopefully our stuff won't be significantly more broken than their stuff, and whatever is can be fixed. Right. As long as we're clear that this initial version of TDE is with XTS then I really don't think we'll end up with anyone showing up and saying we screwed up by not generating a per-page nonce to store with it- the point of XTS is that you don't need that. I agree that we shouldn't really catch flack for any weaknesses of the underlying algorithm: if XTS turns out to be secure even when used properly, and we use it properly, the resulting weakness is somebody else's fault. On the other hand, if we use it improperly, that's our fault, so we need to be really sure that we understand what guarantees we need to provide from our end, and that we are providing them. Like if we pick an encryption mode that requires nonces to be unique, we will be at fault if they aren't; if it requires nonces to be unpredictable, we will be at fault if they aren't; and so on. Sure, I get that. Would be awesome if all these things were clearly documented somewhere but I've never been able to find it quite as explicitly laid out as one would like. So that's what is making me nervous here ... it doesn't seem likely we have complete unanimity about whether XTS is the right thing, though that does seem to be the majority position certainly, and it is not really clear to me that any of us can speak with authority about what the requirements are around the nonces in particular. The authority to look at, in my view anyway, are NIST publications. Following a bit more digging, I came across something which makes sense to me as intuitive but explains it in a way that might help everyone understand a bit better what's going on here: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38G.pdf specifically: Appendix C: Tweaks Quoting a couple of paragraphs from that
Re: storing an explicit nonce
Greetings, * Antonin Houska (a...@cybertec.at) wrote: > Stephen Frost wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > > > On Thu, Oct 7, 2021 at 3:38 PM Stephen Frost wrote: > > > > While I certainly also appreciate that we want to get this as right as > > > > we possibly can from the start, I strongly suspect we'll have one of two > > > > reactions- either we'll be more-or-less ignored and it'll be crickets > > > > from the security folks, or we're going to get beat up by them for > > > > $reasons, almost regardless of what we actually do. Best bet to > > > > limit the risk ( ;) ) of the latter happening would be to try our best > > > > to do what existing solutions already do- such as by using XTS. > > > > There's things we can do to limit the risk of known-plaintext attacks, > > > > like simply not encrypting empty pages, or about possible known-IV > > > > risks, like using the LSN as part of the IV/tweak. Will we get > > > > everything? Probably not, but I don't think that we're going to really > > > > go wrong by using XTS as it's quite popularly used today and it's > > > > explicitly used for cases where you haven't got a place to store the > > > > extra nonce that you would need for AEAD encryption schemes. > > > > > > I agree that using a popular approach is a good way to go. If we do > > > what other people do, then hopefully our stuff won't be significantly > > > more broken than their stuff, and whatever is can be fixed. > > > > Right. > > > > > > As long as we're clear that this initial version of TDE is with XTS then > > > > I really don't think we'll end up with anyone showing up and saying we > > > > screwed up by not generating a per-page nonce to store with it- the > > > > point > > > > of XTS is that you don't need that. > > > > > > I agree that we shouldn't really catch flack for any weaknesses of the > > > underlying algorithm: if XTS turns out to be secure even when used > > > properly, and we use it properly, the resulting weakness is somebody > > > else's fault. On the other hand, if we use it improperly, that's our > > > fault, so we need to be really sure that we understand what guarantees > > > we need to provide from our end, and that we are providing them. Like > > > if we pick an encryption mode that requires nonces to be unique, we > > > will be at fault if they aren't; if it requires nonces to be > > > unpredictable, we will be at fault if they aren't; and so on. > > > > Sure, I get that. Would be awesome if all these things were clearly > > documented somewhere but I've never been able to find it quite as > > explicitly laid out as one would like. > > > > > So that's what is making me nervous here ... it doesn't seem likely we > > > have complete unanimity about whether XTS is the right thing, though > > > that does seem to be the majority position certainly, and it is not > > > really clear to me that any of us can speak with authority about what > > > the requirements are around the nonces in particular. > > > > The authority to look at, in my view anyway, are NIST publications. > > Following a bit more digging, I came across something which makes sense > > to me as intuitive but explains it in a way that might help everyone > > understand a bit better what's going on here: > > > > https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38G.pdf > > > > specifically: Appendix C: Tweaks > > > > Quoting a couple of paragraphs from that appendix: > > > > """ > > In general, if there is information that is available and statically > > associated with a plaintext, it is recommended to use that information > > as a tweak for the plaintext. Ideally, the non-secret tweak associated > > with a plaintext is associated only with that plaintext. > > > > Extensive tweaking means that fewer plaintexts are encrypted under any > > given tweak. This corresponds, in the security model that is described > > in [1], to fewer queries to the target instance of the encryption. > > """ > > > > The gist of this being- the more diverse the tweaking being used, the > > better. That's where I was going with my "limit the risk" comment. If > > we can make the tweak vary more for a given encryption invokation, > > that's going to be better, pretty much by definition, and as explained > > in publications by NIST. > > > > That isn't to say that using the same tweak for the same block over and > > over "breaks" the encryption (unlike with CTR/GCM, where IV reuse leads > > directly to plaintext being recoverable), but it does mean that an > > observer who can see the block writes over time could see what parts are > > changing (and which aren't) and may be able to derive insight from that. > > This reminds me of Joe Conway's response to me email earlier: > > https://www.postgresql.org/message-id/50335f56-041b-1a1f-59ea-b5f7bf917352%40joeconway.com > > In the document he recommended > >
Re: storing an explicit nonce
Stephen Frost wrote: > Greetings, > > * Robert Haas (robertmh...@gmail.com) wrote: > > On Thu, Oct 7, 2021 at 3:38 PM Stephen Frost wrote: > > > While I certainly also appreciate that we want to get this as right as > > > we possibly can from the start, I strongly suspect we'll have one of two > > > reactions- either we'll be more-or-less ignored and it'll be crickets > > > from the security folks, or we're going to get beat up by them for > > > $reasons, almost regardless of what we actually do. Best bet to > > > limit the risk ( ;) ) of the latter happening would be to try our best > > > to do what existing solutions already do- such as by using XTS. > > > There's things we can do to limit the risk of known-plaintext attacks, > > > like simply not encrypting empty pages, or about possible known-IV > > > risks, like using the LSN as part of the IV/tweak. Will we get > > > everything? Probably not, but I don't think that we're going to really > > > go wrong by using XTS as it's quite popularly used today and it's > > > explicitly used for cases where you haven't got a place to store the > > > extra nonce that you would need for AEAD encryption schemes. > > > > I agree that using a popular approach is a good way to go. If we do > > what other people do, then hopefully our stuff won't be significantly > > more broken than their stuff, and whatever is can be fixed. > > Right. > > > > As long as we're clear that this initial version of TDE is with XTS then > > > I really don't think we'll end up with anyone showing up and saying we > > > screwed up by not generating a per-page nonce to store with it- the point > > > of XTS is that you don't need that. > > > > I agree that we shouldn't really catch flack for any weaknesses of the > > underlying algorithm: if XTS turns out to be secure even when used > > properly, and we use it properly, the resulting weakness is somebody > > else's fault. On the other hand, if we use it improperly, that's our > > fault, so we need to be really sure that we understand what guarantees > > we need to provide from our end, and that we are providing them. Like > > if we pick an encryption mode that requires nonces to be unique, we > > will be at fault if they aren't; if it requires nonces to be > > unpredictable, we will be at fault if they aren't; and so on. > > Sure, I get that. Would be awesome if all these things were clearly > documented somewhere but I've never been able to find it quite as > explicitly laid out as one would like. > > > So that's what is making me nervous here ... it doesn't seem likely we > > have complete unanimity about whether XTS is the right thing, though > > that does seem to be the majority position certainly, and it is not > > really clear to me that any of us can speak with authority about what > > the requirements are around the nonces in particular. > > The authority to look at, in my view anyway, are NIST publications. > Following a bit more digging, I came across something which makes sense > to me as intuitive but explains it in a way that might help everyone > understand a bit better what's going on here: > > https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38G.pdf > > specifically: Appendix C: Tweaks > > Quoting a couple of paragraphs from that appendix: > > """ > In general, if there is information that is available and statically > associated with a plaintext, it is recommended to use that information > as a tweak for the plaintext. Ideally, the non-secret tweak associated > with a plaintext is associated only with that plaintext. > > Extensive tweaking means that fewer plaintexts are encrypted under any > given tweak. This corresponds, in the security model that is described > in [1], to fewer queries to the target instance of the encryption. > """ > > The gist of this being- the more diverse the tweaking being used, the > better. That's where I was going with my "limit the risk" comment. If > we can make the tweak vary more for a given encryption invokation, > that's going to be better, pretty much by definition, and as explained > in publications by NIST. > > That isn't to say that using the same tweak for the same block over and > over "breaks" the encryption (unlike with CTR/GCM, where IV reuse leads > directly to plaintext being recoverable), but it does mean that an > observer who can see the block writes over time could see what parts are > changing (and which aren't) and may be able to derive insight from that. This reminds me of Joe Conway's response to me email earlier: https://www.postgresql.org/message-id/50335f56-041b-1a1f-59ea-b5f7bf917352%40joeconway.com In the document he recommended https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a.pdf specifically, in the Appendix C I read: """ For the CBC and CFB modes, the IVs must be unpredictable. In particular, for any given plaintext, it must not be possible to predict the IV that will be associated to the
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 12:57 PM Stephen Frost wrote: > > Yes, for integrity verification (also known as 'authenticated > > encryption') we'd definitely need to store a larger nonce value. In the > > very, very long term, I think it'd be great to have that, and the patch > > proposed on this thread seems really cool as a way to get us there. > > OK. I'm not sure why that has to be relegated to the very, very long > term, but I'm really very happy to hear that you think the approach is > cool. Folks are shy about a page format change and I get that. > > Having TDE, even without authenticated encryption, is certainly > > valuable. Reducing the amount of engineering required to get there is > > worthwhile. Implementing TDE w/ XTS or similar, provided we do agree > > that we can do so with an IV that we don't need to find additional space > > for, would avoid that page-level format change. I agree we should do > > some research to make sure we at least have a reasonable answer to that > > question. I've spent a bit of time on that and haven't gotten to a sure > > answer one way or the other as yet, but will continue to look. > > I mean, I feel like this meeting that Bruce was talking about was > perhaps making decisions in the wrong order. We have to decide which > encryption mode is secure enough for our needs FIRST, and then AFTER > that we can decide whether we need to store a nonce in the page. Now > if it turns out that we can do either with or without a nonce in the > page, then I'm just as happy as anyone else to start with the method > that works without a nonce in the page, because like you say, that's > less work. But unless we've established that such a method is actually > going to survive scrutiny by smart cryptographers, we can't really > decide that storing the nonce is off the table. And it doesn't seem > like we've established that yet. Part of the meeting was specifically about "why are we doing this?" and there were a few different answers- first and foremost was "because people are asking for it", from which followed that, yes, in many cases it's to satisfy an audit or similar requirement which any of the proposed methods would address. There was further discussion that we could address *more* cases by providing something better, but the page format changes were weighed against that and the general consensus was that we should attack the simpler problem first and, potentially, gain a solution for 90% of the folks asking for it, and then later see if there's enough interest and desire to attack the remaining 10%. As such, it's just not so simple as "what is 'secure enough'" because it depends on who you're talking to. Based on the collective discussion at the meeting, XTS is 'secure enough' for the needs of probably 90% of those asking, while the other 10% want better (an AEAD method such as GCM or GCM-SIV). Therefore, what should we do? Spend all of the extra resources and engineering effort to address the 10% and maybe not get anything because of the level of difficulty, or go the simpler route first and get the 90%? Through that lense, the choice seemed reasonably clear, at least to me, hence why I agreed that we should work on an XTS based approach first. (Admittedly, the overall discussion wasn't quite as specific as XTS vs. GCM-SIV, but the gist was "page format change" vs. "no page format change" and that seems to equate, based on this subsequent discussion to the choice between XTS and GCM/GCM-SIV.) Thanks! Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 3:38 PM Stephen Frost wrote: > > While I certainly also appreciate that we want to get this as right as > > we possibly can from the start, I strongly suspect we'll have one of two > > reactions- either we'll be more-or-less ignored and it'll be crickets > > from the security folks, or we're going to get beat up by them for > > $reasons, almost regardless of what we actually do. Best bet to > > limit the risk ( ;) ) of the latter happening would be to try our best > > to do what existing solutions already do- such as by using XTS. > > There's things we can do to limit the risk of known-plaintext attacks, > > like simply not encrypting empty pages, or about possible known-IV > > risks, like using the LSN as part of the IV/tweak. Will we get > > everything? Probably not, but I don't think that we're going to really > > go wrong by using XTS as it's quite popularly used today and it's > > explicitly used for cases where you haven't got a place to store the > > extra nonce that you would need for AEAD encryption schemes. > > I agree that using a popular approach is a good way to go. If we do > what other people do, then hopefully our stuff won't be significantly > more broken than their stuff, and whatever is can be fixed. Right. > > As long as we're clear that this initial version of TDE is with XTS then > > I really don't think we'll end up with anyone showing up and saying we > > screwed up by not generating a per-page nonce to store with it- the point > > of XTS is that you don't need that. > > I agree that we shouldn't really catch flack for any weaknesses of the > underlying algorithm: if XTS turns out to be secure even when used > properly, and we use it properly, the resulting weakness is somebody > else's fault. On the other hand, if we use it improperly, that's our > fault, so we need to be really sure that we understand what guarantees > we need to provide from our end, and that we are providing them. Like > if we pick an encryption mode that requires nonces to be unique, we > will be at fault if they aren't; if it requires nonces to be > unpredictable, we will be at fault if they aren't; and so on. Sure, I get that. Would be awesome if all these things were clearly documented somewhere but I've never been able to find it quite as explicitly laid out as one would like. > So that's what is making me nervous here ... it doesn't seem likely we > have complete unanimity about whether XTS is the right thing, though > that does seem to be the majority position certainly, and it is not > really clear to me that any of us can speak with authority about what > the requirements are around the nonces in particular. The authority to look at, in my view anyway, are NIST publications. Following a bit more digging, I came across something which makes sense to me as intuitive but explains it in a way that might help everyone understand a bit better what's going on here: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38G.pdf specifically: Appendix C: Tweaks Quoting a couple of paragraphs from that appendix: """ In general, if there is information that is available and statically associated with a plaintext, it is recommended to use that information as a tweak for the plaintext. Ideally, the non-secret tweak associated with a plaintext is associated only with that plaintext. Extensive tweaking means that fewer plaintexts are encrypted under any given tweak. This corresponds, in the security model that is described in [1], to fewer queries to the target instance of the encryption. """ The gist of this being- the more diverse the tweaking being used, the better. That's where I was going with my "limit the risk" comment. If we can make the tweak vary more for a given encryption invokation, that's going to be better, pretty much by definition, and as explained in publications by NIST. That isn't to say that using the same tweak for the same block over and over "breaks" the encryption (unlike with CTR/GCM, where IV reuse leads directly to plaintext being recoverable), but it does mean that an observer who can see the block writes over time could see what parts are changing (and which aren't) and may be able to derive insight from that. Now, as I mentioned before, that particular case isn't something that XTS is particularly good at and that's generally accepted, yet lots of folks use XTS anyway because the concern isn't "someone has root access on the box and is watching all block writes" but rather "laptop was stolen" where the attacker doesn't get to see multiple writes where the same key+tweak has been used, and the latter is really the attack vector we're looking to address with XTS too. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 3:31 PM Ashwin Agrawal wrote: > > Not at all knowledgeable on security topics (bravely using terms and > > recommendation), can we approach decisions like AES-XTS vs AES-GCM (which > > in turn decides whether we need to store nonce or not) based on which > > compliance it can achieve or not. Like can using AES-XTS make it FIPS 140-2 > > compliant or not? > > To the best of my knowledge, the encryption mode doesn't have much to > do with whether such compliance can be achieved. The encryption > algorithm could matter, but I assume everyone still thinks AES is > acceptable. (We should assume that will eventually change.) The > encryption mode is, at least as I understand, more of an internal > thing that you have to get right to avoid having people break your > encryption and write papers about how they did it. The issue regarding FIPS 140-2 specifically is actually about the encryption used (AES-XTS is approved) *and* about the actual library which is doing the encryption, which isn't really anything to do with us but rather is OpenSSL (or perhaps NSS if we can get that finished and included), or maybe some third party that implements one of those APIs that you decide to use (of which there's a few, some of which have FIPS 140-2 certification). So, can you have a FIPS 140-2 compliant system with AES-XTS? Yes, as it's approved: https://csrc.nist.gov/csrc/media/projects/cryptographic-module-validation-program/documents/fips140-2/fips1402ig.pdf Will your system be FIPS 140-2 certified? That's a big "it depends" and will involve you actually taking your fully built system through a testing lab to get it certified. I certainly don't think we can make any promises that taking it through such a test would be successful the first time around, or even ever. First step though would be to get something implemented so that $someone can try and can provide feedback. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 3:38 PM Stephen Frost wrote: > While I certainly also appreciate that we want to get this as right as > we possibly can from the start, I strongly suspect we'll have one of two > reactions- either we'll be more-or-less ignored and it'll be crickets > from the security folks, or we're going to get beat up by them for > $reasons, almost regardless of what we actually do. Best bet to > limit the risk ( ;) ) of the latter happening would be to try our best > to do what existing solutions already do- such as by using XTS. > There's things we can do to limit the risk of known-plaintext attacks, > like simply not encrypting empty pages, or about possible known-IV > risks, like using the LSN as part of the IV/tweak. Will we get > everything? Probably not, but I don't think that we're going to really > go wrong by using XTS as it's quite popularly used today and it's > explicitly used for cases where you haven't got a place to store the > extra nonce that you would need for AEAD encryption schemes. I agree that using a popular approach is a good way to go. If we do what other people do, then hopefully our stuff won't be significantly more broken than their stuff, and whatever is can be fixed. > As long as we're clear that this initial version of TDE is with XTS then > I really don't think we'll end up with anyone showing up and saying we > screwed up by not generating a per-page nonce to store with it- the point > of XTS is that you don't need that. I agree that we shouldn't really catch flack for any weaknesses of the underlying algorithm: if XTS turns out to be secure even when used properly, and we use it properly, the resulting weakness is somebody else's fault. On the other hand, if we use it improperly, that's our fault, so we need to be really sure that we understand what guarantees we need to provide from our end, and that we are providing them. Like if we pick an encryption mode that requires nonces to be unique, we will be at fault if they aren't; if it requires nonces to be unpredictable, we will be at fault if they aren't; and so on. So that's what is making me nervous here ... it doesn't seem likely we have complete unanimity about whether XTS is the right thing, though that does seem to be the majority position certainly, and it is not really clear to me that any of us can speak with authority about what the requirements are around the nonces in particular. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 03:38:58PM -0400, Stephen Frost wrote: > > Now none of that is to say that we shouldn't limit risk - I mean less > > risk is always better than more. But we need to be sure this is not > > like a 90% thing, where we're pretty sure it works. We can get by with > > that for a lot of things, but I think here we had better try > > extra-hard to make sure that we don't have any exposures. We probably > > will anyway, but at least if they're just bugs and not architectural > > deficiencies, we can hope to be able to patch them as they are > > discovered. > > As long as we're clear that this initial version of TDE is with XTS then > I really don't think we'll end up with anyone showing up and saying we > screwed up by not generating a per-page nonce to store with it- the point > of XTS is that you don't need that. I am sold. ;-) -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 2:52 PM Stephen Frost wrote: > > Assuming that's correct, and I don't see any reason to doubt it, then > > perhaps it would make sense to have the LSN be unencrypted and include > > it in the tweak as that would limit the risk from re-use of the same > > tweak over time. > > Talking about things like "limiting the risk" makes me super-nervous. All of this is about limiting risks. :) > Maybe we're all on the same page here, but just to make my assumptions > explicit: I think we have to approach this feature with the idea in > mind that there are going to be very smart people actively attacking > any TDE implementation we ship. I expect that if you are lucky enough > to get your hands on a PostgreSQL cluster's data files and they happen > to be encrypted, your best option for handling that situation is not > going to be attacking the encryption, but rather something like > calling the person who has the password and pretending to be someone > to whom they ought to disclose it. However, I also believe that > PostgreSQL is a sufficiently high-profile project that security > researchers will find it a tempting target. And if they manage to > write a shell script or tool that breaks our encryption without too > much difficulty, it will generate a ton of negative PR for the > project. This will be especially true if the problem can't be fixed > without re-engineering the whole thing, because we're not > realistically going to be able to re-engineer the whole thing in a > minor release, and thus will be saddled with the defective > implementation for many years. While I certainly also appreciate that we want to get this as right as we possibly can from the start, I strongly suspect we'll have one of two reactions- either we'll be more-or-less ignored and it'll be crickets from the security folks, or we're going to get beat up by them for $reasons, almost regardless of what we actually do. Best bet to limit the risk ( ;) ) of the latter happening would be to try our best to do what existing solutions already do- such as by using XTS. There's things we can do to limit the risk of known-plaintext attacks, like simply not encrypting empty pages, or about possible known-IV risks, like using the LSN as part of the IV/tweak. Will we get everything? Probably not, but I don't think that we're going to really go wrong by using XTS as it's quite popularly used today and it's explicitly used for cases where you haven't got a place to store the extra nonce that you would need for AEAD encryption schemes. > Now none of that is to say that we shouldn't limit risk - I mean less > risk is always better than more. But we need to be sure this is not > like a 90% thing, where we're pretty sure it works. We can get by with > that for a lot of things, but I think here we had better try > extra-hard to make sure that we don't have any exposures. We probably > will anyway, but at least if they're just bugs and not architectural > deficiencies, we can hope to be able to patch them as they are > discovered. As long as we're clear that this initial version of TDE is with XTS then I really don't think we'll end up with anyone showing up and saying we screwed up by not generating a per-page nonce to store with it- the point of XTS is that you don't need that. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 3:31 PM Ashwin Agrawal wrote: > Not at all knowledgeable on security topics (bravely using terms and > recommendation), can we approach decisions like AES-XTS vs AES-GCM (which in > turn decides whether we need to store nonce or not) based on which compliance > it can achieve or not. Like can using AES-XTS make it FIPS 140-2 compliant or > not? To the best of my knowledge, the encryption mode doesn't have much to do with whether such compliance can be achieved. The encryption algorithm could matter, but I assume everyone still thinks AES is acceptable. (We should assume that will eventually change.) The encryption mode is, at least as I understand, more of an internal thing that you have to get right to avoid having people break your encryption and write papers about how they did it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:12 PM Robert Haas wrote: > On Thu, Oct 7, 2021 at 2:52 PM Stephen Frost wrote: > > Assuming that's correct, and I don't see any reason to doubt it, then > > perhaps it would make sense to have the LSN be unencrypted and include > > it in the tweak as that would limit the risk from re-use of the same > > tweak over time. > > Talking about things like "limiting the risk" makes me super-nervous. > > Maybe we're all on the same page here, but just to make my assumptions > explicit: I think we have to approach this feature with the idea in > mind that there are going to be very smart people actively attacking > any TDE implementation we ship. I expect that if you are lucky enough > to get your hands on a PostgreSQL cluster's data files and they happen > to be encrypted, your best option for handling that situation is not > going to be attacking the encryption, but rather something like > calling the person who has the password and pretending to be someone > to whom they ought to disclose it. However, I also believe that > PostgreSQL is a sufficiently high-profile project that security > researchers will find it a tempting target. And if they manage to > write a shell script or tool that breaks our encryption without too > much difficulty, it will generate a ton of negative PR for the > project. This will be especially true if the problem can't be fixed > without re-engineering the whole thing, because we're not > realistically going to be able to re-engineer the whole thing in a > minor release, and thus will be saddled with the defective > implementation for many years. > > Now none of that is to say that we shouldn't limit risk - I mean less > risk is always better than more. But we need to be sure this is not > like a 90% thing, where we're pretty sure it works. We can get by with > that for a lot of things, but I think here we had better try > extra-hard to make sure that we don't have any exposures. We probably > will anyway, but at least if they're just bugs and not architectural > deficiencies, we can hope to be able to patch them as they are > discovered. > Not at all knowledgeable on security topics (bravely using terms and recommendation), can we approach decisions like AES-XTS vs AES-GCM (which in turn decides whether we need to store nonce or not) based on which compliance it can achieve or not. Like can using AES-XTS make it FIPS 140-2 compliant or not?
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 2:52 PM Stephen Frost wrote: > Assuming that's correct, and I don't see any reason to doubt it, then > perhaps it would make sense to have the LSN be unencrypted and include > it in the tweak as that would limit the risk from re-use of the same > tweak over time. Talking about things like "limiting the risk" makes me super-nervous. Maybe we're all on the same page here, but just to make my assumptions explicit: I think we have to approach this feature with the idea in mind that there are going to be very smart people actively attacking any TDE implementation we ship. I expect that if you are lucky enough to get your hands on a PostgreSQL cluster's data files and they happen to be encrypted, your best option for handling that situation is not going to be attacking the encryption, but rather something like calling the person who has the password and pretending to be someone to whom they ought to disclose it. However, I also believe that PostgreSQL is a sufficiently high-profile project that security researchers will find it a tempting target. And if they manage to write a shell script or tool that breaks our encryption without too much difficulty, it will generate a ton of negative PR for the project. This will be especially true if the problem can't be fixed without re-engineering the whole thing, because we're not realistically going to be able to re-engineer the whole thing in a minor release, and thus will be saddled with the defective implementation for many years. Now none of that is to say that we shouldn't limit risk - I mean less risk is always better than more. But we need to be sure this is not like a 90% thing, where we're pretty sure it works. We can get by with that for a lot of things, but I think here we had better try extra-hard to make sure that we don't have any exposures. We probably will anyway, but at least if they're just bugs and not architectural deficiencies, we can hope to be able to patch them as they are discovered. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 09:59:31PM +0300, Ants Aasma wrote: > On Thu, 7 Oct 2021 at 21:52, Stephen Frost wrote: > > With XTS this isn't actually the case though, is it..? Part of the > point of XTS is that the last block doesn't have to be a full 16 bytes. > What you're saying is true for XEX, but that's also why XEX isn't used > for FDE in a lot of cases, because disk sectors aren't typically > divisible by 16. > > https://en.wikipedia.org/wiki/Disk_encryption_theory > > Assuming that's correct, and I don't see any reason to doubt it, then > perhaps it would make sense to have the LSN be unencrypted and include > it in the tweak as that would limit the risk from re-use of the same > tweak over time. > > > Right, my thought was to leave the first 8 bytes of pages, the LSN, > unencrypted > and include the value in the tweak. Just tested that OpenSSL aes-256-xts > handles non multiple-of-16 messages just fine. Great. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 02:52:07PM -0400, Stephen Frost wrote: > > > Is there a particular reason why you would prefer not to use LSN? I > > > suggested > > > it because in my view having a variable tweak is still better than not > > > having > > > it even if we deem the risks of XTS tweak reuse not important for our use > > > case. > > > The comment was made under the assumption that requiring wal_log_hints for > > > encryption is acceptable. > > > > Well, using the LSN means we have to store the LSN unencrypted, and that > > means we have to carve out a 16-byte block on the page that is not > > encrypted. > > With XTS this isn't actually the case though, is it..? Part of the > point of XTS is that the last block doesn't have to be a full 16 bytes. > What you're saying is true for XEX, but that's also why XEX isn't used > for FDE in a lot of cases, because disk sectors aren't typically > divisible by 16. Oh, I was not aware of that XTS feature. Nice. > https://en.wikipedia.org/wiki/Disk_encryption_theory > > Assuming that's correct, and I don't see any reason to doubt it, then > perhaps it would make sense to have the LSN be unencrypted and include > it in the tweak as that would limit the risk from re-use of the same > tweak over time. Yes, seems like a plan. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:57 PM Stephen Frost wrote: > Yes, for integrity verification (also known as 'authenticated > encryption') we'd definitely need to store a larger nonce value. In the > very, very long term, I think it'd be great to have that, and the patch > proposed on this thread seems really cool as a way to get us there. OK. I'm not sure why that has to be relegated to the very, very long term, but I'm really very happy to hear that you think the approach is cool. > Having TDE, even without authenticated encryption, is certainly > valuable. Reducing the amount of engineering required to get there is > worthwhile. Implementing TDE w/ XTS or similar, provided we do agree > that we can do so with an IV that we don't need to find additional space > for, would avoid that page-level format change. I agree we should do > some research to make sure we at least have a reasonable answer to that > question. I've spent a bit of time on that and haven't gotten to a sure > answer one way or the other as yet, but will continue to look. I mean, I feel like this meeting that Bruce was talking about was perhaps making decisions in the wrong order. We have to decide which encryption mode is secure enough for our needs FIRST, and then AFTER that we can decide whether we need to store a nonce in the page. Now if it turns out that we can do either with or without a nonce in the page, then I'm just as happy as anyone else to start with the method that works without a nonce in the page, because like you say, that's less work. But unless we've established that such a method is actually going to survive scrutiny by smart cryptographers, we can't really decide that storing the nonce is off the table. And it doesn't seem like we've established that yet. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, 7 Oct 2021 at 21:52, Stephen Frost wrote: > With XTS this isn't actually the case though, is it..? Part of the > point of XTS is that the last block doesn't have to be a full 16 bytes. > What you're saying is true for XEX, but that's also why XEX isn't used > for FDE in a lot of cases, because disk sectors aren't typically > divisible by 16. > > https://en.wikipedia.org/wiki/Disk_encryption_theory > > Assuming that's correct, and I don't see any reason to doubt it, then > perhaps it would make sense to have the LSN be unencrypted and include > it in the tweak as that would limit the risk from re-use of the same > tweak over time. > Right, my thought was to leave the first 8 bytes of pages, the LSN, unencrypted and include the value in the tweak. Just tested that OpenSSL aes-256-xts handles non multiple-of-16 messages just fine. -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 1:09 PM Bruce Momjian wrote: > > Are you saying a base backup could read a page from the file system and > > see a partial write, even though the write is written as 8k? I had not > > thought about that. > > Yes; see my other response. Yes, that is something that has been seen before. > > I think this whole discussion is about whether we need full page images > > for hint bit changes. I think we do if we use the LSN for the nonce (in > > the old patch), and probably need it for hint bit changes when using > > block cipher modes (XTS) if we feel basebackup could read only part of a > > 16-byte page change. > > I think all the encryption modes that we're still considering have the > (very desirable) property that changing a single bit of the > unencrypted page perturbs the entire output. But that just means that > encrypted clusters will have to run in the same mode as clusters with > checksums, or clusters with wal_log_hints=on, features which the > community has already accepted as having reasonable overhead. I have > in the past expressed skepticism about whether that overhead is really > small enough to be considered acceptable, but if I recall correctly, > the test results posted to the list suggest that you need a working > set just a little bit large than shared_buffers to make it really > sting. And that's not a super-common thing to do. Anyway, if people > aren't screaming about the overhead of that system now, they're not > likely to complain about applying it to some new situation either. Agreed. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 02:44:43PM -0400, Robert Haas wrote: > > I think this whole discussion is about whether we need full page images > > for hint bit changes. I think we do if we use the LSN for the nonce (in > > the old patch), and probably need it for hint bit changes when using > > block cipher modes (XTS) if we feel basebackup could read only part of a > > 16-byte page change. > > I think all the encryption modes that we're still considering have the > (very desirable) property that changing a single bit of the > unencrypted page perturbs the entire output. But that just means that Well, XTS perturbs the 16-byte block, while CBC changes the rest of the page. > encrypted clusters will have to run in the same mode as clusters with > checksums, or clusters with wal_log_hints=on, features which the > community has already accepted as having reasonable overhead. I have > in the past expressed skepticism about whether that overhead is really > small enough to be considered acceptable, but if I recall correctly, > the test results posted to the list suggest that you need a working > set just a little bit large than shared_buffers to make it really > sting. And that's not a super-common thing to do. Anyway, if people > aren't screaming about the overhead of that system now, they're not > likely to complain about applying it to some new situation either. Yes, agreed, good conclusions. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Thu, Oct 7, 2021 at 09:38:45PM +0300, Ants Aasma wrote: > > On Wed, 6 Oct 2021 at 23:08, Bruce Momjian wrote: > > > > Yes, I would prefer we don't use the LSN. I only mentioned it since > > Ants Aasma mentioned LSN use above. > > > > > > Is there a particular reason why you would prefer not to use LSN? I > > suggested > > it because in my view having a variable tweak is still better than not > > having > > it even if we deem the risks of XTS tweak reuse not important for our use > > case. > > The comment was made under the assumption that requiring wal_log_hints for > > encryption is acceptable. > > Well, using the LSN means we have to store the LSN unencrypted, and that > means we have to carve out a 16-byte block on the page that is not > encrypted. With XTS this isn't actually the case though, is it..? Part of the point of XTS is that the last block doesn't have to be a full 16 bytes. What you're saying is true for XEX, but that's also why XEX isn't used for FDE in a lot of cases, because disk sectors aren't typically divisible by 16. https://en.wikipedia.org/wiki/Disk_encryption_theory Assuming that's correct, and I don't see any reason to doubt it, then perhaps it would make sense to have the LSN be unencrypted and include it in the tweak as that would limit the risk from re-use of the same tweak over time. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 12:56 PM Bruce Momjian wrote: > > Uh, do backups get torn and later used? > > Yep. That's why base backup mode forces full_page_writes on > temporarily even if it's off in general. Right, so this shouldn't be an issue as any such torn pages will have an FPI in the WAL that will be replayed as part of restoring that backup. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 1:09 PM Bruce Momjian wrote: > Are you saying a base backup could read a page from the file system and > see a partial write, even though the write is written as 8k? I had not > thought about that. Yes; see my other response. > I think this whole discussion is about whether we need full page images > for hint bit changes. I think we do if we use the LSN for the nonce (in > the old patch), and probably need it for hint bit changes when using > block cipher modes (XTS) if we feel basebackup could read only part of a > 16-byte page change. I think all the encryption modes that we're still considering have the (very desirable) property that changing a single bit of the unencrypted page perturbs the entire output. But that just means that encrypted clusters will have to run in the same mode as clusters with checksums, or clusters with wal_log_hints=on, features which the community has already accepted as having reasonable overhead. I have in the past expressed skepticism about whether that overhead is really small enough to be considered acceptable, but if I recall correctly, the test results posted to the list suggest that you need a working set just a little bit large than shared_buffers to make it really sting. And that's not a super-common thing to do. Anyway, if people aren't screaming about the overhead of that system now, they're not likely to complain about applying it to some new situation either. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 09:38:45PM +0300, Ants Aasma wrote: > On Wed, 6 Oct 2021 at 23:08, Bruce Momjian wrote: > > Yes, I would prefer we don't use the LSN. I only mentioned it since > Ants Aasma mentioned LSN use above. > > > Is there a particular reason why you would prefer not to use LSN? I suggested > it because in my view having a variable tweak is still better than not having > it even if we deem the risks of XTS tweak reuse not important for our use > case. > The comment was made under the assumption that requiring wal_log_hints for > encryption is acceptable. Well, using the LSN means we have to store the LSN unencrypted, and that means we have to carve out a 16-byte block on the page that is not encrypted. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 12:26 PM Andres Freund wrote: > > We rely on it today, e.g. for the control file. > > I think that's the only place, though. We can't rely on it for data > files because base backups don't go through shared buffers, so reads > and writes can get torn in memory and not just on sector boundaries. There was a recent discussion with Munro, as I recall, that actually points out how we probably shouldn't be relying on that even for the control file and proposed having multiple control files (something which I generally agree with as a good idea), particularly due to SSD technology, as I recall. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Wed, 6 Oct 2021 at 23:08, Bruce Momjian wrote: > Yes, I would prefer we don't use the LSN. I only mentioned it since > Ants Aasma mentioned LSN use above. > Is there a particular reason why you would prefer not to use LSN? I suggested it because in my view having a variable tweak is still better than not having it even if we deem the risks of XTS tweak reuse not important for our use case. The comment was made under the assumption that requiring wal_log_hints for encryption is acceptable. -- Ants Aasma Senior Database Engineerwww.cybertec-postgresql.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:56 PM Bruce Momjian wrote: > Uh, do backups get torn and later used? Yep. That's why base backup mode forces full_page_writes on temporarily even if it's off in general. Crazy, right? -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:56:22PM -0400, Bruce Momjian wrote: > On Thu, Oct 7, 2021 at 12:32:16PM -0400, Robert Haas wrote: > > On Thu, Oct 7, 2021 at 12:26 PM Andres Freund wrote: > > > We rely on it today, e.g. for the control file. > > > > I think that's the only place, though. We can't rely on it for data > > files because base backups don't go through shared buffers, so reads > > and writes can get torn in memory and not just on sector boundaries. > > Uh, do backups get torn and later used? Are you saying a base backup could read a page from the file system and see a partial write, even though the write is written as 8k? I had not thought about that. I think this whole discussion is about whether we need full page images for hint bit changes. I think we do if we use the LSN for the nonce (in the old patch), and probably need it for hint bit changes when using block cipher modes (XTS) if we feel basebackup could read only part of a 16-byte page change. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 7, 2021 at 11:45 AM Bruce Momjian wrote: > > I continue to be concerned that a page format change will decrease the > > desirability of this feature by making migration complex and increasing > > its code complexity. I am unclear if it is necessary. > > > > I think the big question is whether XTS with db/relfilenode/blocknumber > > is sufficient as an IV without a nonce that changes for updates. > > Those are fair concerns. I think I agree with everything you say here. > > There was some discussion earlier (not sure if it was on this thread) > about integrity verification. And I don't think that there's any way > we can do that without storing some kind of integrity verifier in each > page. And if we're doing that anyway to support that feature, then > there's no problem if it also includes the IV. I had read Stephen's > previous comments to indicate that he thought we should go this way, > and it sounded cool to me, too. However, it does make migrations > somewhat more complex, because you would then have to actually > dump-and-reload, rather than, perhaps, just encrypting all the > existing pages while the cluster was offline. Personally, I'm not that > fussed about that problem, but I'm also rarely the one who has to help > people migrate to new releases, so I may not be as sympathetic to > those problems there as I should be. Yes, for integrity verification (also known as 'authenticated encryption') we'd definitely need to store a larger nonce value. In the very, very long term, I think it'd be great to have that, and the patch proposed on this thread seems really cool as a way to get us there. > If we don't care about the integrity verification features, then as > you say the next question is whether it's acceptable to use a > predictable nonce that is computing from values that can be known > without looking at the block contents. If so, we can forget about > $SUBJECT and save ourselves some engineering work. If not, then I > think we need to do $SUBJECT anyway. And so far I am not really > convinced that we know which of those two things is the case. I don't, > anyway. Having TDE, even without authenticated encryption, is certainly valuable. Reducing the amount of engineering required to get there is worthwhile. Implementing TDE w/ XTS or similar, provided we do agree that we can do so with an IV that we don't need to find additional space for, would avoid that page-level format change. I agree we should do some research to make sure we at least have a reasonable answer to that question. I've spent a bit of time on that and haven't gotten to a sure answer one way or the other as yet, but will continue to look. Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:32:16PM -0400, Robert Haas wrote: > On Thu, Oct 7, 2021 at 12:26 PM Andres Freund wrote: > > We rely on it today, e.g. for the control file. > > I think that's the only place, though. We can't rely on it for data > files because base backups don't go through shared buffers, so reads > and writes can get torn in memory and not just on sector boundaries. Uh, do backups get torn and later used? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 09:26:26AM -0700, Andres Freund wrote: > Hi, > > On October 7, 2021 8:54:54 AM PDT, Bruce Momjian wrote: > > >Uh, technically most drives use 512-byte sectors, but I don't know if > >there is any guarantee that 512-byte sectors will not be torn --- I have > >a feeling there isn't. I think we get away with the hint bit case > >because you can't tear a single bit. ;-) > > We rely on it today, e.g. for the control file. OK, good to know, and we can be sure the 16-byte blocks will terminate on 512-byte boundaries. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:29:04PM -0400, Robert Haas wrote: > On Thu, Oct 7, 2021 at 11:45 AM Bruce Momjian wrote: > > I continue to be concerned that a page format change will decrease the > > desirability of this feature by making migration complex and increasing > > its code complexity. I am unclear if it is necessary. > > > > I think the big question is whether XTS with db/relfilenode/blocknumber > > is sufficient as an IV without a nonce that changes for updates. > > Those are fair concerns. I think I agree with everything you say here. > > There was some discussion earlier (not sure if it was on this thread) > about integrity verification. And I don't think that there's any way > we can do that without storing some kind of integrity verifier in each > page. And if we're doing that anyway to support that feature, then > there's no problem if it also includes the IV. I had read Stephen's Agreed. > previous comments to indicate that he thought we should go this way, > and it sounded cool to me, too. However, it does make migrations Uh, what has not been publicly stated yet is that there was a meeting, prompted by Stephen, with him, Cybertec staff, and myself on September 16 at the Cybertec office in Vienna to discuss this. After vigorous discussion, it was agreed that a simpliied version of this feature would be implemented that would not have temper detection (beyond encrypted checksums) and would use XTS so that the LSN would not need to be used. > If we don't care about the integrity verification features, then as > you say the next question is whether it's acceptable to use a > predictable nonce that is computing from values that can be known > without looking at the block contents. If so, we can forget about > $SUBJECT and save ourselves some engineering work. If not, then I Yes, that is now the question. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 12:26 PM Andres Freund wrote: > We rely on it today, e.g. for the control file. I think that's the only place, though. We can't rely on it for data files because base backups don't go through shared buffers, so reads and writes can get torn in memory and not just on sector boundaries. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 11:45 AM Bruce Momjian wrote: > I continue to be concerned that a page format change will decrease the > desirability of this feature by making migration complex and increasing > its code complexity. I am unclear if it is necessary. > > I think the big question is whether XTS with db/relfilenode/blocknumber > is sufficient as an IV without a nonce that changes for updates. Those are fair concerns. I think I agree with everything you say here. There was some discussion earlier (not sure if it was on this thread) about integrity verification. And I don't think that there's any way we can do that without storing some kind of integrity verifier in each page. And if we're doing that anyway to support that feature, then there's no problem if it also includes the IV. I had read Stephen's previous comments to indicate that he thought we should go this way, and it sounded cool to me, too. However, it does make migrations somewhat more complex, because you would then have to actually dump-and-reload, rather than, perhaps, just encrypting all the existing pages while the cluster was offline. Personally, I'm not that fussed about that problem, but I'm also rarely the one who has to help people migrate to new releases, so I may not be as sympathetic to those problems there as I should be. If we don't care about the integrity verification features, then as you say the next question is whether it's acceptable to use a predictable nonce that is computing from values that can be known without looking at the block contents. If so, we can forget about $SUBJECT and save ourselves some engineering work. If not, then I think we need to do $SUBJECT anyway. And so far I am not really convinced that we know which of those two things is the case. I don't, anyway. -- Robert Haas EDB: http://www.enterprisedb.com
Re: storing an explicit nonce
Hi, On October 7, 2021 8:54:54 AM PDT, Bruce Momjian wrote: >Uh, technically most drives use 512-byte sectors, but I don't know if >there is any guarantee that 512-byte sectors will not be torn --- I have >a feeling there isn't. I think we get away with the hint bit case >because you can't tear a single bit. ;-) We rely on it today, e.g. for the control file. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 10:27:15AM +0200, Antonin Houska wrote: > Bruce Momjian wrote: > > The above text isn't very clear. What I am saying is that currently > > torn pages can be tolerated by hint bit writes because only a single > > byte is changing. If we use a block cipher like AES-XTS, later 16-byte > > encrypted blocks would be changed by hint bit changes, meaning torn > > pages could not be tolerated. This means we would have to use full page > > writes for hint bit changes, perhaps making this feature have > > unacceptable performance overhead. > > IIRC, in the XTS scheme, a change of a single byte in the 16-byte block causes > the whole encrypted block to be different after the next encryption, however > the following blocks are not affected. CBC (cipher-block chaining) is the mode > where the change in one block does affect the encryption of the following > block. Oh, good point. I was not aware of that. It means XTS does not feed the previous block as part of the nonce to the next block. > I'm not sure if this fact is important from the hint bit perspective > though. It would be an important difference if there was a guarantee that the > 16-byte blocks are consitent even on torn page - does e.g. proper alignment of > pages guarantee that? Nevertheless, the absence of the chaining may be a > reason to prefer CBC to XTS anyway. Uh, technically most drives use 512-byte sectors, but I don't know if there is any guarantee that 512-byte sectors will not be torn --- I have a feeling there isn't. I think we get away with the hint bit case because you can't tear a single bit. ;-) However, my patch created a full page write for hint bit changes. If we don't use the LSN, those full page writes will only happen once per checkpoint, which seems acceptable, at least to Robert. Interesting on the CBC idea which would force the rest of the page to change --- not sure if that is valuable. I know stream ciphers can be diff'ed to see data because they are xor'ing the data --- I don't remember if block ciphers have similar weaknesses. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: storing an explicit nonce
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Oct 6, 2021 at 3:17 PM Stephen Frost wrote: > > With AES-XTS, we don't need to use the LSN as part of the nonce though, > > so I don't think this argument is actually valid..? As discussed > > previously regarding AES-XTS, the general idea was to use the path to > > the file and the filename itself plus the block number as the IV, and > > that works fine for XTS because it's ok to reuse it (unlike with CTR). > > However, there's also the option of storing a nonce in each page, as > suggested by the subject of this thread. I think that's probably a > pretty workable approach, as demonstrated by the patch that started > this thread. We'd need to think a bit carefully about whether any of > the compile-time calculations the patch moves to runtime are expensive > enough to matter and whether any such impacts can be mitigated, but I > think there is a good chance that such issues are manageable. I agree with this in general, though I would think we'd use that for GCM or another authenticated encryption mode (perhaps GCM-SIV with the LSN as the IV) at some point off in the future. Alternatively, we could use that technique to just provide a better per-page checksum than what we have today. Maybe we could figure out how to leverage that to move to 64bit transaction IDs with some page-level epoch. Definitely a lot of possibilities. Ultimately though, regarding TDE at least, I would think we'd rather start with something that's block level and doesn't require a page format change. > I'm a little concerned by the email from "Sasasu" saying that even in > XTS reusing the IV is not cryptographically weak. I don't know enough > about these different encryption modes to know if he's right, but if > he is then perhaps we need to consider his suggestion of using > AES-GCM. Or, uh, something else. Think you meant 'strong' above (or maybe omit the 'not', either way the oppostie of the double-negative that seems to be what was written). As I understand it, XTS isn't great for dealing with someone who has ongoing access to watch writes over time, just in general, but that wasn't what it is generally used to address (and isn't what we would be looking for it to address either). Perhaps there's other modes which don't require that we change the page format to support them besides XTS (in particular, as our pages are multiples of 16 bytes, it's possible we don't really need XTS since there aren't any partial blocks and could simply use XEX instead..) Thanks, Stephen signature.asc Description: PGP signature
Re: storing an explicit nonce
On Thu, Oct 7, 2021 at 10:28:55AM -0400, Robert Haas wrote: > However, there's also the option of storing a nonce in each page, as > suggested by the subject of this thread. I think that's probably a > pretty workable approach, as demonstrated by the patch that started > this thread. We'd need to think a bit carefully about whether any of > the compile-time calculations the patch moves to runtime are expensive > enough to matter and whether any such impacts can be mitigated, but I > think there is a good chance that such issues are manageable. > > I'm a little concerned by the email from "Sasasu" saying that even in > XTS reusing the IV is not cryptographically weak. I don't know enough > about these different encryption modes to know if he's right, but if > he is then perhaps we need to consider his suggestion of using > AES-GCM. Or, uh, something else. I continue to be concerned that a page format change will decrease the desirability of this feature by making migration complex and increasing its code complexity. I am unclear if it is necessary. I think the big question is whether XTS with db/relfilenode/blocknumber is sufficient as an IV without a nonce that changes for updates. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
