Access log reports SSL/TLS info per connection as follows. [..] conn=11 fd=66 slot=66 SSL connection from 192.168.122.159 to 192.168.122.1 [..] conn=11 SSL 128-bit AES
It should report the protocolVersion, as well.
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... :(
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]:
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
Metadata Update from @rmeggins: - Issue assigned to nhosoi - Issue set to the milestone: 1.3.3 backlog
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Log in to comment on this ticket.