sslinfo extension - add notbefore and notafter timestamps

On 20 Aug 2022, at 01:00, Cary Huang <cary.huang@highgo.ca> wrote:

I noticed that sslinfo extension does not have functions to return current client certificate's notbefore and notafter timestamps which are also quite important attributes in a X509 certificate. The attached patch adds 2 functions to get notbefore and notafter timestamps from the currently connected client certificate.

Off the cuff that doesn't seem like a bad idea, but I wonder if we should add
them to pg_stat_ssl (or both) instead if we deem them valuable?

Re the patch, it would be nice to move the logic in ssl_client_get_notafter and
the _notbefore counterpart to a static function since they are copies of
eachother.

--
Daniel Gustafsson https://vmware.com/

Off the cuff that doesn't seem like a bad idea, but I wonder if we should add
them to pg_stat_ssl (or both) instead if we deem them valuable?

I think the same information should be available to pg_stat_ssl as well. pg_stat_ssl can show the client certificate information for all connecting clients, having it to show not_before and not_after timestamps of every client are important in my opinion. The attached patch "v2-0002-pg-stat-ssl-add-notbefore-and-notafter-timestamps.patch" adds this support

Re the patch, it would be nice to move the logic in ssl_client_get_notafter and
the _notbefore counterpart to a static function since they are copies of
eachother.

Yes agreed. I have made the adjustment attached as "v2-0001-sslinfo-add-notbefore-and-notafter-timestamps.patch"

would this feature be suitable to be added to commitfest? What do you think?

thank you

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

On 23 Jun 2023, at 22:10, Cary Huang <cary.huang@highgo.ca> wrote:

would this feature be suitable to be added to commitfest? What do you think?

Yes, please add it to the July commitfest and feel free to set me as Reviewer,
I intend to take a look at it.

--
Daniel Gustafsson

Yes, please add it to the July commitfest and feel free to set me as Reviewer,
I intend to take a look at it.

Thank you Daniel, I have added this patch to July commitfest under security category and added you as reviewer.

best regards

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

On 23 Jun 2023, at 22:10, Cary Huang <cary.huang@highgo.ca> wrote:

Off the cuff that doesn't seem like a bad idea, but I wonder if we should add
them to pg_stat_ssl (or both) instead if we deem them valuable?

I think the same information should be available to pg_stat_ssl as well. pg_stat_ssl can show the client certificate information for all connecting clients, having it to show not_before and not_after timestamps of every client are important in my opinion. The attached patch "v2-0002-pg-stat-ssl-add-notbefore-and-notafter-timestamps.patch" adds this support

This needs to adjust the tests in src/test/ssl which now fails due to SELECT *
returning a row which doesn't match what the test was coded for.

Re the patch, it would be nice to move the logic in ssl_client_get_notafter and
the _notbefore counterpart to a static function since they are copies of
eachother.

Yes agreed. I have made the adjustment attached as "v2-0001-sslinfo-add-notbefore-and-notafter-timestamps.patch"

The new patchset isn't updating contrib/sslinfo/meson with the 1.3 update so it
fails to build with Meson.

--
Daniel Gustafsson

This needs to adjust the tests in src/test/ssl which now fails due to SELECT *
returning a row which doesn't match what the test was coded for.

Thank you so much for pointing out. I have adjusted the extra ssl test to account for the extra columns returned. It should not fail now.

The new patchset isn't updating contrib/sslinfo/meson with the 1.3 update so it
fails to build with Meson.

Thanks again for pointing out, I have adjusted the meson build file to include the 1.3 update

Please see attached patches for the fixes.
Thank you so much!

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

On 30 Jun 2023, at 20:12, Cary Huang <cary.huang@highgo.ca> wrote:

This needs to adjust the tests in src/test/ssl which now fails due to SELECT *
returning a row which doesn't match what the test was coded for.

Thank you so much for pointing out. I have adjusted the extra ssl test to account for the extra columns returned. It should not fail now.

Thanks for the new version! It doesn't fail the ssl tests, but the Kerberos
test now fails. You can see the test reports from the CFBot here:

http://cfbot.cputube.org/cary-huang.html

This runs on submitted patches, you can also run the same CI checks in your own
Github clone using the supplied CI files in the postgres repo.

