[389-devel] Please review (take 4): [389 Project] #47945: Add SSL/TLS version info to the access log

[Noriko Hosoi]

https://fedorahosted.org/389/ticket/47945

https://fedorahosted.org/389/attachment/ticket/47945/0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.4.patch
git patch file (master) -- updated the patch following comment:9 
<https://fedorahosted.org/389/ticket/47945#comment:9> by Rich

On 11/10/2014 08:01 AM, 389 Project wrote:
> #47945: Add SSL/TLS version info to the access log
> -------------------------------------+-------------------------------
>          Reporter:  nhosoi            |          Owner:  nhosoi
>              Type:  defect            |         Status:  accepted
>          Priority:  major             |      Milestone:  1.3.3 backlog
>         Component:  Directory Server  |        Version:  1.3.0
>        Resolution:                    |       Keywords:
>        Blocked By:                    |       Blocking:
>            Review:  review?           |  Ticket origin:  Community
> Red Hat Bugzilla:                    |
> -------------------------------------+-------------------------------
>
> Comment (by rmeggins):
>
>   one small coding problem
>   {{{
>           (void) slapi_getSSLVersion_str(enabledNSSVersions.max, emax,
>   sizeof(emin));
>   }}}
>   should be sizeof(emax)
>
>   Also, in restrict_SSLVersionRange() - I think you can just format all of
>   the strings once at the top of the function.  e.g. something like this:
>   {{{
>       char mymin[VERSION_STR_LENGTH], mymax[VERSION_STR_LENGTH];
>       char emin[VERSION_STR_LENGTH], emax[VERSION_STR_LENGTH];
>       char recommendedmin[VERSION_STR_LENGTH];
>
>       (void) slapi_getSSLVersion_str(slapdNSSVersions.min, mymin,
>   sizeof(mymin));
>       (void) slapi_getSSLVersion_str(slapdNSSVersions.max, mymax,
>   sizeof(mymax));
>       (void) slapi_getSSLVersion_str(enabledNSSVersions.min, emin,
>   sizeof(emin));
>       (void) slapi_getSSLVersion_str(enabledNSSVersions.max, emax,
>   sizeof(emax));
>       (void) slapi_getSSLVersion_str(SSL_LIBRARY_VERSION_TLS_1_1,
>   recommendedmin, sizeof(recommendedmin));
>   }}}
>   It would save a lot of code below.
>
>   Finally, we don't have to hardcode the "1" in "TLS1".  Take a look at the
>   definitions:
>   {{{
>       #define SSL_LIBRARY_VERSION_TLS_1_0             0x0301
>       #define SSL_LIBRARY_VERSION_TLS_1_1             0x0302
>   }}}
>   The major version ("1") is just (value >> 8) - 2.  So for "TLS" we could
>   format the string version like this:
>   {{{
>   char *
>   slapi_getSSLVersion_str(PRUint16 vnum, char *buf, size_t bufsize)
>   {
>   ...
>       if (vnum >= SSL_LIBRARY_VERSION_3_0) { /* e.g. TLSv1.x, TLSv2.x, etc.
>   */
>           if (vnum & 0xff) { /* TLS */
>               if (buf && bufsize) {
>                   PR_snprintf(buf, bufsize, "TLS%d.%d", (vnum >> 8) - 2,
>   (vnum & 0xff) - 1);
>               } else {
>                   vstr = slapi_ch_smprintf("TLS%d.%d", (vnum >> 8) - 2,
>   (vnum & 0xff) - 1);
>               }
>   ...
>   }}}
>   That should give us a few years of future-proofing if TLS 2.x comes out
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.fedoraproject.org/pipermail/389-devel/attachments/20141110/531ee2a3/attachment.html>