<p>24.06.2014, 00:07, "Andreas Karlsson" <<a href="mailto:andreas(at)proxel(dot)se">andreas(at)proxel(dot)se</a>>:</p><blockquote> On 04/21/2014 07:51 AM, Воронин Дмитрий wrote:<br /><blockquote> I put patch generated on git diffs to this letter. I make an a thread in<br /> postgresql commit fest:<br /> <a href="https://commitfest.postgresql.org/action/patch_view?id=1438">https://commitfest.postgresql.org/action/patch_view?id=1438</a></blockquote> Thanks for the patch, it seems like a useful feature.<br /><br /> === General ===<br /><br /> - Applies cleanly to HEAD and compiles without warnings.<br /><br /> - The test suite passes.<br /><br /> - x509v3 support was added in OpenSSL 0.9.2 and sslinfo already depends<br /> heavily on OpenSSL so no new dependencies.<br /><br /> === User functionality ===<br /><br /> - If you are a user of the sslinfo extension the new functions should be<br /> useful additions.<br /><br /> - I tested the code without SSL, with certificate but without client<br /> certificate, with client certificates first without extensions and the<br /> with. All of this worked fine except for some usability which could be<br /> improved, see below.<br /><br /> - I cannot see the use for ssl_get_count_of_extensions(). When would<br /> anyone need to know the number of extensions? I think this is an<br /> implementation detail of OpenSSL which we do not need to expose. If any<br /> user wants this feature he can count the extension names.<br /><br /> - Documentation is missing for the new functions.<br /><br /> - I think the names of the new functions should be change. Below are my<br /> suggestions. Other suggestions are welcome.<br /><br /> * ssl_extension_value(text)<br /> * ssl_extension_is_critical()<br /> * ssl_extension_names()<br /> * ssl_extension_count() (If we should keep it.)<br /><br /> - Would it be interesting to have a set returning function which returns<br /> all extensions with both the names and the values? Like the below.<br /><br /> $ SELECT * FROM ssl_extensions();<br /> name | value<br /> ------------------+------------------------------------------------------<br /> basicConstraints | CA:FALSE<br /> keyUsage | Digital Signature, Non Repudiation, Key Encipherment<br /> (2 rows)<br /><br /> - I do not think that ssl_get_extension_names() should return a single<br /> row with NULL when the certificate has no extensions or when there is no<br /> certificate at all. Returning zero rows in this case should make it<br /> easier to use.<br /><br /> - Currently ssl_is_critical_extension() and ssl_get_extension_value()<br /> throw an error when the requested extension is not in the certificate.<br /><br /> I am not sure if I like this behavior. I think I would prefer if the<br /> code always throws an error when name lookup fails, and never when it is<br /> successful. For a valid extension name I think NULL should be returned<br /> when it does not exist.<br /><br /> === Code review: main ===<br /><br /> - Why are X509_NAME_field_to_text(), X509_NAME_to_text(),<br /> ASN1_STRING_to_text() and get_extension() not static? None of these are<br /> a symbol which should be exported.<br /><br /> - I have not worked with extension myself, but since your change adds<br /> functions to the extension I think you need to create a version 1.1<br /> instead of modifying 1.0 in place. If so you also need to write an<br /> upgrade script from 1.0 to 1.1. See dblink--1.0--1.1.sql for an example.<br /><br /> - Please break out the comment fix for ssl_cipher() into a separate patch.<br /><br /> - Why do you use pg_do_encoding_conversion() over pg_any_to_server()?<br /> pg_any_to_server() is implemented using pg_do_encoding_conversion().<br /><br /> - I think you should use OBJ_txt2nid() rather than OBJ_sn2nid() +<br /> OBJ_ln2nid(). You should probably also use OBJ_txt2obj() since<br /> X509_get_ext_by_NID() will call OBJ_nid2obj() anyway.<br /><br /> - You should free the extension_name string. I do not think it is ok to<br /> leak it to the end of the query.<br /><br /> - I think you need to convert the extension values and names to the<br /> server encoding. I just wonder if we need to support data which is<br /> incorrectly encoded.<br /><br /> === Code review: style issues ===<br /><br /> - Trailing whitespace in sslinfo--1.0.sql and sslinfo.c.q<br /><br /> - sslinfo--1.0.sql does not end in a newline.<br /><br /> - I do not think the PostgreSQL project adds authors in the top comment<br /> of files in cases like this. Authors get credit in the commit messages.<br /><br /> - I think you can remove the prototypes of all the ssl_* functions.<br /><br /> - Adding the have in "Indicates whether current client have provided a<br /> certificate" is not necessary. The previous wording looks correct to my<br /> non-native speaker eyes.<br /><br /> - Too much white space in variable declarations in get_extension().<br /><br /> - Extra space before -1 in "X509_get_ext_by_NID(certificate,<br /> extension_nid, -1);"<br /><br /> - Please do not initialize variables unless necessary. Compilers are<br /> pretty good at warning about uninitialized usage. For example both<br /> locate and extension_nid do not need to be initialized.<br /><br /> - Remove empty line directly before ssl_get_extension_value().<br /><br /> - Try to follow variable naming conventions from other functions (e.g.<br /> use nid rather than extension_nid, membuf rather than bio, sp rather<br /> than value).<br /><br /> - I am pretty sure the variable you call locate should be called<br /> location (or loc for short).<br /><br /> - There should not be any spaces around "->".<br /><br /> - The declaration of *extension in ssl_get_extension_value is not<br /> aligned properly.<br /><br /> - Remove white space in variable declaration in<br /> ssl_get_count_of_extensions().<br /><br /> - Incorrectly alignment of variable declarations in<br /> ssl_get_extension_names().<br /><br /> - All the "Returns X datum" comments look redundant to me, but this is a<br /> matter of preference.<br /><br /> - The star when declaring result in ssl_get_extension_names() should be<br /> put on the other side of the white space.<br /><br /> --<br /> Andreas Karlsson</blockquote><p>Hello, Andreas!<br /><br /><span lang="en"><span>I apologize, that I am writing this message today. Thank you for testing my patch!<br /><br />About user functionality</span></span>. <br />I rename my functions, your suggestions are seemed great. When I wrote those functions, I created names like others in sslinfo extension. OK, I will rename it.<br /><br />About ssl_get_extension_count(). I will delete this function. <br />I will modify functions ssl_extensions(), that it returns a set (key, value). Could you get me an example of code those function? <br /><br /> >>> - Why are X509_NAME_field_to_text(), X509_NAME_to_text(), ASN1_STRING_to_text() and get_extension() not static? None of these are a symbol which should be exported.<br />>>> Why do you use pg_do_encoding_conversion() over pg_any_to_server()? pg_any_to_server() is implemented using pg_do_encoding_conversion().<br /><br />I don't write a code of those functions and I can't answer on your question.</p><p>I will fix your notes and create a new patch version. Thank you.<br />--</p><p>Best regards, Dmitry Voronin</p>