There are also some trivial whitespace issues shown with "git diff --check",
these can of course easily be addressed by a committer in a final-version patch
but when sending a new version you might as well fix those.

The new patchset isn't updating contrib/sslinfo/meson with the 1.3 update so it
fails to build with Meson.

Thanks again for pointing out, I have adjusted the meson build file to include the 1.3 update

+ asn1_notbefore = X509_getm_notBefore(cert);

X509_getm_notBefore() and X509_getm_notAfter() are only available in OpenSSL
1.1.1 and onwards, but postgres support 1.0.2 (as of today with 8e278b6576).
X509_get_notAfter() is available in 1.0.2 but deprecated in 1.1.1 and turned
into an alias for X509_getm_notAfter() (same with _notBefore of course), and
since we set 1.0.2 as the API compatibility we should be able to use that
without warnings instead.

+     <function>ssl_client_get_notbefore() returns text</function>
+     <function>ssl_client_get_notafter() returns text</function>

These functions should IMO return timestamp data types to save the user from
having to convert them. Same with the additions to pg_stat_get_activity.

You should add tests for the new functions in src/test/ssl/t/003_sslinfo.pl.

--
Daniel Gustafsson

Thanks for the new version! It doesn't fail the ssl tests, but the Kerberos
test now fails. You can see the test reports from the CFBot here:

Yes, kerberos tests failed due to the addition of notbefore and notafter values. The values array within "pg_stat_get_activity" function related to "pg_stat_gssapi" were not set correctly. It is now fixed

This runs on submitted patches, you can also run the same CI checks in your own
Github clone using the supplied CI files in the postgres repo.

Thank you for pointing this out. I followed the CI instruction as suggested and am able to run the same CI checks to reproduce the test failures.

There are also some trivial whitespace issues shown with "git diff --check",
these can of course easily be addressed by a committer in a final-version patch
but when sending a new version you might as well fix those.

Yes, the white spaces issues should be addressed in the attached patches.

X509_getm_notBefore() and X509_getm_notAfter() are only available in OpenSSL
1.1.1 and onwards, but postgres support 1.0.2 (as of today with 8e278b6576).
X509_get_notAfter() is available in 1.0.2 but deprecated in 1.1.1 and turned
into an alias for X509_getm_notAfter() (same with _notBefore of course), and
since we set 1.0.2 as the API compatibility we should be able to use that
without warnings instead.

Thank you so much for catching this openssl function compatibility issue. I have changed the function calls to:
- X509_get_notBefore()
- X509_get_notAfter()

which are compatible in OpenSSL v1.0.2 and also v1.1.1 where they will get translated to X509_getm_notBefore() and X509_getm_notAfter() respectively

These functions should IMO return timestamp data types to save the user from
having to convert them. Same with the additions to pg_stat_get_activity.

Yes, agreed, the attached patches have the output changed to timestamp datatype instead of text.

You should add tests for the new functions in src/test/ssl/t/003_sslinfo.pl.

Yes, agreed, I added 2 additional tests in src/test/ssl/t/003_sslinfo.pl to compare the notbefore and notafter outputs from sslinfo extension and pg_stat_ssl outputs. Both should be tested equal.

Also added related documentation about the new not before and not after timestamps in pg_stat_ssl.

thank you

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

I had another look at this today and I think this patch is in pretty good
shape, below are a few comments on this revision:

-  'sslinfo--1.2.sql',
+  'sslinfo--1.2--1.3.sql',
+  'sslinfo--1.3.sql',

The way we typically ship extensions in contrib/ is to not create a new base
version .sql file for smaller changes like adding a few functions. For this
patch we should keep --1.2.sql and instead supply a 1.2--1.3.sql with the new
functions.

+ <structfield>not_befoer</structfield> <type>text</type>

s/not_befoer/not_before/

+ errmsg("failed to convert tm to timestamp")));

I think this error is too obscure for the user to act on, what we use elsewhere
is "timestamp out of range" and I think thats more helpful. I do wonder if
there is ever a legitimate case when this can fail while still having a
authenticated client connection?

+ ^\d+,t,TLSv[\d.]+,[\w-]+,\d+,/?CN=ssltestuser,$serialno,/?\QCN=Test CA for PostgreSQL SSL regression test client certs\E,\Q2021-03-03 22:12:07\E,\Q2048-07-19 22:12:07\E\r?$}mx,

