Thread: 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
notaftertimestamps which are also quite important attributes in a X509 certificate. The attached patch adds 2 functions
toget 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
informationfor all connecting clients, having it to show not_before and not_after timestamps of every client are
importantin 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
informationfor all connecting clients, having it to show not_before and not_after timestamps of every client are
importantin 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
shouldnot 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
shouldnot 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
reproducethe 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
fromsslinfo 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-0322: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
clientconnection, there should not be a legitimate case where the not before and not after can fall out of range. The
"notbefore" and "not after" timestamps in a X509 certificate are normally represented by ASN1, which has maximum
timestampof 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),
theconversion 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
someother 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
certificateand also adjusting 003_sslinfo.pl to expect the new static timestamps in the v5 patches. I am able to apply
bothand 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
someother 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
seesome 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"
isexactly 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
someother 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
seesome 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
changesinto 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
differencesfrom GMT. It shall be okay to specifically set the outputs of pg_stat_ssl, ssl_client_get_notbefore, and
ssl_client_get_notafteto be in GMT time zone. The not before and not after time stamps in a client certificate are
generallyexpressed 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
differencesfrom GMT. It shall be okay to specifically set the outputs of pg_stat_ssl, ssl_client_get_notbefore, and
ssl_client_get_notafteto be in GMT time zone. The not before and not after time stamps in a client certificate are
generallyexpressed 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

Hi Jacob

> 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.

Sorry I kind of missed this review comment from your last email. Thanks for bringing it up again though. I think it is
rightto change the backend references of "timestamp" to "timestampTz" for consistency reasons. I have gone ahead to
makethe changes.
 

I have also reviewed the wording on the documentation and removed "UTC" from the descriptions. Since sslinfo extension
andpg_stat_ssl both return timestampTz in whatever timezone PostgreSQL is running on, they do not always return UTC
timestamps.

Attached is the v10 patch with the above changes. Thanks again for the review.

Best regards

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

On Mon, Mar 18, 2024 at 1:48 PM Cary Huang <cary.huang@highgo.ca> wrote:
> Attached is the v10 patch with the above changes. Thanks again for the review.

Awesome, looks good.

On my final review pass I saw one last thing that bothered me (sorry
for not seeing it before). The backend version of
ASN1_TIME_to_timestamptz returns the following:

> +   return ((int64)days * 24 * 60 * 60) + (int64)seconds;

...but I think Timestamp[Tz]s are stored as microseconds, so we're off
by a factor of a million. This still works because later we cast to
double and pass it back through float8_timestamptz, which converts it:

> +               if (beentry->st_sslstatus->ssl_not_before != 0)
> +                   values[25] = DirectFunctionCall1(float8_timestamptz,
> +                                                    Float8GetDatum((double) beentry->st_sslstatus->ssl_not_before));

But anyone who ends up inspecting the value of
st_sslstatus->ssl_not_before directly is going to find an incorrect
timestamp. I think it'd be clearer to store microseconds to begin
with, and then just use TimestampTzGetDatum rather than the
DirectFunctionCall1 we have now. (I looked for an existing
implementation to reuse and didn't find one. Maybe we should use the
overflow-aware multiplication/addition routines -- i.e.
pg_mul_s64_overflow et al -- to multiply `days` and `seconds` by
USECS_PER_DAY/USECS_PER_SEC and combine them.)

And I think sslinfo can remain as-is, because that way overflow is
caught by float8_timestamptz. WDYT?

--Jacob

Thank you for your review again. 

 > ...but I think Timestamp[Tz]s are stored as microseconds, so we're off
 > by a factor of a million. This still works because later we cast to
 > double and pass it back through float8_timestamptz, which converts it:

In my test, if I made ASN1_TIME_to_timestamptz to return in microseconds, the
float8_timestamptz() function will catch a "timestamp out of range" exception as
this function treats the input as seconds.

> But anyone who ends up inspecting the value of
 > st_sslstatus->ssl_not_before directly is going to find an incorrect
 > timestamp. I think it'd be clearer to store microseconds to begin
 > with, and then just use TimestampTzGetDatum rather than the
 > DirectFunctionCall1 we have now. 

I have also tried TimestampTzGetDatum with ASN1_TIME_to_timestamptz 
outputting in microseconds. No exception is caught, but pg_stat_ssl displays 
incorrect results. The timestamps are extra large because the extra factor of 
1 million is considered in the timestamp computation as well.

The comments for TimestampTz says:

 * Timestamps, as well as the h/m/s fields of intervals, are stored as
 * int64 values with units of microseconds.  (Once upon a time they were
 * double values with units of seconds.)

but it seems to me that many of the timestamp related functions still consider
timestamp or timestampTz as "double values with units of seconds" though. 

Best regards

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

> On 20 Mar 2024, at 00:24, Cary Huang <cary.huang@highgo.ca> wrote:

> but it seems to me that many of the timestamp related functions still consider
> timestamp or timestampTz as "double values with units of seconds" though.

The issue here is that postgres use a different epoch from the unix epoch, so
any dates calcuated based on the unix epoch need to be adjusted.  I've hacked
this up in the attached v11 using overflow-safe integer mul/add as proposed by
Jacob upthread (we really shouldn't risk overflowing an int64 here but there is
no harm in using belts and suspenders here as a defensive measure).

The attached v11 is what I propose we go ahead with unless there further
comments on this.

