Thread: New functions
<div><pre>Hello, </pre><pre>I make an a patch, which adds 4 functions to sslinfo extension module:<br />1) ssl_extension_names() --- get shortnames of X509v3 extensions from client certificate and it's values; 2) ssl_extension_value(text) --- get value of extension from certificate (argument --- short name of extension); 3) ssl_extension_is_critical(text) --- returns true, if extension is critical and false, if is not (argument --- short nameof extension). You can view some information of certificate's extensions via those functions. What do you think about it?</pre></div><div>-- </div><div>Best regards, Dmitry Voronin</div><div> </div>
Sorry, 3 functions. 08.03.2015, 22:16, "Dmitry Voronin" <carriingfate92@yandex.ru>: > Hello, > > I make an a patch, which adds 4 functions to sslinfo extension module: > 1) ssl_extension_names() --- get short names of X509v3 extensions from client certificate and it's values; 2) ssl_extension_value(text)--- get value of extension from certificate (argument --- short name of extension); 3) ssl_extension_is_critical(text)--- returns true, if extension is critical and false, if is not (argument --- short name ofextension). You can view some information of certificate's extensions via those functions. What do you think about it? > -- > Best regards, Dmitry Voronin > > , > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Best regards, Dmitry Voronin
Dmitry Voronin wrote: > <div><pre>Hello, > </pre><pre>I make an a patch, which adds 4 functions to sslinfo extension module:<br />1) ssl_extension_names() --- getshort names of X509v3 extensions from client certificate and it's values; > 2) ssl_extension_value(text) --- get value of extension from certificate (argument --- short name of extension); > 3) ssl_extension_is_critical(text) --- returns true, if extension is critical and false, if is not (argument --- shortname of extension). > > You can view some information of certificate's extensions via those functions. Since you change default version from 1.0 to 1.1 you need to supply the sslinfo--1.1.sql script. You also need to supply a sslinfo--1.0--1.1.sql script which enables updates from version 1.0 to 1.1. (The test for the upgrade is to initdb 9.4, and in some database install the sslinfo extension; then pg_upgrade to a version with your patch. In the database with the extension then try to upgrade it to 1.1.) Please use C type bool not int when that's what the code means. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
<p>You're right. <br /><br />I changed:<br /> - at sslinfo.contol return default module version to '1.0';<br /> - functionget_extension() returns now boolean (true, if we found extension, and false otherwise).<br /><br />09.03.2015, 16:43,"Alvaro Herrera" <<a href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>:<blockquote> DmitryVoronin wrote:<br /><blockquote> <div><pre>Hello,<br/> </pre><pre>I make an a patch, which adds 4 functions to sslinfoextension module:<br />1) ssl_extension_names() --- get short names of X509v3 extensions from client certificateand it's values;<br /> 2) ssl_extension_value(text) --- get value of extension from certificate (argument ---short name of extension);<br /> 3) ssl_extension_is_critical(text) --- returns true, if extension is critical and false,if is not (argument --- short name of extension).<br /><br /> You can view some information of certificate's extensionsvia those functions.</blockquote> Since you change default version from 1.0 to 1.1 you need to supply the<br /> sslinfo--1.1.sqlscript. You also need to supply a sslinfo--1.0--1.1.sql<br /> script which enables updates from version1.0 to 1.1.<br /><br /> (The test for the upgrade is to initdb 9.4, and in some database install<br /> the sslinfoextension; then pg_upgrade to a version with your patch. In<br /> the database with the extension then try to upgradeit to 1.1.)<br /><br /> Please use C type bool not int when that's what the code means.<br /><br /> --<br /> ÁlvaroHerrera <a href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a><br /> PostgreSQL Development,24x7 Support, Remote DBA, Training & Services</blockquote><p>-- <br />Best regards, Dmitry Voronin
On 03/08/2015 08:14 PM, Dmitry Voronin wrote: > What do you think about it? Nice to see you back working on the patch. For reviewers: the previous discussion and review of the patch can be found at http://www.postgresql.org/message-id/53A88911.6060300@proxel.se. -- Andreas Karlsson
On Tue, Mar 10, 2015 at 7:24 AM, Andreas Karlsson <andreas@proxel.se> wrote: > On 03/08/2015 08:14 PM, Dmitry Voronin wrote: >> >> What do you think about it? > > > Nice to see you back working on the patch. > > For reviewers: the previous discussion and review of the patch can be found > at http://www.postgresql.org/message-id/53A88911.6060300@proxel.se. Worth noticing as well, this has been added to CF 2015-06: https://commitfest.postgresql.org/5/192/ (I am going to remove the duplicate entry 191..) -- Michael
Hello, Michael. Please, attach new version of my patch to commitfest page. -- Best regards, Dmitry Voronin
Attachment
> Please, attach new version of my patch to commitfest page. Micheal, I found a way to attach patch. sorry to trouble. -- Best regards, Dmitry Voronin
On Mon, Mar 23, 2015 at 1:48 AM, Воронин Дмитрий <carriingfate92@yandex.ru> wrote: > >> Please, attach new version of my patch to commitfest page. > > Michael, I found a way to attach patch. sorry to trouble. Cool. Thanks. I am seeing your patch entry here: https://commitfest.postgresql.org/5/192/ I'll try to take a look at it for the next commit fest, but please do not expect immediate feedback things are getting wrapped up for 9.5. Regards, -- Michael
On Mon, Mar 23, 2015 at 12:46 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Mar 23, 2015 at 1:48 AM, Воронин Дмитрий > <carriingfate92@yandex.ru> wrote: >> >>> Please, attach new version of my patch to commitfest page. >> >> Michael, I found a way to attach patch. sorry to trouble. > > Cool. Thanks. I am seeing your patch entry here: > https://commitfest.postgresql.org/5/192/ > I'll try to take a look at it for the next commit fest, but please do > not expect immediate feedback things are getting wrapped up for 9.5. OK, I am back on this patch and I am just looking at it. There are a couple of issues that need to be fixed, and I will send back feedback and a new patch by tomorrow. -- Michael
On Mon, Mar 23, 2015 at 12:46 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Mar 23, 2015 at 1:48 AM, Воронин Дмитрий > <carriingfate92@yandex.ru> wrote: >> >>> Please, attach new version of my patch to commitfest page. >> >> Michael, I found a way to attach patch. sorry to trouble. > > Cool. Thanks. I am seeing your patch entry here: > https://commitfest.postgresql.org/5/192/ > I'll try to take a look at it for the next commit fest, but please do > not expect immediate feedback things are getting wrapped up for 9.5. OK, so I have looked at this patch in more details. And here are some comments: 1) As this is an upgrade to sslinfo 1.1, sslinfo--1.0.sql is not necessary. 2) contrib/sslinfo/Makefile needs to be updated with sslinfo--1.0--1.1.sql and sslinfo--1.1.sql. 3) This return type is not necessary: + CREATE TYPE extension AS ( + name text, + value text + ); + + CREATE OR REPLACE FUNCTION ssl_extension_names() RETURNS SETOF extension + AS 'MODULE_PATHNAME', 'ssl_extension_names' + LANGUAGE C STRICT; Instead, I think that we should make ssl_extension_names return a SETOF record with some OUT parameters. Also, using a tuple descriptor saved in the user context would bring more readability. 4) sslinfo.control needs to be updated to 1.1. 5) I think that it is better to return an empty result should no client certificate be present or should ssl be disabled for a given connection. And the patch failed to do that with SRF_RETURN_DONE. 6) The code is critically lacking comments, and contains many typos. 7) The return status of get_extention() is not necessary. All the code paths calling it simply ERROR should the status be false. It is better to move the error message directly in the function and remove the status code. 8) As proposed, the patch adds 3 new functions: ssl_extension_is_critical, ssl_extension_value and ssl_extension_names. But actually I am wondering why ssl_extension_is_critical and ssl_extension_value are actually useful. I mean, what counts is the extension information about the existing client certificate, no? Hence I think that we should remove ssl_extension_is_critical and ssl_extension_value, and extend ssl_extension_names with a new boolean flag is_critical to determine if a extension given is critical or not. Let's rename ssl_extension_names to ssl_extension_info at the same time. get_extension is not needed anymore with that as well. Speaking of which, I have rewritten the patch as attached. This looks way cleaner than the previous version submitted. Dmitry, does that look fine for you? I am switching this patch as "Waiting on Author". Regards, -- Michael
Attachment
07.07.2015, 18:34, "Michael Paquier" <michael.paquier@gmail.com>: > Speaking of which, I have rewritten the patch as attached. This looks > way cleaner than the previous version submitted. Dmitry, does that > look fine for you? > I am switching this patch as "Waiting on Author". > Regards, > -- > Michael Michael, hello. I'm looking your variant of patch. You create function ssl_extensions_info(), that gives information of SSLextensions in more informative view. So, it's cool. -- Best regards, Dmitry Voronin
On Wed, Jul 8, 2015 at 9:15 PM, Дмитрий Воронин <carriingfate92@yandex.ru> wrote: > > > 07.07.2015, 18:34, "Michael Paquier" <michael.paquier@gmail.com>: > >> Speaking of which, I have rewritten the patch as attached. This looks >> way cleaner than the previous version submitted. Dmitry, does that >> look fine for you? >> I am switching this patch as "Waiting on Author". >> Regards, >> -- >> Michael > > Michael, hello. I'm looking your variant of patch. You create function ssl_extensions_info(), that gives information ofSSL extensions in more informative view. So, it's cool. OK cool. I think a committer could look at it, hence switching it to "Ready for Committer". -- Michael
On Wed, Jul 8, 2015 at 10:18 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > > On Wed, Jul 8, 2015 at 9:15 PM, Дмитрий Воронин > <carriingfate92@yandex.ru> wrote: > > > > > > 07.07.2015, 18:34, "Michael Paquier" <michael.paquier@gmail.com>: > > > >> Speaking of which, I have rewritten the patch as attached. This looks > >> way cleaner than the previous version submitted. Dmitry, does that > >> look fine for you? > >> I am switching this patch as "Waiting on Author". > >> Regards, > >> -- > >> Michael > > > > Michael, hello. I'm looking your variant of patch. You create function ssl_extensions_info(), that gives informationof SSL extensions in more informative view. So, it's cool. > > OK cool. I think a committer could look at it, hence switching it to > "Ready for Committer". Note for committers: attached is a small script that will generate a client certificate with extensions enabled. This is helpful when testing this patch. Once created, then simply connect with something like this connection string: "host=127.0.0.1 sslmode=verify-full sslcert=client.crt sslkey=client.key sslrootcert=server.crt" By querying the new function implemented by this patch the information about the client certificate extensions will show up. -- Michael
Attachment
On 07/08/2015 01:15 PM, Дмитрий Воронин wrote: > 07.07.2015, 18:34, "Michael Paquier" <michael.paquier@gmail.com>: > >> Speaking of which, I have rewritten the patch as attached. This looks >> way cleaner than the previous version submitted. Dmitry, does that >> look fine for you? >> I am switching this patch as "Waiting on Author". > > Michael, hello. I'm looking your variant of patch. You create > function ssl_extensions_info(), that gives information of SSL > extensions in more informative view. So, it's cool. Should check the return value of every OpenSSL call for errors. At least BIO_new() could return NULL, but check all the docs of the others too. Are all the functions used in the patch available in all the versions of OpenSSL we support? Other than those little things, looks good to me. - Heikki
On Sat, Aug 22, 2015 at 11:24 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 07/08/2015 01:15 PM, Дмитрий Воронин wrote: >> >> 07.07.2015, 18:34, "Michael Paquier" <michael.paquier@gmail.com>: >> >>> Speaking of which, I have rewritten the patch as attached. This looks >>> way cleaner than the previous version submitted. Dmitry, does that >>> look fine for you? >>> I am switching this patch as "Waiting on Author". >> >> >> Michael, hello. I'm looking your variant of patch. You create >> function ssl_extensions_info(), that gives information of SSL >> extensions in more informative view. So, it's cool. > > > Should check the return value of every OpenSSL call for errors. At least > BIO_new() could return NULL, but check all the docs of the others too. Right, agreed for BIO_new(). BIO_write and BIO_get_mem_data can return negative error code, but that's not necessarily the indication of an error by looking at the docs, so I'd rather let them as-is. X509V3_EXT_print is not documented but it can return <= 0 state code if the operation fails so I guess that it makes sense to elog an ERROR. sk_X509_EXTENSION_value and X509_EXTENSION_get_object return NULL in case of failure (looking at the code tree of openssl), and OBJ_obj2nid will return NID_undef in this case, so I think the code as-is is fine. Another interesting thing is that BIO_free can fail and we don't track that. By the way, perhaps it would be worth doing similar things for the other calls of BIO_free and BIO_new, no? I have attached a second patch. > Are all the functions used in the patch available in all the versions of > OpenSSL we support? We support openssl down to 0.9.7, right? And those functions are present there (I recall vaguely checking that when looking at this patch, and I just rechecked in case I missed something). > Other than those little things, looks good to me. Thanks! -- Michael
Attachment
On Sun, Aug 23, 2015 at 10:21 PM, Michael Paquier wrote: > By the way, perhaps it would be worth doing similar things for the > other calls of BIO_free and BIO_new, no? I have attached a second > patch. And... This second patch had a stupid mistake making for example ssl_client_dn() fail, so here they are again. -- Michael
Attachment
Michael Paquier wrote: > On Sun, Aug 23, 2015 at 10:21 PM, Michael Paquier wrote: > > By the way, perhaps it would be worth doing similar things for the > > other calls of BIO_free and BIO_new, no? I have attached a second > > patch. > > And... This second patch had a stupid mistake making for example > ssl_client_dn() fail, so here they are again. What stupid mistake was this? I checked your 0002 files and they looked identical. I just pushed it all the way back to 9.0, after changing the elogs to ereports. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 8, 2015 at 7:31 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Michael Paquier wrote: >> On Sun, Aug 23, 2015 at 10:21 PM, Michael Paquier wrote: >> > By the way, perhaps it would be worth doing similar things for the >> > other calls of BIO_free and BIO_new, no? I have attached a second >> > patch. >> >> And... This second patch had a stupid mistake making for example >> ssl_client_dn() fail, so here they are again. > > What stupid mistake was this? I checked your 0002 files and they looked > identical. I just pushed it all the way back to 9.0, after changing the > elogs to ereports. Never mind. I cannot recall either. -- Michael
Michael Paquier wrote: > Note for committers: attached is a small script that will generate a > client certificate with extensions enabled. This is helpful when > testing this patch. Once created, then simply connect with something > like this connection string: > "host=127.0.0.1 sslmode=verify-full sslcert=client.crt > sslkey=client.key sslrootcert=server.crt" > By querying the new function implemented by this patch the information > about the client certificate extensions will show up. Thanks, this was useful. I made a couple extra cleanups to the patch, namely: do not call CreateTemplateTupleDesc() just to discard the resulting tupdesc with a subsequent get_call_result_type(); and do not write a \0 to the BIO_s_mem, and instead use BIO_get_mem_data's return value as length when converting str to text *. And pushed. FWIW I now think I made a mistake with the error checks that I backpatched, because the wording of the error messages I used "failed to foo" is frowned upon by our message style guidelines. Should be "could not do foo" instead. Thanks, -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 8, 2015 at 9:32 AM, Alvaro Herrera wrote: > Michael Paquier wrote: > I made a couple extra cleanups to the patch, namely: do not call > CreateTemplateTupleDesc() just to discard the resulting tupdesc with a > subsequent get_call_result_type(); and do not write a \0 to the > BIO_s_mem, and instead use BIO_get_mem_data's return value as length > when converting str to text *. Thanks! > FWIW I now think I made a mistake with the error checks that I > backpatched, because the wording of the error messages I used "failed to > foo" is frowned upon by our message style guidelines. Should be "could > not do foo" instead. Well... Feel free to use that if it makes you gain two minutes. -- Michael
Attachment
Michael Paquier wrote: > On Tue, Sep 8, 2015 at 9:32 AM, Alvaro Herrera wrote: > > Michael Paquier wrote: > > I made a couple extra cleanups to the patch, namely: do not call > > CreateTemplateTupleDesc() just to discard the resulting tupdesc with a > > subsequent get_call_result_type(); and do not write a \0 to the > > BIO_s_mem, and instead use BIO_get_mem_data's return value as length > > when converting str to text *. > > Thanks! > > > FWIW I now think I made a mistake with the error checks that I > > backpatched, because the wording of the error messages I used "failed to > > foo" is frowned upon by our message style guidelines. Should be "could > > not do foo" instead. > > Well... Feel free to use that if it makes you gain two minutes. Thanks. I changed the ones for BIO_free() too and pushed. Those comments /* done if we're done */ look a bit redundant to me. I considered just removing it altogether ... -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services