This test output won't actually work for testing against, it works now because
the dates match the current set of certificates, but the certificates can be
regenerated with `cd src/test/ssl && make -f sslfiles.mk` and that will change
the not_before/not_after dates. In order to have stable test data we need to
set fixed end/start dates and reissue all the client certs.

+	"SELECT ssl_client_get_notbefore() = not_before FROM pg_stat_ssl WHERE pid = pg_backend_pid();",
+	connstr => $common_connstr);
+is($result, 't', "ssl_client_get_notbefore() for not_before timestamp");

While this works, it will fail to catch if we have the same bug in both sslinfo
and the backend. With stable test data we can add the actual date in the mix
and verify that both timestamps are equal and that they match the expected
date.

I have addressed the issues above in a new v5 patchset which includes a new
patch for setting stable validity on the test certificates (the notBefore time
was arbitrarily chosen to match the date of opening up the tree for v17 - we
just need a date in the past). Your two patches are rolled into a single one
with a commit message added to get started on that part as well.

--
Daniel Gustafsson

Hello

The way we typically ship extensions in contrib/ is to not create a new base
version .sql file for smaller changes like adding a few functions. For this
patch we should keep --1.2.sql and instead supply a 1.2--1.3.sql with the new
functions.

Thank you for pointing this out. It makes sense to me.

+    errmsg("failed to convert tm to timestamp")));

I think this error is too obscure for the user to act on, what we use elsewhere
is "timestamp out of range" and I think thats more helpful. I do wonder if
there is ever a legitimate case when this can fail while still having a
authenticated client connection?

My bad here, you are right. "timestamp out of range" is a much better error message. However, in an authenticated client connection, there should not be a legitimate case where the not before and not after can fall out of range. The "not before" and "not after" timestamps in a X509 certificate are normally represented by ASN1, which has maximum timestamp of December 31, 9999. The timestamp data structure in PostgreSQL on the other hand can support year up to June 3, 5874898. Assuming the X509 certificate is generated correctly and no data corruptions happening (which is unlikely), the conversion from ASN1 to timestamp shall not result in out of range error.

Perhaps calling "tm2timestamp(&pgtm_time, 0, NULL, &ts)" without checking the return code would be just fine. I see some other usages of tm2timstamp() in other code areas also skip checking the return code.

I have addressed the issues above in a new v5 patchset which includes a new
patch for setting stable validity on the test certificates (the notBefore time
was arbitrarily chosen to match the date of opening up the tree for v17 - we
just need a date in the past). Your two patches are rolled into a single one
with a commit message added to get started on that part as well.

thank you so much for addressing the ssl tests to make "not before" and "not after" timestamps static in the test certificate and also adjusting 003_sslinfo.pl to expect the new static timestamps in the v5 patches. I am able to apply both and all tests are passing. I did not know this test certificate could be changed by `cd src/test/ssl && make -f sslfiles.mk`, but now I know, thanks to you :p.

Best regards

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

On 14 Jul 2023, at 20:41, Cary Huang <cary.huang@highgo.ca> wrote:

Perhaps calling "tm2timestamp(&pgtm_time, 0, NULL, &ts)" without checking the return code would be just fine. I see some other usages of tm2timstamp() in other code areas also skip checking the return code.

I think we want to know about any failures, btu we can probably make it into an
elog() instead, as it should never fail.

--
Daniel Gustafsson

Hello

Perhaps calling "tm2timestamp(&pgtm_time, 0, NULL, &ts)" without checking the return code would be just fine. I see some other usages of tm2timstamp() in other code areas also skip checking the return code.

I think we want to know about any failures, btu we can probably make it into an
elog() instead, as it should never fail.

Yes, sure. I have corrected the error message to elog(ERROR, "timestamp out of range") on a rare tm2timestamp() failure. Please see the attached patch based on your v5. "v6-0001-Set-fixed-dates-for-test-certificates-validity.patch" is exactly the same as "v5-0001-Set-fixed-dates-for-test-certificates-validity.patch", I just up the version to be consistent.

thank you very much

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

