git patch file (master)
0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.patch

Description: Added the currently used SSL library version info per
connection to the access log.
Sample output:
SSL
[..] conn=3 fd=64 slot=64 SSL connection from ::1 to ::1
[..] conn=3 TLS1.2 128-bit AES-GCM

startTLS
[..] conn=4 op=0 EXT oid="1.3.6.1.4.1.1466.20037" name="startTLS"
[..] conn=4 op=0 RESULT err=0 tag=120 nentries=0 etime=0
[..] conn=4 TLS1.2 128-bit AES-GCM

I think the code that maps version numbers to strings should be in one place. We already have such a table in ssl.c - NSSVersion_list.

Also, for TLS, it seems that there could be a way to convert from the protocolVersion directly to a TLS version, so that we do not have to maintain (and update with new versions :P) a string to table mapping. I think we may have to for SSL, but maybe not for TLS.

{{{
FPRINTF(stderr,
"strsclnt: SSL version %d.%d using %d-bit %s with %d-bit %s MAC\n",
channel.protocolVersion >> 8, channel.protocolVersion & 0xff,
suite.effectiveKeyBits, suite.symCipherName,
suite.macBits, suite.macAlgorithmName);
}}}

So it appears that protocolVersion is some sort of bitfield that encodes the version.

It is unfortunate that NSS doesn't provide an int-to-string (and vice versa) mapping . . .

Would it also be possible to log what version is used when a connection fails?

[07/Nov/2014:09:55:10 -0500] conn=1 fd=64 slot=64 connection from 127.0.0.1 to 127.0.0.1
[07/Nov/2014:09:55:10 -0500] conn=1 op=0 EXT oid="1.3.6.1.4.1.1466.20037" name="startTLS"
[07/Nov/2014:09:55:10 -0500] conn=1 op=0 RESULT err=0 tag=120 nentries=0 etime=0
[07/Nov/2014:09:55:10 -0500] conn=1 op=-1 fd=64 closed - Cannot communicate securely with peer: no common encryption algorithm(s).

[07/Nov/2014:09:57:47 -0500] conn=3 fd=64 slot=64 SSL connection from 127.0.0.1 to 127.0.0.1
[07/Nov/2014:09:57:47 -0500] conn=3 op=-1 fd=64 closed - Cannot communicate securely with peer: no common encryption algorithm(s).

Would be nice to see what was attempted.

Replying to [comment:3 mreynolds]:

Would it also be possible to log what version is used when a connection fails?

[07/Nov/2014:09:55:10 -0500] conn=1 fd=64 slot=64 connection from 127.0.0.1 to 127.0.0.1
[07/Nov/2014:09:55:10 -0500] conn=1 op=0 EXT oid="1.3.6.1.4.1.1466.20037" name="startTLS"
[07/Nov/2014:09:55:10 -0500] conn=1 op=0 RESULT err=0 tag=120 nentries=0 etime=0
[07/Nov/2014:09:55:10 -0500] conn=1 op=-1 fd=64 closed - Cannot communicate securely with peer: no common encryption algorithm(s).

[07/Nov/2014:09:57:47 -0500] conn=3 fd=64 slot=64 SSL connection from 127.0.0.1 to 127.0.0.1
[07/Nov/2014:09:57:47 -0500] conn=3 op=-1 fd=64 closed - Cannot communicate securely with peer: no common encryption algorithm(s).

Would be nice to see what was attempted.

I also thought it's an excellent idea, but it seems NSS does not return the channel info including the version number if the SSL handshake is not successful... :(

This is the channel info when "TLS error -12190:Peer reports incompatible or unsupported protocol version." is returned. protocolVersion is supposed to be 0x2, 0x300, or above. But no such meaningful value is returned.
{{{
(gdb) p channelInfo
$3 = {length = 80, protocolVersion = 0, cipherSuite = 0, authKeyBits = 0, keaKeyBits = 0, creationTime = 0, lastAccessTime = 0,
expirationTime = 0, sessionIDLength = 0, sessionID = '\000' <repeats 31 times>, compressionMethodName = 0x0,
compressionMethod = ssl_compression_null}
}}}

