contrib/sslinfo cleanup and OpenSSL errorhandling
While hacking on the NSS patch I realized that sslinfo was passing the ->ssl
Port member directly to OpenSSL in order to extract information regarding the
connection. This breaks the API provided by the backend, as well as duplicates
code for no real benefit. The attached 0001 patch rewrites sslinfo to use the
be_tls_* API where possible to reduce duplication and keep the codebase TLS
dependency (mostly) tucked away behind a nice API. 0001 also contains a small
sslinfo doc update to cover that TLSv1.3 is a supported protocol.
0002 ports OpenSSL errorhandling introduced in d94c36a45ab which was performed
for sslinfo but not the backend. I agree with the commit message that the risk
is small (but not non-existing), but if the checks were important enough for
sslinfo I'd argue they make sense for the backend too.
This patchset was pulled from the NSS patch, but it is entirely independent
from NSS.
cheers ./daniel
Attachments:
0002-Improve-error-handling-in-backend-OpenSSL-implementa.patchapplication/octet-stream; name=0002-Improve-error-handling-in-backend-OpenSSL-implementa.patch; x-unix-mode=0644Download
From 0ba1431b39d5496f5a711cf7af2ad5e534a0f341 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 29 Oct 2020 14:30:29 +0100
Subject: [PATCH 2/2] Improve error handling in backend OpenSSL implementation
Commit d94c36a45ab introduced error handling to sslinfo to handle
OpenSSL errors gracefully. This ports this errorhandling to the
backend TLS implementation.
---
src/backend/libpq/be-secure-openssl.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 8b21ff4065..9231a1470c 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -1298,15 +1298,28 @@ X509_NAME_to_cstring(X509_NAME *name)
char *dp;
char *result;
+ if (membuf == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("failed to create BIO")));
+
(void) BIO_set_close(membuf, BIO_CLOSE);
for (i = 0; i < count; i++)
{
e = X509_NAME_get_entry(name, i);
nid = OBJ_obj2nid(X509_NAME_ENTRY_get_object(e));
+ if (nid == NID_undef)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not get NID for ASN1_OBJECT object")));
v = X509_NAME_ENTRY_get_data(e);
field_name = OBJ_nid2sn(nid);
- if (!field_name)
+ if (field_name == NULL)
field_name = OBJ_nid2ln(nid);
+ if (field_name == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not convert NID %d to an ASN1_OBJECT structure", nid)));
BIO_printf(membuf, "/%s=", field_name);
ASN1_STRING_print_ex(membuf, v,
((ASN1_STRFLGS_RFC2253 & ~ASN1_STRFLGS_ESC_MSB)
@@ -1322,7 +1335,8 @@ X509_NAME_to_cstring(X509_NAME *name)
result = pstrdup(dp);
if (dp != sp)
pfree(dp);
- BIO_free(membuf);
+ if (BIO_free(membuf) != 1)
+ elog(ERROR, "could not free OpenSSL BIO structure");
return result;
}
--
2.21.1 (Apple Git-122.3)
0001-Use-be_tls_-API-for-SSL-information-in-sslinfo.patchapplication/octet-stream; name=0001-Use-be_tls_-API-for-SSL-information-in-sslinfo.patch; x-unix-mode=0644Download
From 5ff486d9318b340ba06d10dcc419f012b87c368a Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 29 Oct 2020 14:25:37 +0100
Subject: [PATCH 1/2] Use be_tls_* API for SSL information in sslinfo
sslinfo was passing the Port->ssl member directly to OpenSSL in order
to extract information regarding the connection. This breaks the API
provided by the backend TLS implementation, as well as duplicates code
for no benefit. Rewrite to make use of the backend API as much as
possible.
---
contrib/sslinfo/sslinfo.c | 139 +++++++++++++-------------------------
doc/src/sgml/sslinfo.sgml | 2 +-
2 files changed, 48 insertions(+), 93 deletions(-)
diff --git a/contrib/sslinfo/sslinfo.c b/contrib/sslinfo/sslinfo.c
index 5ba3988e27..30cae0bb98 100644
--- a/contrib/sslinfo/sslinfo.c
+++ b/contrib/sslinfo/sslinfo.c
@@ -22,7 +22,6 @@
PG_MODULE_MAGIC;
static Datum X509_NAME_field_to_text(X509_NAME *name, text *fieldName);
-static Datum X509_NAME_to_text(X509_NAME *name);
static Datum ASN1_STRING_to_text(ASN1_STRING *str);
/*
@@ -54,9 +53,16 @@ PG_FUNCTION_INFO_V1(ssl_version);
Datum
ssl_version(PG_FUNCTION_ARGS)
{
- if (MyProcPort->ssl == NULL)
+ const char *version;
+
+ if (!MyProcPort->ssl_in_use)
+ PG_RETURN_NULL();
+
+ version = be_tls_get_version(MyProcPort);
+ if (version == NULL)
PG_RETURN_NULL();
- PG_RETURN_TEXT_P(cstring_to_text(SSL_get_version(MyProcPort->ssl)));
+
+ PG_RETURN_TEXT_P(cstring_to_text(version));
}
@@ -67,9 +73,16 @@ PG_FUNCTION_INFO_V1(ssl_cipher);
Datum
ssl_cipher(PG_FUNCTION_ARGS)
{
- if (MyProcPort->ssl == NULL)
+ const char *cipher;
+
+ if (!MyProcPort->ssl_in_use)
+ PG_RETURN_NULL();
+
+ cipher = be_tls_get_cipher(MyProcPort);
+ if (cipher == NULL)
PG_RETURN_NULL();
- PG_RETURN_TEXT_P(cstring_to_text(SSL_get_cipher(MyProcPort->ssl)));
+
+ PG_RETURN_TEXT_P(cstring_to_text(cipher));
}
@@ -83,7 +96,7 @@ PG_FUNCTION_INFO_V1(ssl_client_cert_present);
Datum
ssl_client_cert_present(PG_FUNCTION_ARGS)
{
- PG_RETURN_BOOL(MyProcPort->peer != NULL);
+ PG_RETURN_BOOL(MyProcPort->peer_cert_valid);
}
@@ -99,25 +112,21 @@ PG_FUNCTION_INFO_V1(ssl_client_serial);
Datum
ssl_client_serial(PG_FUNCTION_ARGS)
{
+ char decimal[NAMEDATALEN];
Datum result;
- Port *port = MyProcPort;
- X509 *peer = port->peer;
- ASN1_INTEGER *serial = NULL;
- BIGNUM *b;
- char *decimal;
- if (!peer)
+ if (!MyProcPort->ssl_in_use || !MyProcPort->peer_cert_valid)
+ PG_RETURN_NULL();
+
+ be_tls_get_peer_serial(MyProcPort, decimal, NAMEDATALEN);
+
+ if (!*decimal)
PG_RETURN_NULL();
- serial = X509_get_serialNumber(peer);
- b = ASN1_INTEGER_to_BN(serial, NULL);
- decimal = BN_bn2dec(b);
- BN_free(b);
result = DirectFunctionCall3(numeric_in,
CStringGetDatum(decimal),
ObjectIdGetDatum(0),
Int32GetDatum(-1));
- OPENSSL_free(decimal);
return result;
}
@@ -228,7 +237,7 @@ ssl_client_dn_field(PG_FUNCTION_ARGS)
text *fieldname = PG_GETARG_TEXT_PP(0);
Datum result;
- if (!(MyProcPort->peer))
+ if (!MyProcPort->ssl_in_use || !MyProcPort->peer_cert_valid)
PG_RETURN_NULL();
result = X509_NAME_field_to_text(X509_get_subject_name(MyProcPort->peer), fieldname);
@@ -275,76 +284,6 @@ ssl_issuer_field(PG_FUNCTION_ARGS)
}
-/*
- * Equivalent of X509_NAME_oneline that respects encoding
- *
- * This function converts X509_NAME structure to the text variable
- * converting all textual data into current database encoding.
- *
- * Parameter: X509_NAME *name X509_NAME structure to be converted
- *
- * Returns: text datum which contains string representation of
- * X509_NAME
- */
-static Datum
-X509_NAME_to_text(X509_NAME *name)
-{
- BIO *membuf = BIO_new(BIO_s_mem());
- int i,
- nid,
- count = X509_NAME_entry_count(name);
- X509_NAME_ENTRY *e;
- ASN1_STRING *v;
- const char *field_name;
- size_t size;
- char nullterm;
- char *sp;
- char *dp;
- text *result;
-
- if (membuf == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("could not create OpenSSL BIO structure")));
-
- (void) BIO_set_close(membuf, BIO_CLOSE);
- for (i = 0; i < count; i++)
- {
- e = X509_NAME_get_entry(name, i);
- nid = OBJ_obj2nid(X509_NAME_ENTRY_get_object(e));
- if (nid == NID_undef)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("could not get NID for ASN1_OBJECT object")));
- v = X509_NAME_ENTRY_get_data(e);
- field_name = OBJ_nid2sn(nid);
- if (field_name == NULL)
- field_name = OBJ_nid2ln(nid);
- if (field_name == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("could not convert NID %d to an ASN1_OBJECT structure", nid)));
- BIO_printf(membuf, "/%s=", field_name);
- ASN1_STRING_print_ex(membuf, v,
- ((ASN1_STRFLGS_RFC2253 & ~ASN1_STRFLGS_ESC_MSB)
- | ASN1_STRFLGS_UTF8_CONVERT));
- }
-
- /* ensure null termination of the BIO's content */
- nullterm = '\0';
- BIO_write(membuf, &nullterm, 1);
- size = BIO_get_mem_data(membuf, &sp);
- dp = pg_any_to_server(sp, size - 1, PG_UTF8);
- result = cstring_to_text(dp);
- if (dp != sp)
- pfree(dp);
- if (BIO_free(membuf) != 1)
- elog(ERROR, "could not free OpenSSL BIO structure");
-
- PG_RETURN_TEXT_P(result);
-}
-
-
/*
* Returns current client certificate subject as one string
*
@@ -358,9 +297,17 @@ PG_FUNCTION_INFO_V1(ssl_client_dn);
Datum
ssl_client_dn(PG_FUNCTION_ARGS)
{
- if (!(MyProcPort->peer))
+ char subject[NAMEDATALEN];
+
+ if (!MyProcPort->ssl_in_use || !MyProcPort->peer_cert_valid)
+ PG_RETURN_NULL();
+
+ be_tls_get_peer_subject_name(MyProcPort, subject, NAMEDATALEN);
+
+ if (!*subject)
PG_RETURN_NULL();
- return X509_NAME_to_text(X509_get_subject_name(MyProcPort->peer));
+
+ PG_RETURN_TEXT_P(cstring_to_text(subject));
}
@@ -377,9 +324,17 @@ PG_FUNCTION_INFO_V1(ssl_issuer_dn);
Datum
ssl_issuer_dn(PG_FUNCTION_ARGS)
{
- if (!(MyProcPort->peer))
+ char issuer[NAMEDATALEN];
+
+ if (!MyProcPort->ssl_in_use || !MyProcPort->peer_cert_valid)
PG_RETURN_NULL();
- return X509_NAME_to_text(X509_get_issuer_name(MyProcPort->peer));
+
+ be_tls_get_peer_issuer_name(MyProcPort, issuer, NAMEDATALEN);
+
+ if (!*issuer)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(cstring_to_text(issuer));
}
diff --git a/doc/src/sgml/sslinfo.sgml b/doc/src/sgml/sslinfo.sgml
index e16f61b41d..96da7621b7 100644
--- a/doc/src/sgml/sslinfo.sgml
+++ b/doc/src/sgml/sslinfo.sgml
@@ -54,7 +54,7 @@
<listitem>
<para>
Returns the name of the protocol used for the SSL connection (e.g., TLSv1.0
- TLSv1.1, or TLSv1.2).
+ TLSv1.1, TLSv1.2 or TLSv1.3).
</para>
</listitem>
</varlistentry>
--
2.21.1 (Apple Git-122.3)
Hi,
Thanks for extracting these.
On 2020-10-29 23:48:57 +0100, Daniel Gustafsson wrote:>
/* @@ -54,9 +53,16 @@ PG_FUNCTION_INFO_V1(ssl_version); Datum ssl_version(PG_FUNCTION_ARGS) { - if (MyProcPort->ssl == NULL) + const char *version; + + if (!MyProcPort->ssl_in_use) + PG_RETURN_NULL(); + + version = be_tls_get_version(MyProcPort); + if (version == NULL) PG_RETURN_NULL(); - PG_RETURN_TEXT_P(cstring_to_text(SSL_get_version(MyProcPort->ssl))); + + PG_RETURN_TEXT_P(cstring_to_text(version)); }
There's quite a few copies of this code that look exactly the same,
except for the be_tls_get_* call. Do you see a way to have fewer copies
of the same code?
Greetings,
Andres Freund
On Thu, Oct 29, 2020 at 04:40:32PM -0700, Andres Freund wrote:
There's quite a few copies of this code that look exactly the same,
except for the be_tls_get_* call. Do you see a way to have fewer copies
of the same code?
Each one of those code paths is working on a different sub-API aiming
at fetching a specific piece of TLS information, and the way each
sub-API does its lookup at MyProcPort is different. One possible way
would be to move the checks on ssl_in_use into a macro for the
beginning part. The end part could be improved by making
X509_NAME_field_to_text() and such return a text and not a Datum, and
move the null check + text-to-datum conversion into a separate macro.
I am not sure if this would be an improvement in terms of readability
though.
--
Michael
On 30 Oct 2020, at 00:40, Andres Freund <andres@anarazel.de> wrote:
There's quite a few copies of this code that look exactly the same,
except for the be_tls_get_* call. Do you see a way to have fewer copies
of the same code?
There's really only two of the same, and two sets of those. I tried some
variations but didn't really achieve anything that would strike the right
balance on the codegolf-to-readability scale. Maybe others have a richer
imagination than me.
cheers ./daniel
On Fri, Oct 30, 2020 at 11:20 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 30 Oct 2020, at 00:40, Andres Freund <andres@anarazel.de> wrote:
There's quite a few copies of this code that look exactly the same,
except for the be_tls_get_* call. Do you see a way to have fewer copies
of the same code?There's really only two of the same, and two sets of those. I tried some
variations but didn't really achieve anything that would strike the right
balance on the codegolf-to-readability scale. Maybe others have a richer
imagination than me.
Yeah, since it's only 2 of each, moving it to a macro wouldn't really
save a lot -- and it would make things less readable overall I think.
So I'd say the current version is OK.
One thing I noted was in the docs part of the patch there is a missing
comma -- but that one is missing previously as well. I'll go apply
that fix to the back branches while waiting to see if somebody comes
up with a more creative way to avoid the repeated code :)
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Mon, Nov 2, 2020 at 3:19 PM Magnus Hagander <magnus@hagander.net> wrote:
On Fri, Oct 30, 2020 at 11:20 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 30 Oct 2020, at 00:40, Andres Freund <andres@anarazel.de> wrote:
There's quite a few copies of this code that look exactly the same,
except for the be_tls_get_* call. Do you see a way to have fewer copies
of the same code?There's really only two of the same, and two sets of those. I tried some
variations but didn't really achieve anything that would strike the right
balance on the codegolf-to-readability scale. Maybe others have a richer
imagination than me.Yeah, since it's only 2 of each, moving it to a macro wouldn't really
save a lot -- and it would make things less readable overall I think.So I'd say the current version is OK.
One thing I noted was in the docs part of the patch there is a missing
comma -- but that one is missing previously as well. I'll go apply
that fix to the back branches while waiting to see if somebody comes
up with a more creative way to avoid the repeated code :)
Applied, with the small adjustment of the comma in the docs.
I wonder if we should perhaps backpatch 0002? The changes to sslinfo
that were ported go all the way back to 9.6, so it should be a safe
one I think?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On 3 Nov 2020, at 10:05, Magnus Hagander <magnus@hagander.net> wrote:
Applied, with the small adjustment of the comma in the docs.
Thanks!
I wonder if we should perhaps backpatch 0002? The changes to sslinfo
that were ported go all the way back to 9.6, so it should be a safe
one I think?
It should be safe given that the code has been in production for a long time.
That being said, sslinfo doesn't have tests (yet) and probably isn't used that
much; perhaps it's best to let this mature in HEAD for a few buildfarm cycles
first?
cheers ./daniel
On Tue, Nov 3, 2020 at 10:22 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 3 Nov 2020, at 10:05, Magnus Hagander <magnus@hagander.net> wrote:
Applied, with the small adjustment of the comma in the docs.
Thanks!
I wonder if we should perhaps backpatch 0002? The changes to sslinfo
that were ported go all the way back to 9.6, so it should be a safe
one I think?It should be safe given that the code has been in production for a long time.
That being said, sslinfo doesn't have tests (yet) and probably isn't used that
much; perhaps it's best to let this mature in HEAD for a few buildfarm cycles
first?
Yeah, that's a good point. I didn't realize we had no tests for that
one, so then it's a bit less safe in that regard. I agree with leaving
it for at least one complete buildfarm run before backporting
anything.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/