On 17 Jul 2023, at 20:26, Cary Huang <cary.huang@highgo.ca> wrote:

Perhaps calling "tm2timestamp(&pgtm_time, 0, NULL, &ts)" without checking the return code would be just fine. I see some other usages of tm2timstamp() in other code areas also skip checking the return code.

I think we want to know about any failures, btu we can probably make it into an
elog() instead, as it should never fail.

Yes, sure. I have corrected the error message to elog(ERROR, "timestamp out of range") on a rare tm2timestamp() failure.

I went over this again and ended up pushing it along with a catversion bump.
Due to a mistake in my testing I didn't however catch that it was using an API
only present in OpenSSL 1.1.1 and higher, which caused buildfailures when using
older OpenSSL versions, so I ended up reverting it again (leaving certificate
changes in place) to keep the buildfarm green.

Will look closer at an implementation which works across all supported versions
of OpenSSL when I have more time.

--
Daniel Gustafsson

On 20 Jul 2023, at 17:24, Daniel Gustafsson <daniel@yesql.se> wrote:

On 17 Jul 2023, at 20:26, Cary Huang <cary.huang@highgo.ca> wrote:

Perhaps calling "tm2timestamp(&pgtm_time, 0, NULL, &ts)" without checking the return code would be just fine. I see some other usages of tm2timstamp() in other code areas also skip checking the return code.

I think we want to know about any failures, btu we can probably make it into an
elog() instead, as it should never fail.

Yes, sure. I have corrected the error message to elog(ERROR, "timestamp out of range") on a rare tm2timestamp() failure.

I went over this again and ended up pushing it along with a catversion bump.
Due to a mistake in my testing I didn't however catch that it was using an API
only present in OpenSSL 1.1.1 and higher, which caused buildfailures when using
older OpenSSL versions, so I ended up reverting it again (leaving certificate
changes in place) to keep the buildfarm green.

Will look closer at an implementation which works across all supported versions
of OpenSSL when I have more time.

Finally had some time, and have made an updated version of the patch.

OpenSSL 1.0.2 doens't expose a function for getting the timestamp, so the patch
instead resorts to the older trick of getting the timestamp by inspecing the
diff against the UNIX epoch. When doing this, OpenSSL internally use the same
function which later in 1.1.1 was exported for getting the timestamp.

The attached version passes ssl tests for me on 1.0.2 through OpenSSL Git HEAD.

--
Daniel Gustafsson

Hello,

On 7/25/23 07:21, Daniel Gustafsson wrote:

The attached version passes ssl tests for me on 1.0.2 through OpenSSL Git HEAD.

Tests pass for me too, including LibreSSL 3.8.

+ /* Calculate the diff from the epoch to the certificat timestamp */

"certificate"

+ <function>ssl_client_get_notbefore() returns text</function>
...> + <function>ssl_client_get_notafter() returns text</function>

I think this should say timestamptz rather than text? Ditto for the
pg_stat_ssl documentation.

Speaking of which: is the use of `timestamp` rather than `timestamptz`
in pg_proc.dat intentional? Will that cause problems with comparisons?

--

I haven't been able to poke any holes in the ASN1_TIME_to_timestamp()
implementations themselves. I went down a rabbit hole trying to find out
whether leap seconds could cause problems for us when we switch to
`struct tm` in the future, but it turns out OpenSSL rejects leap seconds
in the Validity fields. That seems weird -- as far as I can tell, RFC
5280 defers to ASN.1 which defers to ISO 8601 which appears to allow
leap seconds -- but I don't plan to worry about it anymore. (I do idly
wonder whether some CA, somewhere, has ever had a really Unhappy New
Year due to that.)

Thanks,
--Jacob

On 12 Sep 2023, at 21:40, Jacob Champion <jchampion@timescale.com> wrote:

Hello,

On 7/25/23 07:21, Daniel Gustafsson wrote:

The attached version passes ssl tests for me on 1.0.2 through OpenSSL Git HEAD.

Tests pass for me too, including LibreSSL 3.8.

Thanks for testing!

+ /* Calculate the diff from the epoch to the certificat timestamp */

"certificate"

Fixed.

+ <function>ssl_client_get_notbefore() returns text</function>
...> + <function>ssl_client_get_notafter() returns text</function>

