Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: add getProtocol() to TLS sockets #4995

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions doc/api/tls.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -434,14 +434,15 @@ Static boolean value, always `true`. May be used to distinguish TLS sockets
from regular ones.

### tlsSocket.getCipher()
Returns an object representing the cipher name and the SSL/TLS
protocol version of the current connection.

Returns an object representing the cipher name and the SSL/TLS protocol version
that first defined the cipher.

Example:
{ name: 'AES256-SHA', version: 'TLSv1/SSLv3' }

See SSL_CIPHER_get_name() and SSL_CIPHER_get_version() in
https://www.openssl.org/docs/ssl/ssl.html#DEALING_WITH_CIPHERS for more
https://www.openssl.org/docs/manmaster/ssl/SSL_CIPHER_get_name.html for more
information.

### tlsSocket.getEphemeralKeyInfo()
Expand Down Expand Up @@ -493,6 +494,24 @@ Example:
If the peer does not provide a certificate, it returns `null` or an empty
object.

### tlsSocket.getProtocol()

Returns a string containing the negotiated SSL/TLS protocol version of the
current connection. `'unknown'` will be returned for connected sockets that have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this unknown case be tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very easily/reliably as it would probably have to amount to polling or maybe some kind of crazy monkey patching. Even at the stage in which an SNI callback is called, the protocol information is already available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, okay then. Since we claim that it would return unknown, I thought that it would have been better to have a test to confirm that.

not completed the handshaking process. `null` will be returned for server
sockets or disconnected client sockets.

Examples:
```
'SSLv3'
'TLSv1'
'TLSv1.1'
'TLSv1.2'
'unknown'
```

See https://www.openssl.org/docs/manmaster/ssl/SSL_get_version.html for more
information.

### tlsSocket.getSession()

Expand Down
7 changes: 7 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,13 @@ TLSSocket.prototype.getEphemeralKeyInfo = function() {
return null;
};

TLSSocket.prototype.getProtocol = function() {
if (this._handle)
return this._handle.getProtocol();

return null;
};

// TODO: support anonymous (nocert) and PSK


Expand Down
10 changes: 10 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,7 @@ void SSLWrap<Base>::AddMethods(Environment* env, Local<FunctionTemplate> t) {
env->SetProtoMethod(t, "setOCSPResponse", SetOCSPResponse);
env->SetProtoMethod(t, "requestOCSP", RequestOCSP);
env->SetProtoMethod(t, "getEphemeralKeyInfo", GetEphemeralKeyInfo);
env->SetProtoMethod(t, "getProtocol", GetProtocol);

#ifdef SSL_set_max_send_fragment
env->SetProtoMethod(t, "setMaxSendFragment", SetMaxSendFragment);
Expand Down Expand Up @@ -1957,6 +1958,15 @@ void SSLWrap<Base>::GetCurrentCipher(const FunctionCallbackInfo<Value>& args) {
}


template <class Base>
void SSLWrap<Base>::GetProtocol(const FunctionCallbackInfo<Value>& args) {
Base* w = Unwrap<Base>(args.Holder());

const char* tls_version = SSL_get_version(w->ssl_);
args.GetReturnValue().Set(OneByteString(args.GetIsolate(), tls_version));
}


#ifdef OPENSSL_NPN_NEGOTIATED
template <class Base>
int SSLWrap<Base>::AdvertiseNextProtoCallback(SSL* s,
Expand Down
1 change: 1 addition & 0 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ class SSLWrap {
static void RequestOCSP(const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetEphemeralKeyInfo(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetProtocol(const v8::FunctionCallbackInfo<v8::Value>& args);

#ifdef SSL_set_max_send_fragment
static void SetMaxSendFragment(
Expand Down
43 changes: 43 additions & 0 deletions test/parallel/test-tls-getprotocol.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';
const common = require('../common');
const assert = require('assert');

if (!common.hasCrypto) {
console.log('1..0 # Skipped: missing crypto');
return;
}

const tls = require('tls');
const fs = require('fs');

const clientConfigs = [
{ secureProtocol: 'TLSv1_method', version: 'TLSv1' },
{ secureProtocol: 'TLSv1_1_method', version: 'TLSv1.1' },
{ secureProtocol: 'TLSv1_2_method', version: 'TLSv1.2' }
];

const serverConfig = {
key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem')
};

const server = tls.createServer(serverConfig, common.mustCall(function() {

}, clientConfigs.length)).listen(common.PORT, common.localhostIPv4, function() {
let connected = 0;
clientConfigs.forEach(function(v) {
tls.connect({
host: common.localhostIPv4,
port: common.PORT,
rejectUnauthorized: false,
secureProtocol: v.secureProtocol
}, common.mustCall(function() {
assert.strictEqual(this.getProtocol(), v.version);
this.on('end', common.mustCall(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wonder how this is bound to the socket object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback passed to tls.connect() is just added as an event handler for the socket's secureConnect event and all EventEmitter event handlers have this set to the EventEmitter instance when the event handlers are called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. You are right. Thanks for clarifying :)

assert.strictEqual(this.getProtocol(), null);
})).end();
if (++connected === clientConfigs.length)
server.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be inside the end callback? When the third assertion is completed server has to be closed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really need to be. close() just stops listening for more connections. Once the last client has connected, the server doesn't need to listen anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm. Makes sense. Cool then

}));
});
});