Ok. Please file a bug against NSS for this. In the meantime, we'll just have to do without.

git patch file (master) -- To convert the SSL version number to string, instead of maintaining a mapping table, calculates the number and generates the version string
0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.2.patch

Note: I filed 2 RFEs against NSS.
Bug 1161807 - [RFE] API to convert SSL version number to SSL version string
Bug 1161808 - [RFE] method to get the SSL version info from the client regardless of the handshake result

One more optimization - give the function the ability to write the string to a given buffer + length e.g. slapi_getNSSVersion_str(protocolnum, char *buf, size_t bufsize)

If buf is NULL or bufsize is 0, do the smprintf, otherwise, snprintf(buf, bufsize, ...)

git patch file (master) -- slapi_getSSLVersion_str takes buf to return the SSL version string.
0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.3.patch

Replying to [comment:7 rmeggins]:

One more optimization - give the function the ability to write the string to a given buffer + length e.g. slapi_getNSSVersion_str(protocolnum, char *buf, size_t bufsize)

If buf is NULL or bufsize is 0, do the smprintf, otherwise, snprintf(buf, bufsize, ...)

Thank you, Rich!! Updated the patch.

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

git patch file (master) -- updated the patch following comment:9 by Rich
0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.4.patch

Thanks. Almost there
{{{
if ((vnum & SSL_LIBRARY_VERSION_3_0) == SSL_LIBRARY_VERSION_3_0) {
...
}}}
This will only work for TLSv1.x. I would like to see support for TLSv2.x and later, something like this:
{{{
if (vnum >= SSL_LIBRARY_VERSION_3_0) {
if (vnum == SSL_LIBRARY_VERSION_3_0) { / SSL3 /
if (buf && bufsize) {
PR_snprintf(buf, bufsize, "SSL3");
} else {
vstr = slapi_ch_smprintf("SSL3");
}
} else { / TLS v X.Y /
const char TLSFMT = "TLS%d.%d";
int minor_offset = 0; / e.g. 0x0401 -> TLS v 2.1, not 2.0 */

        if ((vnum & SSL_LIBRARY_VERSION_3_0) == SSL_LIBRARY_VERSION_3_0) {
            minor_offset = 1; /* e.g. 0x0301 -> TLS v 1.0, not 1.1 */
        }
        if (buf && bufsize) { 
            PR_snprintf(buf, bufsize, TLSFMT, (vnum >> 8) - 2, (vnum & 0xff) - minor_offset); 
        } else { 
            vstr = slapi_ch_smprintf(TLSFMT, (vnum >> 8) - 2, (vnum & 0xff) - minor_offset); 
        }
    }
} else { /* SSL2 or unknown */
    ...
}

}}}
That way, if vnum > SSL_LIBRARY_VERSION_3_0 (e.g. vnum == 0x0400 e.g. TLS v2.0) our code will support it with no changes.

git patch file (master) -- applied the change in comment:11 by Rich.
0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.5.patch

Looks good. Thanks!

One more thing - please open a ticket for logconv.pl to support parsing/showing/reporting different protocol versions

Replying to [comment:13 rmeggins]:

One more thing - please open a ticket for logconv.pl to support parsing/showing/reporting different protocol versions

Done!
Ticket #47949 (new enhancement)
logconv.pl -- support parsing/showing/reporting different protocol versions

Reviewed by Rich (Thank you!!)

Pushed to master:
8f540a6..47868d3 master -> master
commit a2e0de3

Pushed to 389-ds-base-1.3.3:
d87202a..cb4f0cb 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 0d1087d

https://fedorahosted.org/389/ticket/47949 - addresses the logconv.pl changes for the new access logging.

git patch file (1.2.11) -- Back-ported commit a2e0de3
0001-Ticket-47945-Add-SSL-TLS-version-info-to-the-access-.6.patch

Pushed to 389-ds-base-1.2.11:
099d1ce..8550aaf 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit d62b281

Pushed to 389-ds-base-1.3.1:
d4d34b2..0680446 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit 0680446be988316e26715a2c3dbeb3163f7f6e85

Pushed to 389-ds-base-1.3.2:
f7ae1e8..712d8eb 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 712d8eb

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1276

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.