Thread: 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
Attachment
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
Attachment
> 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/