Please review (take 4): [389 Project] #47945: Add SSL/TLS version info to the access log
by Noriko Hosoi
https://fedorahosted.org/389/ticket/47945
https://fedorahosted.org/389/attachment/ticket/47945/0001-Ticket-47945-Ad...
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
>
9 years, 5 months