public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/1] In psql \copy from, send data to server in larger chunks. 3+ messages / 3 participants [nested] [flat]
* [PATCH 1/1] In psql \copy from, send data to server in larger chunks. @ 2021-02-06 22:10 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Heikki Linnakangas @ 2021-02-06 22:10 UTC (permalink / raw) Previously, we would send each line as a separate CopyData message. That's pretty wasteful if the table is narrow, as each CopyData message has 5 bytes of overhead. For efficiency, buffer up and pack 8 kB of input data into each CopyData message. --- src/bin/psql/copy.c | 114 +++++++++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 45 deletions(-) diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c index 78f0dc5a507..8c56f2470e2 100644 --- a/src/bin/psql/copy.c +++ b/src/bin/psql/copy.c @@ -581,77 +581,101 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res) else { bool copydone = false; + int buflen; + bool at_line_begin = true; + if (showprompt) + { + const char *prompt = get_prompt(PROMPT_COPY, NULL); + + fputs(prompt, stdout); + fflush(stdout); + } + + /* + * In text mode, We have to read the input one line at a time, so that + * we can stop reading at the EOF marker (\.). We mustn't read beyond + * the EOF marker, because if the data is inlined in a SQL script, we + * would eat up the commands after the EOF marker. + */ + buflen = 0; while (!copydone) - { /* for each input line ... */ - bool firstload; - bool linedone; + { + int linelen; + char *fgresult; - if (showprompt) + if (at_line_begin && showprompt) { - const char *prompt = get_prompt(PROMPT_COPY, NULL); + const char *prompt; + sigint_interrupt_enabled = false; + + prompt = get_prompt(PROMPT_COPY, NULL); fputs(prompt, stdout); fflush(stdout); - } - firstload = true; - linedone = false; - - while (!linedone) - { /* for each bufferload in line ... */ - int linelen; - char *fgresult; - - /* enable longjmp while waiting for input */ sigint_interrupt_enabled = true; + } - fgresult = fgets(buf, sizeof(buf), copystream); - - sigint_interrupt_enabled = false; + /* enable longjmp while waiting for input */ + sigint_interrupt_enabled = true; - if (!fgresult) - { - copydone = true; - break; - } + fgresult = fgets(&buf[buflen], COPYBUFSIZ - buflen, copystream); - linelen = strlen(buf); + sigint_interrupt_enabled = false; - /* current line is done? */ - if (linelen > 0 && buf[linelen - 1] == '\n') - linedone = true; + if (!fgresult) + copydone = true; + else + { + linelen = strlen(fgresult); + buflen += linelen; - /* check for EOF marker, but not on a partial line */ - if (firstload) + if (buf[buflen - 1] == '\n') { - /* - * This code erroneously assumes '\.' on a line alone - * inside a quoted CSV string terminates the \copy. - * https://www.postgresql.org/message-id/[email protected] - */ - if (strcmp(buf, "\\.\n") == 0 || - strcmp(buf, "\\.\r\n") == 0) + /* check for EOF marker, but not on a partial line */ + if (at_line_begin) { - copydone = true; - break; + /* + * This code erroneously assumes '\.' on a line alone + * inside a quoted CSV string terminates the \copy. + * https://www.postgresql.org/message-id/[email protected] + */ + if ((linelen == 3 && memcmp(fgresult, "\\.\n", 3) == 0) || + (linelen == 4 && memcmp(fgresult, "\\.\r\n", 4) == 0)) + { + copydone = true; + } } - firstload = false; + if (copystream == pset.cur_cmd_source) + { + pset.lineno++; + pset.stmt_lineno++; + } + at_line_begin = true; } + else + at_line_begin = false; + } - if (PQputCopyData(conn, buf, linelen) <= 0) + /* + * If the buffer is full, or we've reached the EOF, flush it. + * + * Make sure there's always space for four more bytes in the buffer, + * plus a NUL terminator. That way, an EOF marker is never split + * across two fgets() calls, which simplies the logic. + */ + if (buflen >= COPYBUFSIZ - 5 || (copydone && buflen > 0)) + { + if (PQputCopyData(conn, buf, buflen) <= 0) { OK = false; copydone = true; break; } - } - if (copystream == pset.cur_cmd_source) - { - pset.lineno++; - pset.stmt_lineno++; + buflen = 0; } } } -- 2.30.0 --------------20DC7BBD34247104D0B00618-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Transparent column encryption @ 2023-01-19 20:48 Jacob Champion <[email protected]> 2023-01-25 19:00 ` Re: Transparent column encryption Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Jacob Champion @ 2023-01-19 20:48 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; pgsql-hackers On 12/31/22 06:17, Peter Eisentraut wrote: > On 21.12.22 06:46, Peter Eisentraut wrote: >> And another update. The main changes are that I added an 'unspecified' >> CMK algorithm, which indicates that the external KMS knows what it is >> but the database system doesn't. This was discussed a while ago. I >> also changed some details about how the "cmklookup" works in libpq. Also >> added more code comments and documentation and rearranged some code. Trying to delay a review until I had "completed it" has only led to me not reviewing, so here's a partial one. Let me know what pieces of the implementation and/or architecture you're hoping to get more feedback on. I like the existing "caveats" documentation, and I've attached a sample patch with some more caveats documented, based on some of the upthread conversation: - text format makes fixed-length columns leak length information too - you only get partial protection against the Evil DBA - RSA-OAEP public key safety (Feel free to use/remix/discard as desired.) When writing the paragraph on RSA-OAEP I was reminded that we didn't really dig into the asymmetric/symmetric discussion. Assuming that most first-time users will pick the builtin CMK encryption method, do we still want to have an asymmetric scheme implemented first instead of a symmetric keywrap? I'm still concerned about that public key, since it can't really be made public. (And now that "unspecified" is available, I think an asymmetric CMK could be easily created by users that have a niche use case, and then we wouldn't have to commit to supporting it forever.) For the padding caveat: > + There is no concern if all values are of the same length (e.g., credit > + card numbers). I nodded along to this statement last year, and then this year I learned that CCNs aren't fixed-length. So with a 16-byte block, you're probably going to be able to figure out who has an American Express card. The column encryption algorithm is set per-column -- but isn't it tightly coupled to the CEK, since the key length has to match? From a layperson perspective, using the same key to encrypt the same plaintext under two different algorithms (if they happen to have the same key length) seems like it might be cryptographically risky. Is there a reason I should be encouraged to do that? With the loss of \gencr it looks like we also lost a potential way to force encryption from within psql. Any plans to add that for v1? While testing, I forgot how the new option worked and connected with `column_encryption=on` -- and then I accidentally sent unencrypted data to the server, since `on` means "not enabled". :( The server errors out after the damage is done, of course, but would it be okay to strictly validate that option's values? Are there plans to document client-side implementation requirements, to ensure cross-client compatibility? Things like the "PG\x00\x01" associated data are buried at the moment (or else I've missed them in the docs). If you're holding off until the feature is more finalized, that's fine too. Speaking of cross-client compatibility, I'm still disconcerted by the ability to write the value "hello world" into an encrypted integer column. Should clients be required to validate the text format, using the attrealtypid? It occurred to me when looking at the "unspecified" CMK scheme that the CEK doesn't really have to be an encryption key at all. In that case it can function more like a (possibly signed?) cookie for lookup, or even be ignored altogether if you don't want to use a wrapping scheme (similar to JWE's "direct" mode, maybe?). So now you have three ways to look up or determine a column encryption key (CMK realm, CMK name, CEK cookie)... is that a concept worth exploring in v1 and/or the documentation? Thanks, --Jacob diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 55f33a2f5f..06e1c077d5 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -1588,7 +1588,33 @@ export PGCMKLOOKUP card numbers). But if there are signficant length differences between valid values and that length information is security-sensitive, then application-specific workarounds such as padding would need to be applied. - How to do that securely is beyond the scope of this manual. + How to do that securely is beyond the scope of this manual. Note that + column encryption is applied to the text representation of the stored value, + so length differences can be leaked even for fixed-length column types (e.g. + <literal>bigint</literal>, whose largest decimal representation is longer + than 16 bytes). + </para> + + <para> + Column encryption provides only partial protection against a malicious + user with write access to the table. Once encrypted, any modifications to a + stored value on the server side will cause a decryption failure on the + client. However, a user with write access may still freely swap encrypted + values between rows or columns (or even separate database clusters) as long + as they were encrypted with the same key. Attackers may also remove values + by replacing them with nulls, and users with ownership over the table schema + may replace encryption keys or strip encryption from the columns entirely. + All of this is to say: proper access control is still of vital importance + when using this feature. + </para> + + <para> + When using the RSA-OAEP CEK encryption methods, the "public" half of the CMK + may be used to replace existing column encryption keys with keys of an + attacker's choosing, compromising confidentiality and authenticity for + values encrypted under that CMK. For this reason, it's important to keep + both the private <emphasis>and</emphasis> public halves of the CMK keypair + confidential. </para> <note> Attachments: [text/plain] caveats.diff.txt (2.0K, ../../[email protected]/2-caveats.diff.txt) download | inline diff: diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 55f33a2f5f..06e1c077d5 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -1588,7 +1588,33 @@ export PGCMKLOOKUP card numbers). But if there are signficant length differences between valid values and that length information is security-sensitive, then application-specific workarounds such as padding would need to be applied. - How to do that securely is beyond the scope of this manual. + How to do that securely is beyond the scope of this manual. Note that + column encryption is applied to the text representation of the stored value, + so length differences can be leaked even for fixed-length column types (e.g. + <literal>bigint</literal>, whose largest decimal representation is longer + than 16 bytes). + </para> + + <para> + Column encryption provides only partial protection against a malicious + user with write access to the table. Once encrypted, any modifications to a + stored value on the server side will cause a decryption failure on the + client. However, a user with write access may still freely swap encrypted + values between rows or columns (or even separate database clusters) as long + as they were encrypted with the same key. Attackers may also remove values + by replacing them with nulls, and users with ownership over the table schema + may replace encryption keys or strip encryption from the columns entirely. + All of this is to say: proper access control is still of vital importance + when using this feature. + </para> + + <para> + When using the RSA-OAEP CEK encryption methods, the "public" half of the CMK + may be used to replace existing column encryption keys with keys of an + attacker's choosing, compromising confidentiality and authenticity for + values encrypted under that CMK. For this reason, it's important to keep + both the private <emphasis>and</emphasis> public halves of the CMK keypair + confidential. </para> <note> ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Transparent column encryption 2023-01-19 20:48 Re: Transparent column encryption Jacob Champion <[email protected]> @ 2023-01-25 19:00 ` Peter Eisentraut <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Peter Eisentraut @ 2023-01-25 19:00 UTC (permalink / raw) To: Jacob Champion <[email protected]>; pgsql-hackers On 19.01.23 21:48, Jacob Champion wrote: > I like the existing "caveats" documentation, and I've attached a sample > patch with some more caveats documented, based on some of the upthread > conversation: > > - text format makes fixed-length columns leak length information too > - you only get partial protection against the Evil DBA > - RSA-OAEP public key safety > > (Feel free to use/remix/discard as desired.) I have added those in the v15 patch I just posted. > When writing the paragraph on RSA-OAEP I was reminded that we didn't > really dig into the asymmetric/symmetric discussion. Assuming that most > first-time users will pick the builtin CMK encryption method, do we > still want to have an asymmetric scheme implemented first instead of a > symmetric keywrap? I'm still concerned about that public key, since it > can't really be made public. I had started coding that, but one problem was that the openssl CLI doesn't really provide any means to work with those kinds of keys. The "openssl enc" command always wants to mix in a password. Without that, there is no way to write a test case, and more crucially no way for users to set up these kinds of keys. Unless we write our own tooling for this, which, you know, the patch just passed 400k in size. > For the padding caveat: > >> + There is no concern if all values are of the same length (e.g., credit >> + card numbers). > > I nodded along to this statement last year, and then this year I learned > that CCNs aren't fixed-length. So with a 16-byte block, you're probably > going to be able to figure out who has an American Express card. Heh. I have removed that parenthetical remark. > The column encryption algorithm is set per-column -- but isn't it > tightly coupled to the CEK, since the key length has to match? From a > layperson perspective, using the same key to encrypt the same plaintext > under two different algorithms (if they happen to have the same key > length) seems like it might be cryptographically risky. Is there a > reason I should be encouraged to do that? Not really. I was also initially confused by this setup, but that's how other similar systems are set up, so I thought it would be confusing to do it differently. > With the loss of \gencr it looks like we also lost a potential way to > force encryption from within psql. Any plans to add that for v1? \gencr didn't do that either. We could do it. The libpq API supports it. We just need to come up with some syntax for psql. > While testing, I forgot how the new option worked and connected with > `column_encryption=on` -- and then I accidentally sent unencrypted data > to the server, since `on` means "not enabled". :( The server errors out > after the damage is done, of course, but would it be okay to strictly > validate that option's values? fixed in v15 > Are there plans to document client-side implementation requirements, to > ensure cross-client compatibility? Things like the "PG\x00\x01" > associated data are buried at the moment (or else I've missed them in > the docs). If you're holding off until the feature is more finalized, > that's fine too. This is documented in the protocol chapter, which I thought was the right place. Did you want more documentation, or in a different place? > Speaking of cross-client compatibility, I'm still disconcerted by the > ability to write the value "hello world" into an encrypted integer > column. Should clients be required to validate the text format, using > the attrealtypid? Well, we can ask them to, but we can't really require them, in a cryptographic sense. I'm not sure what more we can do. > It occurred to me when looking at the "unspecified" CMK scheme that the > CEK doesn't really have to be an encryption key at all. In that case it > can function more like a (possibly signed?) cookie for lookup, or even > be ignored altogether if you don't want to use a wrapping scheme > (similar to JWE's "direct" mode, maybe?). So now you have three ways to > look up or determine a column encryption key (CMK realm, CMK name, CEK > cookie)... is that a concept worth exploring in v1 and/or the documentation? I don't completely follow this. ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-01-25 19:00 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-02-06 22:10 [PATCH 1/1] In psql \copy from, send data to server in larger chunks. Heikki Linnakangas <[email protected]> 2023-01-19 20:48 Re: Transparent column encryption Jacob Champion <[email protected]> 2023-01-25 19:00 ` Re: Transparent column encryption Peter Eisentraut <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox