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

crypto: use SSL_get_servername. #9347

Closed
wants to merge 1 commit into from
Closed

crypto: use SSL_get_servername. #9347

wants to merge 1 commit into from

Conversation

agl
Copy link
Contributor

@agl agl commented Oct 28, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 28, 2016
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 28, 2016
@mscdex
Copy link
Contributor

mscdex commented Oct 28, 2016

/cc @nodejs/crypto

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM. I've overlooked this during initial commit. Good catch! I guess it is more relevant in 1.1.0

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

Sorry, one nit before landing.

strlen(sess->tlsext_hostname));
info->Set(env->servername_string(), servername);
}
info->Set(env->tls_ticket_string(),
Copy link
Member

Choose a reason for hiding this comment

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

On a second thought, we need to remove tls_ticket_string from env.h too, since it is not used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tls_ticket_string is still used in SSLWrap<Base>::OnClientHello. I believe at that point it is meaningful.

The reference to tlsTicket in _tls_wrap.js is to the one set in OnClientHello and should continue to work.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gosh. You are right.

@indutny
Copy link
Member

indutny commented Oct 29, 2016

One more nit, it looks like tlsTicket is set in some cases after all. This test fails after removing it from _tls_wrap.js:

test/parallel/test-tls-session-cache.js

@indutny
Copy link
Member

indutny commented Oct 29, 2016

Just to state it clearly, I withdraw my LGTM for now. Sorry!

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

strlen(sess->tlsext_hostname));
info->Set(env->servername_string(), servername);
}
info->Set(env->tls_ticket_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Ah, gosh. You are right.

@davidben
Copy link
Contributor

davidben commented Nov 9, 2016

Is there anything else left to be done for this PR?

@indutny
Copy link
Member

indutny commented Nov 9, 2016

Merging it 😉

@nodejs/crypto any comments before I'll land it?

Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

LGTM after CI is green.

@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

shigeki pushed a commit that referenced this pull request Nov 9, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

CI is green except unstable timeout in smartos. Landed in 305f75a. Thanks.

@shigeki shigeki closed this Nov 9, 2016
Fishrock123 pushed a commit that referenced this pull request Nov 22, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

should this be backported?

@bnoordhuis
Copy link
Member

@thealphanerd If it cherry-picks cleanly, sure, why not? If it doesn't, it's fine to leave it out.

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
(Patch by David Benjamin.)

Rather than reach into the SSL_SESSION, use the intended API,
SSL_get_servername. This will also help the transition to OpenSSL 1.1.0.

Also don't fill in the tlsTicket field here. This is never read by
oncertcb and was always false anyway; that field is maintained by
clients and tracks whether the server issued a ticket or a session ID.

(Note this is distinct from the copy passed to onclienthello which is
used and is not a no-op.)

PR-URL: #9347
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants