tls: return correct version from getCipher()

OpenSSL 1.0.0 returned incorrect version information. OpenSSL 1.1.0 fixed this, but returning the correct information broke our tests, so was considered semver-major. Because of this, the version was hard-coded to the OpenSSL 1.0.0 (incorrect) string in 5fe81c8. This is ancient history, start returning the correct cipher version. PR-URL: #26625 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
nodejs · Mar 12, 2019 · 0f745bf9bde4e19dd352f10299d4365a4edaf386 · 0f745bf
1 parent 1ceb6d8
commit 0f745bf9bde4e19dd352f10299d4365a4edaf386
diff --git a/doc/api/tls.md b/doc/api/tls.md
@@ -717,18 +717,25 @@ socket has been destroyed, `null` will be returned.
 ### tlsSocket.getCipher()
 <!-- YAML
 added: v0.11.4
+  
+changes:
+  
+  - version: REPLACEME
+  
+    pr-url: https://github.com/nodejs/node/pull/26625
+  
+    description: Return the minimum cipher version, instead of a fixed string
+  
+      (`'TLSv1/SSLv3'`).
 -->
 * Returns: {Object}
+  
+  * `name` {string} The name of the cipher suite.
+  
+  * `version` {string} The minimum TLS protocol version supported by this cipher
+  
+    suite.
-  
-Returns an object representing the cipher name. The `version` key is a legacy
-  
-field which always contains the value `'TLSv1/SSLv3'`.
+  
+Returns an object containing information on the negotiated cipher suite.
 For example: `{ name: 'AES256-SHA', version: 'TLSv1/SSLv3' }`.
-  
-See `SSL_CIPHER_get_name()` in
-  
-<https://www.openssl.org/docs/man1.1.0/ssl/SSL_CIPHER_get_name.html> for more
-  
-information.
+  
+See
+  
+[OpenSSL](https://www.openssl.org/docs/man1.1.1/man3/SSL_CIPHER_get_name.html)
+  
+for more information.
 ### tlsSocket.getEphemeralKeyInfo()
 <!-- YAML

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
@@ -737,18 +737,18 @@ function makeSocketMethodProxy(name) {
 }
 [
-  
-  'getFinished', 'getPeerFinished', 'getSession', 'isSessionReused',
-  
-  'getEphemeralKeyInfo', 'getProtocol', 'getTLSTicket'
+  
+  'getCipher',
+  
+  'getEphemeralKeyInfo',
+  
+  'getFinished',
+  
+  'getPeerFinished',
+  
+  'getProtocol',
+  
+  'getSession',
+  
+  'getTLSTicket',
+  
+  'isSessionReused',
 ].forEach((method) => {
  TLSSocket.prototype[method] = makeSocketMethodProxy(method);
 });
-  
-TLSSocket.prototype.getCipher = function(err) {
-  
-  if (this._handle)
-  
-    return this._handle.getCurrentCipher();
-  
-  return null;
-  
-};
-  
-
 // TODO: support anonymous (nocert) and PSK

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -1464,7 +1464,7 @@ void SSLWrap<Base>::AddMethods(Environment* env, Local<FunctionTemplate> t) {
  env->SetProtoMethod(t, "loadSession", LoadSession);
  env->SetProtoMethodNoSideEffect(t, "isSessionReused", IsSessionReused);
  env->SetProtoMethodNoSideEffect(t, "verifyError", VerifyError);
-  
-  env->SetProtoMethodNoSideEffect(t, "getCurrentCipher", GetCurrentCipher);
+  
+  env->SetProtoMethodNoSideEffect(t, "getCipher", GetCipher);
  env->SetProtoMethod(t, "endParser", EndParser);
  env->SetProtoMethod(t, "certCbDone", CertCbDone);
  env->SetProtoMethod(t, "renegotiate", Renegotiate);
@@ -2357,7 +2357,7 @@ void SSLWrap<Base>::VerifyError(const FunctionCallbackInfo<Value>& args) {
 template <class Base>
-  
-void SSLWrap<Base>::GetCurrentCipher(const FunctionCallbackInfo<Value>& args) {
+  
+void SSLWrap<Base>::GetCipher(const FunctionCallbackInfo<Value>& args) {
  Base* w;
  ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
  Environment* env = w->ssl_env();
@@ -2371,8 +2371,9 @@ void SSLWrap<Base>::GetCurrentCipher(const FunctionCallbackInfo<Value>& args) {
  const char* cipher_name = SSL_CIPHER_get_name(c);
  info->Set(context, env->name_string(),
            OneByteString(args.GetIsolate(), cipher_name)).FromJust();
+  
+  const char* cipher_version = SSL_CIPHER_get_version(c);
  info->Set(context, env->version_string(),
-  
-            OneByteString(args.GetIsolate(), "TLSv1/SSLv3")).FromJust();
+  
+            OneByteString(args.GetIsolate(), cipher_version)).FromJust();
  args.GetReturnValue().Set(info);
 }

diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -281,7 +281,7 @@ class SSLWrap {
  static void LoadSession(const v8::FunctionCallbackInfo<v8::Value>& args);
  static void IsSessionReused(const v8::FunctionCallbackInfo<v8::Value>& args);
  static void VerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
-  
-  static void GetCurrentCipher(const v8::FunctionCallbackInfo<v8::Value>& args);
+  
+  static void GetCipher(const v8::FunctionCallbackInfo<v8::Value>& args);
  static void EndParser(const v8::FunctionCallbackInfo<v8::Value>& args);
  static void CertCbDone(const v8::FunctionCallbackInfo<v8::Value>& args);
  static void Renegotiate(const v8::FunctionCallbackInfo<v8::Value>& args);

diff --git a/test/parallel/test-tls-getcipher.js b/test/parallel/test-tls-getcipher.js
@@ -30,28 +30,42 @@ const tls = require('tls');
 // Import fixtures directly from its module
 const fixtures = require('../common/fixtures');
-  
-const cipher_list = ['AES128-SHA256', 'AES256-SHA256'];
-  
-const cipher_version_pattern = /TLS|SSL/;
 const options = {
  key: fixtures.readKey('agent2-key.pem'),
  cert: fixtures.readKey('agent2-cert.pem'),
-  
-  ciphers: cipher_list.join(':'),
  honorCipherOrder: true
 };
-  
-const server = tls.createServer(options, common.mustCall());
+  
+let clients = 0;
+  
+const server = tls.createServer(options, common.mustCall(() => {
+  
+  if (--clients === 0)
+  
+    server.close();
+  
+}, 2));
 server.listen(0, '127.0.0.1', common.mustCall(function() {
-  
-  const client = tls.connect({
+  
+  clients++;
+  
+  tls.connect({
    host: '127.0.0.1',
    port: this.address().port,
-  
-    ciphers: cipher_list.join(':'),
+  
+    ciphers: 'AES128-SHA256',
    rejectUnauthorized: false
  }, common.mustCall(function() {
-  
-    const cipher = client.getCipher();
-  
-    assert.strictEqual(cipher.name, cipher_list[0]);
-  
-    assert(cipher_version_pattern.test(cipher.version));
-  
-    client.end();
-  
-    server.close();
+  
+    const cipher = this.getCipher();
+  
+    assert.strictEqual(cipher.name, 'AES128-SHA256');
+  
+    assert.strictEqual(cipher.version, 'TLSv1.2');
+  
+    this.end();
+  
+  }));
+  
+
+  
+  clients++;
+  
+  tls.connect({
+  
+    host: '127.0.0.1',
+  
+    port: this.address().port,
+  
+    cipher: 'ECDHE-RSA-AES128-GCM-SHA256',
+  
+    rejectUnauthorized: false
+  
+  }, common.mustCall(function() {
+  
+    const cipher = this.getCipher();
+  
+    assert.strictEqual(cipher.name, 'ECDHE-RSA-AES128-GCM-SHA256');
+  
+    assert.strictEqual(cipher.version, 'TLSv1.2');
+  
+    this.end();
  }));
 }));
diff --git a/test/parallel/test-tls-multi-key.js b/test/parallel/test-tls-multi-key.js
@@ -157,7 +157,7 @@ function test(options) {
    }, common.mustCall(function() {
      assert.deepStrictEqual(ecdsa.getCipher(), {
        name: 'ECDHE-ECDSA-AES256-GCM-SHA384',
-  
-        version: 'TLSv1/SSLv3'
+  
+        version: 'TLSv1.2'
      });
      assert.strictEqual(ecdsa.getPeerCertificate().subject.CN, eccCN);
      // XXX(sam) certs don't currently include EC key info, so depend on
@@ -177,7 +177,7 @@ function test(options) {
    }, common.mustCall(function() {
      assert.deepStrictEqual(rsa.getCipher(), {
        name: 'ECDHE-RSA-AES256-GCM-SHA384',
-  
-        version: 'TLSv1/SSLv3'
+  
+        version: 'TLSv1.2'
      });
      assert.strictEqual(rsa.getPeerCertificate().subject.CN, rsaCN);
      assert(rsa.getPeerCertificate().exponent, 'cert for an RSA key');

diff --git a/test/parallel/test-tls-multi-pfx.js b/test/parallel/test-tls-multi-pfx.js
@@ -42,9 +42,9 @@ const server = tls.createServer(options, function(conn) {
 process.on('exit', function() {
  assert.deepStrictEqual(ciphers, [{
    name: 'ECDHE-ECDSA-AES256-GCM-SHA384',
-  
-    version: 'TLSv1/SSLv3'
+  
+    version: 'TLSv1.2'
  }, {
    name: 'ECDHE-RSA-AES256-GCM-SHA384',
-  
-    version: 'TLSv1/SSLv3'
+  
+    version: 'TLSv1.2'
  }]);
 });