tls: deprecate newSession/resumeSession events

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)

[indutny]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tls

Description of change

Deprecate asynchronous newSession/resumeSession events, introduce a
synchronous APIs via newSession/resumeSession option-callback for
tls.createServer.

The reason for this transition is rather simple. There is a quite big
amount of code that was added to support this construction, and not that
much users of it. Additionally, that code chunk is running in front of
OpenSSL, so it makes the process of asynchronous session resumption
twice as ineffective.

See: #1462

[indutny]

Next PR will be for removal of this API.

CI: https://ci.nodejs.org/job/node-test-pull-request/1948/

cc @nodejs/crypto
R= @bnoordhuis or @shigeki

[indutny]

also cc @nodejs/lts , not sure what is our deprecation policy.

[indutny]

cc @tlivings

[bnoordhuis]

I don't think synchronous APIs are a good idea. It makes it impossible to store the session data in a remote service like redis or memcached or a cluster master.

[indutny]

@bnoordhuis the sessions are not of a big use anyway, browsers are using TLS tickets, and CLI clients (read other servers) can move to them too. Do you have a use-case for this at IBM?

[bnoordhuis]

Not IBM per se but I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

[indutny]

@bnoordhuis I believe that this use case is very limited and actually helps only on a small percentage of incoming connections.

[tlivings]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time. If it's used to distribute session on new session, isn't super useful either since it doesn't benefit anyone immediately. Pre-caching sessions in memory is a lot faster and doesn't rely on an async method.

[indutny]

@tlivings but it could work with a sync method too?

[tlivings]

Yeah, that's what I am saying. If sessions are pre-cached in memory you only need a sync method.

[bnoordhuis]

Storing sessions in a database or cache like redis doesn't really give any performance benefit of looked up at connect time.

Specious. Do you have numbers to back that up? The extra TCP round-trips in a full TLS handshake aren't free.

[tlivings]

My point is that the time to fetch a session out of redis is slower than to fetch it out of memory. Particularly if you are in a distributed environment where you have to go over the wire to your store.

[bnoordhuis]

Sure, in-process memory is faster than out-of-process memory. That doesn't mean there are no use cases for the latter, though.

[indutny]

@bnoordhuis what I'm trying to say is that this use-case doesn't really help that much in presence of TLS sessions tickets which are supported by almost everyone nowadays.

[bnoordhuis]

I'm willing to be convinced by numbers. I researched this subject a few months ago actually and I wasn't really able to come to a conclusion. Keep in mind that many node apps talk to more than just browsers.

FWIW, I'm open to the argument that a session cache is less secure than session tickets, but that's a nuanced topic because it's also perfectly possible to botch the PFS of session tickets.

[indutny]

@bnoordhuis some numbers from 2012 by @pquernahttps://journal.paul.querna.org/articles/2012/09/07/adoption-of-tls-extensions . I'm sure adoption is much higher now, but honestly... we are currently carrying a SSL Record parsing just for this purpose. Unless you have numbers to prove that it is very useful - I'm really committed to removing of it.

[bnoordhuis]

I'm not worried about browsers but, as an example, I wasn't able to get session tickets working with python2's ssl module. I didn't dive in too deeply (I stopped at objdump -T _ssl.so | grep tlsext) but I couldn't find anything that suggests it supports it.

[bajtos]

I believe @bajtos wrote a module that stores sessions in the cluster master using newSession/resumeSession.

The module is called strong-cluster-tls-store and it uses Node's master<->worker IPC mechanism to synchronize sessions across cluster workers running under the same cluster master process. The usage of the module is described in this blogpost from 2013: https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/

IIRC, the IPC mechanism used to send messages between Node processes is asynchronous, therefore it cannot be used with a sync version of newSession/resumeSession.

To be honest, I am not following the Node core development closely, I don't know how many people use the native cluster + port sharing capability (as opposed to having each worker listen on an unique port and have Nginx to do both TLS termination and load balancing, for example). Therefore I have no idea whether strong-cluster-tls-store and newSession/resumeSession events are of any use today.

From the point of API consumers, I think it's important to preserve the ability to set newSession and resumeSession after the server was created:

// appvarhttps=require('https');varshareTlsSessions=require('strong-cluster-tls-store');varhttpsOpts={/* configure certificates, etc. */}varserver=https.createServer(httpsOpts,function(req,res){// handle the request});shareTlsSessions(server);server.listen(443);// inside strong-cluster-tls-storemodule.exports=functionshareTlsServer(server){server.newSession=function(){ ... };server.resumeSession=function(){ ... };}

If I am reading the patch correctly, then my example above should work, but it depends on undocumented APIs server.newSession and server.resumeSession. Perhaps these two new properties/methods should be included in the documentation and the API contract?

[indutny]

@bajtos perhaps!

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

[bajtos]

The question that we are trying to figure out here is how much use does this async session API really provide. The new API is going to be all synchronous, so ruling out async use cases is important to proceed here.

I see. Well, I cannot help you much with that, beyond pointing the single use case which requires async session API that I am aware of - sharing TLS sessions among cluster workers. For what's it worth, because it may not be a use case worth supporting anyways. strong-cluster-tls-store has 85 downloads per months according to npmjs.

I am afraid it's really up to you to make the decision.

One more idea to consider: since the synchronous API has extremely limited use, is it worth to support it at all? Perhaps, instead of changing newSession and resumeSession from async to sync, should we remove it completely?

[tlivings]

We shouldn't remove it, since the point of this change is to maintain support, albeit sync.

The Strongloop example is one way of synchronizing across clusters, but such methods should be able to convert to local population over async, while the outgoing http call looks up from local synchronously. In other words, rather than performing an async call to fetch a session for resume, the expectation would be that some tool/framework would be populating into memory sessions to resume from the shared storage, and therefor not require the complexity of async in node core.

[bnoordhuis]

You mean push-to-workers instead of pull-from-master? That can work but it produces traffic relative to the size of the cluster (O(n) instead of O(1)) and it makes it more likely to get cache misses because the session data hasn't fully propagated yet.

Also, I'm not sure what would happen if there is a cache hit but it's for stale data... I think centralization is easier to reason about in this case.

[indutny]

@bnoordhuis I really suggest that the most of the clients will use tickets, unless they are legacy. Even python will use tickets if it will use TLS protocol with openssl.so that supports tickets. In case of tickets - there is no need to pre-distribute them, it is just the ticket key that should be in sync between workers.

Also, from the client-side perspective it works the same way as old SSL sessions, they just decode SSL_SESSION and use it when connecting. No difference at all, hence it will work with many of them, even those that didn't support it at first.

[jasnell]

ping @indutny ... is this something you still want to pursue?

[indutny]

Yes, I want to pursue it, but it stalled.

[jasnell]

@indutny ... I'm going to close this due to lack of forward progress. When you're ready to tackle it again, feel free to reopen or open a new PR :-)