--
Daniel Gustafsson

On Wed, Mar 20, 2024 at 7:03 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> The issue here is that postgres use a different epoch from the unix epoch, so
> any dates calcuated based on the unix epoch need to be adjusted.

Ah, thank you! I had just reproduced Cary's problem and was really confused...

> I've hacked
> this up in the attached v11 using overflow-safe integer mul/add as proposed by
> Jacob upthread (we really shouldn't risk overflowing an int64 here but there is
> no harm in using belts and suspenders here as a defensive measure).
>
> The attached v11 is what I propose we go ahead with unless there further
> comments on this.

One last question:

> +   result -= ((POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * USECS_PER_DAY);
> +   return TimestampTzGetDatum(result);

Is that final bare subtraction able to wrap around for dates far in the past?

--Jacob

> On 20 Mar 2024, at 15:28, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

>> +   result -= ((POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * USECS_PER_DAY);
>> +   return TimestampTzGetDatum(result);
>
> Is that final bare subtraction able to wrap around for dates far in the past?

We are subtracting 30 years from a calculation that we know didnt overflow, so
I guess if the certificate notBefore (the notAfter cannot be that early since
we wouldn't be able to connect with it) was set to early enough?  It didn't
strike me as anything above academical unless I'm thinking wrong here.

--
Daniel Gustafsson

On Wed, Mar 20, 2024 at 7:50 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> We are subtracting 30 years from a calculation that we know didnt overflow, so
> I guess if the certificate notBefore (the notAfter cannot be that early since
> we wouldn't be able to connect with it) was set to early enough?  It didn't
> strike me as anything above academical unless I'm thinking wrong here.

Yeah, it's super nitpicky. The CA would have had to sign a really
broken certificate somehow, anyway...

I can't find anything else to note; patch LGTM.

Thanks,
--Jacob

> On 20 Mar 2024, at 17:32, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> I can't find anything else to note; patch LGTM.

While staging this to commit I realized one silly thing about it warranting
another round here.  The ASN.1 timediff code can diff against *any* timestamp,
not just the UNIX epoch, so we could just pass in the postgres epoch and skip
the final subtraction since we're already correctly adjusted.  This removes the
non-overflow checked arithmetic with a simpler logic.

--
Daniel Gustafsson

On Fri, Mar 22, 2024 at 6:15 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> While staging this to commit I realized one silly thing about it warranting
> another round here.  The ASN.1 timediff code can diff against *any* timestamp,
> not just the UNIX epoch, so we could just pass in the postgres epoch and skip
> the final subtraction since we're already correctly adjusted.  This removes the
> non-overflow checked arithmetic with a simpler logic.

Ah, that's much better! +1.

--Jacob

 > > The recent bump in minmum required versions of OpenSSL and LibreSSL made me
 > > remember to revisit this patch which was previously reverted due to library
 > > incompatibility (with *both* OpenSSL and LibreSSL on different APIs).
 > > 
 > > The attached removes the timestamp conversion workaround which is no longer
 > > needed.
 > 
 > The patch was marked as ready for committer and is currently failing
 > in the CI.  I've moved it to next CF waiting on author.  Could you
 > provide a rebase?

Since the minimum OpenSSL version is now 1.1.1, the v13 patch would fail the CI because
it uses the old APIs to obtain notBefore and notAfter timestamps:

- X509_get_notBefore
- X509_get_notAfter

which where deprecated in OpenSSL 1.1.0...
Instead, it should use:

- X509_get0_notBefore
- X509_get0_notAfter

which are available in version 1.1.1 and beyond. These APIs now return a "const ASN1_TIME*"
instead of "ASN1_TIME*".

The changes below should remove the CI failing when applied to v13 patch:

---   a/contrib/sslinfo/sslinfo.c
+++ b/contrib/sslinfo/sslinfo.c

-static Datum ASN1_TIME_to_timestamptz(ASN1_TIME *time);
+static Datum ASN1_TIME_to_timestamptz(const ASN1_TIME *time);

-ASN1_TIME_to_timestamptz(ASN1_TIME *ASN1_cert_ts)
+ASN1_TIME_to_timestamptz(const ASN1_TIME *ASN1_cert_ts)

-       return ASN1_TIME_to_timestamptz(X509_get_notBefore(cert));
+      return ASN1_TIME_to_timestamptz(X509_get0_notBefore(cert));

-       return ASN1_TIME_to_timestamptz(X509_get_notAfter(cert));
+      return ASN1_TIME_to_timestamptz(X509_get0_notAfter(cert));


---   a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c

-static TimestampTz ASN1_TIME_to_timestamptz(ASN1_TIME *time);
+static TimestampTz ASN1_TIME_to_timestamptz(const ASN1_TIME *time);

-ASN1_TIME_to_timestamptz(ASN1_TIME *ASN1_cert_ts)
+ASN1_TIME_to_timestamptz(const ASN1_TIME *ASN1_cert_ts)

-    *ptr = ASN1_TIME_to_timestamptz(X509_get_notBefore(port->peer));
+    *ptr = ASN1_TIME_to_timestamptz(X509_get0_notBefore(port->peer));

-      *ptr = ASN1_TIME_to_timestamptz(X509_get_notAfter(port->peer));
+     *ptr = ASN1_TIME_to_timestamptz(X509_get0_notAfter(port->peer));

can you make a rebase with the above changes?


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