Skip to content
This repository has been archived by the owner on May 5, 2020. It is now read-only.

Graph tls changes #1

Open
wants to merge 8 commits into
base: graph-base-v8-11-0
Choose a base branch
from
Open

Conversation

FalconerTC
Copy link

@FalconerTC FalconerTC commented Jul 3, 2018

This PR will be kept open and maintained and releases of the observer-pool will be from the branch graph-tls-changes

@@ -218,7 +227,7 @@ function onnewsession(key, session) {
var once = false;

this._newSessionPending = true;
if (!this.server.emit('newSession', key, session, done))
if (!this.server.emit('newSession', key, session, self, done))
Copy link
Author

Choose a reason for hiding this comment

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

Original sessionID change

self.server &&
!self.server.emit('resumeSession', hello.sessionId, onSession)) {
!self.server.emit('resumeSession', hello.tlsTicket || hello.sessionId, self, onSession)) {
Copy link
Author

Choose a reason for hiding this comment

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

Partially original sessionID change

@@ -97,9 +97,9 @@ function loadSession(self, hello, cb) {
}

if (hello.sessionId.length <= 0 ||
hello.tlsTicket ||
// hello.tlsTicket ||
Copy link
Author

Choose a reason for hiding this comment

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

This was added in nodejs@dda22a5 to prevent resumeSession being executed on Session Tickets, but we want that to happen.

@@ -167,7 +169,7 @@ void ClientHelloParser::ParseExtension(const uint16_t type,
break;
case kTLSSessionTicket:
tls_ticket_size_ = len;
tls_ticket_ = data + len;
tls_ticket_ = data;
Copy link
Author

@FalconerTC FalconerTC Jul 11, 2018

Choose a reason for hiding this comment

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

90% sure this was a small bug in node that was never being used for anything. Maybe I'll make a PR on some new Github account and actually contribute to open source node.

size_t key_name_offset = 16;
// IV is the only part we are actually going to take
size_t key_iv_size = 16;
buff = Buffer::Copy(
Copy link
Author

Choose a reason for hiding this comment

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

We strip the IV from the session ticket and expose it as part of the ClientHello object that propagates to JS land

src/tls_wrap.cc Outdated
// New Session Ticket
// https://www.ietf.org/rfc/rfc5077.txt
// TODO base this on the actual packet, not length
if (write_size_ == 258) {
Copy link
Author

Choose a reason for hiding this comment

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

This is the real hack. We listen to all socket output for New Session Ticket events, and strip the IV from the ticket and call our function in JS land that simulates a newSession.

@JoshFerge
Copy link

great work ⭐️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants