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