I think this should say timestamptz rather than text? Ditto for the
pg_stat_ssl documentation.

Speaking of which: is the use of `timestamp` rather than `timestamptz`
in pg_proc.dat intentional? Will that cause problems with comparisons?

It should be timestamptz, it was a tyop on my part. Fixed.

I haven't been able to poke any holes in the ASN1_TIME_to_timestamp()
implementations themselves. I went down a rabbit hole trying to find out
whether leap seconds could cause problems for us when we switch to
`struct tm` in the future, but it turns out OpenSSL rejects leap seconds
in the Validity fields. That seems weird -- as far as I can tell, RFC
5280 defers to ASN.1 which defers to ISO 8601 which appears to allow
leap seconds -- but I don't plan to worry about it anymore. (I do idly
wonder whether some CA, somewhere, has ever had a really Unhappy New
Year due to that.)

That's an interesting thought, maybe the CA's have adapted given the
marketshare of OpenSSL?

Thanks for reviewing, the attached v8 contains the fixes from this review along
with a fresh rebase and some attempts at making tests more stable in the face
of timezones by casting to date.

--
Daniel Gustafsson

On Mon, Mar 4, 2024 at 6:23 AM Daniel Gustafsson <daniel@yesql.se> wrote:

On 12 Sep 2023, at 21:40, Jacob Champion <jchampion@timescale.com> wrote:

Sorry for the long delay!

+ <function>ssl_client_get_notbefore() returns text</function>
...> + <function>ssl_client_get_notafter() returns text</function>

I think this should say timestamptz rather than text? Ditto for the
pg_stat_ssl documentation.

Speaking of which: is the use of `timestamp` rather than `timestamptz`
in pg_proc.dat intentional? Will that cause problems with comparisons?

It should be timestamptz, it was a tyop on my part. Fixed.

Looks like sslinfo--1.2--1.3.sql is also declaring the functions as
timestamp rather than timestamptz, which is breaking comparisons with
the not_before/after columns. It might also be nice to rename
ASN1_TIME_to_timestamp().

Squinting further at the server backend implementation, should that
also be using TimestampTz throughout, instead of Timestamp? It all
goes through float8_timestamptz at the end, so I guess it shouldn't
have a material impact, but it's a bit confusing.

Thanks for reviewing, the attached v8 contains the fixes from this review along
with a fresh rebase and some attempts at making tests more stable in the face
of timezones by casting to date.

In my -08 timezone, the date doesn't match what's recorded either
(it's my "tomorrow"). I think those probably just need to be converted
to UTC explicitly? I've attached a sample diff on top of v8 that
passes tests on my machine.

--Jacob

Hello

Thank you for the review and your patch. I have tested with minimum OpenSSL version 1.0.2 support and incorporated your changes into the v9 patch as attached.

In my -08 timezone, the date doesn't match what's recorded either
(it's my "tomorrow"). I think those probably just need to be converted
to UTC explicitly? I've attached a sample diff on top of v8 that
passes tests on my machine.

Yes, I noticed this in the SSL test too. I am also in GTM-8, so for me the tests would fail too due to the time zone differences from GMT. It shall be okay to specifically set the outputs of pg_stat_ssl, ssl_client_get_notbefore, and ssl_client_get_notafte to be in GMT time zone. The not before and not after time stamps in a client certificate are generally expressed in GMT.

Thank you!

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

On Fri, Mar 8, 2024 at 4:16 PM Cary Huang <cary.huang@highgo.ca> wrote:

Yes, I noticed this in the SSL test too. I am also in GTM-8, so for me the tests would fail too due to the time zone differences from GMT. It shall be okay to specifically set the outputs of pg_stat_ssl, ssl_client_get_notbefore, and ssl_client_get_notafte to be in GMT time zone. The not before and not after time stamps in a client certificate are generally expressed in GMT.

Hi Cary, did you have any thoughts on the timestamptz notes from my last mail?

It might also be nice to rename
ASN1_TIME_to_timestamp().

Squinting further at the server backend implementation, should that
also be using TimestampTz throughout, instead of Timestamp? It all
goes through float8_timestamptz at the end, so I guess it shouldn't
have a material impact, but it's a bit confusing.

Thanks,
--Jacob