Re: Adding support for SSLKEYLOGFILE in the frontend
Thanks, Daniel. Here is an updated patch with all previous changes,
added a simple connection test and another check to make sure file
mode is correct, and the env var fix. Please let me know if anything
needs to be changed. I tested this locally using meson running all TAP
tests, and also manually.
Thanks
Abhishek
On Fri, Jan 10, 2025 at 9:29 AM Daniel Gustafsson wrote:
>
> > On 10 Jan 2025, at 04:59, Abhishek Chanda wrote:
>
> Thanks for the new version.
>
> + {"sslkeylogfile", "SSLKEYLOGFILE",
> The env var should be PGSSLKEYLOGFILE with the PG prefix.
>
> > 3. Added docs and tests
>
> There is no test in the attached patch, did you write one but forgot to git
> add
> it before committing locally?
>
> --
> Daniel Gustafsson
>
--
Thanks and regards
Abhishek Chanda
v3-0001-Add-support-for-dumping-SSL-keylog-to-a-file.patch
Description: Binary data
Re: Adding support for SSLKEYLOGFILE in the frontend
Thanks for the feedback, everyone. Attached a followup with the following changes compared to the initial version: 1. Converted sslkeylogfile to a connection parameter 2. Added a mechanism to chmod the key log file to 0600 3. Added docs and tests I tested this manually. Also ran make check and make check-world locally. Please let me know if this needs any other changes. Thanks On Thu, Jan 9, 2025 at 2:36 PM Jacob Champion wrote: > > On Wed, Jan 8, 2025 at 5:17 PM Tom Lane wrote: > > I think it might be safer if we only accepted it as a connection > > parameter and not via an environment variable. > > Making it a connection parameter also keeps us from colliding with any > other linked libraries' use of SSLKEYLOGFILE (I'm thinking about > libcurl at the moment, but I think maybe NSS used it too?). > > --Jacob -- Thanks and regards Abhishek Chanda v2-0001-Add-support-for-dumping-SSL-keylog-to-a-file.patch Description: Binary data
Adding support for SSLKEYLOGFILE in the frontend
Hello folks, Attached is a patch to add support for logging secrets used in TLS connection after psql is initialized. This adds a new env var SSLKEYLOGFILE on the client side that points to a text file where keys will be logged. If a user runs psql multiple times with the same SSLKEYLOGFILE, new entries will be appended to that file. There is no change in behavior if that env var is not set or set to an empty string. This is useful for cases when a client wants to analyze TCP packets using a tool like wireshark while using TLS. This will enable wireshark to decrypt the packets and decode as postgres wire protocol messages. I did not add this to the backend because I thought using wireshark is more common on the frontend. The keylogfile format is documented here https://www.ietf.org/archive/id/draft-thomson-tls-keylogfile-00.html Example usage: root@guest:~/postgres# SSLKEYLOGFILE=./key.txt /usr/local/pgsql/bin/psql "postgresql://user:pass@host:5432/postgres?sslmode=require" psql (18devel, server 17.2) SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384, compression: off, ALPN: postgresql) Type "help" for help. postgres=> \q root@guest:~/postgres# cat key.txt SERVER_HANDSHAKE_TRAFFIC_SECRET EXPORTER_SECRET SERVER_TRAFFIC_SECRET_0 CLIENT_HANDSHAKE_TRAFFIC_SECRET *** CLIENT_TRAFFIC_SECRET_0 A few things I am not sure about: 1. Where should I add automated tests and docs for this? I did not see any unit tests for the surrounding functions. 2. Should I use perror to report error here? I did not want to use libpq_append_conn_error because this is not a connection related error. Please let me know if I can clarify anything. -- Thanks and regards Abhishek Chanda 0001-Add-support-for-dumping-SSL-keylog-to-a-file.patch Description: Binary data
Re: Adding error messages to a few slash commands
Thanks for the feedback, attached an updated patch that changes most of those "Did not find" messages to empty tables. I did not change two sets, listDbRoleSettings and listTables both have comments describing that these are deliberately this way. I wanted to post this update to close this loop for now. I will follow up once the merge window opens again. Thanks On Sun, Apr 13, 2025 at 1:47 AM Pavel Luzanov wrote: > > On 13.04.2025 08:29, Tom Lane wrote: > > Abhishek Chanda writes: > > Currently, some slash commands in psql return an error saying "Did not > find any named " while some return an empty table. This patch > changes a few of the slash commands to return a similar error message. > > > +1 for this patch. > > Personally, if I were trying to make these things consistent, I'd have > gone in the other direction (ie return empty tables). > > > +1 > Returning empty tables is a more appropriate behavior. > > -- > Pavel Luzanov > Postgres Professional: https://postgrespro.com -- Thanks and regards Abhishek Chanda v2-0001-Print-empty-table-when-a-given-object-is-not-foun.patch Description: Binary data
Re: Adding error messages to a few slash commands
Thanks Pavel, done now https://commitfest.postgresql.org/patch/5699/ Thanks On Mon, Apr 14, 2025 at 4:27 AM Pavel Luzanov wrote: > > On 13.04.2025 19:40, Abhishek Chanda wrote: > > Thanks for the feedback, attached an updated patch that changes most > of those "Did not find" messages to empty tables. I did not change two > sets, listDbRoleSettings and listTables both have comments describing > that these are deliberately this way. > > > Thanks. > > I wanted to post this update to close this loop for now. I will follow > up once the merge window opens again. > > > I recommend to create a new entry for this patch in the open July commitfest. > I will be able to review this patch in a couple months later in June, > if no one wants to review it earlier. > > -- > Pavel Luzanov > Postgres Professional: https://postgrespro.com -- Thanks and regards Abhishek Chanda
Adding error messages to a few slash commands
Hello hackers, Currently, some slash commands in psql return an error saying "Did not find any named " while some return an empty table. This patch changes a few of the slash commands to return a similar error message. I did not change all of the slash commands in this initial patch to wait for feedback, happy to do that if this patch is acceptable. I did not add any tests yet because the existing ones did not seem to have any, happy to add tests if needed. Also, I know that we are in a feature freeze, is such a change acceptable now? Thanks -- Thanks and regards Abhishek Chanda v1-0001-Add-an-error-message-when-a-given-object-is-not-f.patch Description: Binary data
Re: Adding error messages to a few slash commands
Sorry, I forgot to attach example usage. Here is how these currently behave: postgres=> \dT foo List of data types Schema | Name | Description +--+- (0 rows) postgres => \du foo List of roles Role name | Attributes ---+ postgres => \df foo List of functions Schema | Name | Result data type | Argument data types | Type +--+--+-+-- (0 rows) Here is how these look like after this change: postgres=> \dT foo Did not find any data type named "foo". postgres => \df foo Did not find any function named "foo". postgres => \du foo Did not find any role named "foo". Thanks On Sat, Apr 12, 2025 at 10:43 PM Abhishek Chanda wrote: > > Hello hackers, > > Currently, some slash commands in psql return an error saying "Did not > find any named " while some return an empty table. This patch > changes a few of the slash commands to return a similar error message. > I did not change all of the slash commands in this initial patch to > wait for feedback, happy to do that if this patch is acceptable. I did > not add any tests yet because the existing ones did not seem to have > any, happy to add tests if needed. Also, I know that we are in a > feature freeze, is such a change acceptable now? > > Thanks > > -- > Thanks and regards > Abhishek Chanda -- Thanks and regards Abhishek Chanda
Re: Adding support for SSLKEYLOGFILE in the frontend
Attached a v6 with O_NOFOLLOW removed, I noticed that it failed on
windows. Please let me know if I should do this in any other way that
is portable across platforms.
Thanks
On Thu, Feb 27, 2025 at 11:39 PM Abhishek Chanda
wrote:
>
> Thanks for the review, Daniel. Please find v5 of this patch attached.
> I tried to bump up the minimum OpenBSD version in installation.sgml,
> do I need to add this anywhere else? Please let me know if this needs
> anything else.
>
> Thanks
>
> On Thu, Feb 20, 2025 at 4:25 PM Daniel Gustafsson wrote:
> >
> > > On 20 Feb 2025, at 04:36, Abhishek Chanda wrote:
> > > Please find attached a new version of this patch. I rebased on master,
> > > added better error reporting and skipped the permissions check on
> > > windows. Please let me know if this needs any changes. I tested this
> > > locally using meson running all TAP tests.
> >
> > Thanks for the new version, a few comments on this one from reading:
> >
> > +./src/test/ssl/key.txt
> > The toplevel .gitignore should not be used for transient test output, there
> > is
> > a .gitignore in src/test/ssl for that. The key.txt file should also be
> > placed
> > in PostgreSQL::Test::Utils::tempdir and then cleaned up after the test.
> >
> >
> > + > xreflabel="sslkeylogfile">
> > The documentation should state that the file will use the NSS format.
> >
> >
> > + if (log_file == NULL) {
> > + libpq_append_conn_error(conn, "could not open ssl key log
> > ...
> > + }
> > This raise an error for not being able to operate on the file, but it
> > doesn't
> > error out from the function and instead keeps going with more operations on
> > the
> > file which couldn't be opened.
> >
> >
> > + if (chmod(conn->sslkeylogfile, 0600) == -1) {
> > Rather than opening and try to set permissions separately, why not use
> > open(2)
> > and set the umask? We probably also want to set O_NOFOLLOW.
> >
> >
> > + if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0)
> > + SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb);
> > This callback does exist in OpenSSL 1.1.1 but it's only available in
> > LibreSSL
> > from OpenBSD 7.1 and onwards where we support 7.0 as the earliest version.
> > This feature seems like a good enough reason to bump the minimum LibreSSL
> > version, and 7.1 is well out support (7.5 goes EOL next), but it would need
> > to
> > get done here.
> >
> >
> > +# Skip file mode check on Windows
> > +return 1 if $windows_os;
> > It should be up to each individual test to determine what to skip, not the
> > library code (and it should use SKIP:). src/test/ssl/t/001_ssltests.pl is
> > an
> > example which skips permissions checks on Windows. Also, I'm not sure we
> > need
> > a generic function in the testlibrary for something so basic.
> >
> >
> > + print STDERR sprintf("%s mode must be %04o\n",
> > + $path, $expected_file_mode);
> > Test code should not print anything to stdout/stderr, it should use the TAP
> > compliant methods like diag() etc for this.
> >
> >
> > + "connect with server root certand sslkeylogfile=key.txt");
> > s/certand/cert and/ ?
> >
> > --
> > Daniel Gustafsson
> >
>
>
> --
> Thanks and regards
> Abhishek Chanda
--
Thanks and regards
Abhishek Chanda
v6-0001-Add-support-for-dumping-SSL-keylog-to-a-file.patch
Description: Binary data
Re: Adding support for SSLKEYLOGFILE in the frontend
Thanks for the review, Daniel. Please find v5 of this patch attached.
I tried to bump up the minimum OpenBSD version in installation.sgml,
do I need to add this anywhere else? Please let me know if this needs
anything else.
Thanks
On Thu, Feb 20, 2025 at 4:25 PM Daniel Gustafsson wrote:
>
> > On 20 Feb 2025, at 04:36, Abhishek Chanda wrote:
> > Please find attached a new version of this patch. I rebased on master,
> > added better error reporting and skipped the permissions check on
> > windows. Please let me know if this needs any changes. I tested this
> > locally using meson running all TAP tests.
>
> Thanks for the new version, a few comments on this one from reading:
>
> +./src/test/ssl/key.txt
> The toplevel .gitignore should not be used for transient test output, there is
> a .gitignore in src/test/ssl for that. The key.txt file should also be placed
> in PostgreSQL::Test::Utils::tempdir and then cleaned up after the test.
>
>
> + xreflabel="sslkeylogfile">
> The documentation should state that the file will use the NSS format.
>
>
> + if (log_file == NULL) {
> + libpq_append_conn_error(conn, "could not open ssl key log ...
> + }
> This raise an error for not being able to operate on the file, but it doesn't
> error out from the function and instead keeps going with more operations on
> the
> file which couldn't be opened.
>
>
> + if (chmod(conn->sslkeylogfile, 0600) == -1) {
> Rather than opening and try to set permissions separately, why not use open(2)
> and set the umask? We probably also want to set O_NOFOLLOW.
>
>
> + if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0)
> + SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb);
> This callback does exist in OpenSSL 1.1.1 but it's only available in LibreSSL
> from OpenBSD 7.1 and onwards where we support 7.0 as the earliest version.
> This feature seems like a good enough reason to bump the minimum LibreSSL
> version, and 7.1 is well out support (7.5 goes EOL next), but it would need to
> get done here.
>
>
> +# Skip file mode check on Windows
> +return 1 if $windows_os;
> It should be up to each individual test to determine what to skip, not the
> library code (and it should use SKIP:). src/test/ssl/t/001_ssltests.pl is an
> example which skips permissions checks on Windows. Also, I'm not sure we need
> a generic function in the testlibrary for something so basic.
>
>
> +print STDERR sprintf("%s mode must be %04o\n",
> + $path, $expected_file_mode);
> Test code should not print anything to stdout/stderr, it should use the TAP
> compliant methods like diag() etc for this.
>
>
> + "connect with server root certand sslkeylogfile=key.txt");
> s/certand/cert and/ ?
>
> --
> Daniel Gustafsson
>
--
Thanks and regards
Abhishek Chanda
v5-0001-Add-support-for-dumping-SSL-keylog-to-a-file.patch
Description: Binary data
Re: Adding support for SSLKEYLOGFILE in the frontend
Thanks, Daniel.
Should there be the ifdef guard in here as well?
+ {"sslkeylogfile", NULL, NULL, NULL,
+ "SSL-Key-Log-File", "", 0, /* sizeof("") = 0 */
+ offsetof(struct pg_conn, sslkeylogfile)},
+
A small nit, this line should say NULL
+ /* line is guaranteed by OpenSSL to be NUL terminated */
Thanks
On Thu, Mar 13, 2025 at 5:07 PM Daniel Gustafsson wrote:
>
>
>
> > On 13 Mar 2025, at 19:31, Tom Lane wrote:
> >
> > Jacob Champion writes:
> >> Adding the PG prefix to the envvar name addresses my collision
> >> concern, but I think Tom's comment upthread [1] was saying that we
> >> should not provide any envvar at all:
> >
> >>> I think it might be safer if we only accepted it as a connection
> >>> parameter and not via an environment variable.
> >
> >> Is the addition of the PG prefix enough to address that concern too?
> >
> > Indeed, I was advocating for *no* environment variable. The PG prefix
> > does not comfort me.
>
> Attached is a rebased version which fixes the test failure under autoconf (I
> had missed git adding the configure file..) and Windows where the backslashes
> weren't escaped properly. It also removes the environment variable and has
> documentation touchups.
>
> --
> Daniel Gustafsson
>
--
Thanks and regards
Abhishek Chanda
Re: Adding support for SSLKEYLOGFILE in the frontend
Hi all,
Please find attached a new version of this patch. I rebased on master,
added better error reporting and skipped the permissions check on
windows. Please let me know if this needs any changes. I tested this
locally using meson running all TAP tests.
Thanks
On Fri, Jan 10, 2025 at 5:16 PM Abhishek Chanda wrote:
>
> Thanks, Daniel. Here is an updated patch with all previous changes,
> added a simple connection test and another check to make sure file
> mode is correct, and the env var fix. Please let me know if anything
> needs to be changed. I tested this locally using meson running all TAP
> tests, and also manually.
>
> Thanks
> Abhishek
>
> On Fri, Jan 10, 2025 at 9:29 AM Daniel Gustafsson wrote:
> >
> > > On 10 Jan 2025, at 04:59, Abhishek Chanda wrote:
> >
> > Thanks for the new version.
> >
> > + {"sslkeylogfile", "SSLKEYLOGFILE",
> > The env var should be PGSSLKEYLOGFILE with the PG prefix.
> >
> > > 3. Added docs and tests
> >
> > There is no test in the attached patch, did you write one but forgot to git
> > add
> > it before committing locally?
> >
> > --
> > Daniel Gustafsson
> >
>
>
> --
> Thanks and regards
> Abhishek Chanda
--
Thanks and regards
Abhishek Chanda
v4-0001-Add-support-for-dumping-SSL-keylog-to-a-file.patch
Description: Binary data
