-
Notifications
You must be signed in to change notification settings - Fork 30k
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 getter and setter for session ticket number. #34020
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1315,6 +1315,9 @@ Server.prototype.setSecureContext = function(options) { | |
.slice(0, 32); | ||
} | ||
|
||
if (options.numTickets) | ||
this.numTickets = options.numTickets; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto, plus it introduces a performance gotcha in that it creates two hidden classes: one with the property, one without. Always set the property. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @bnoordhuis could you please clarify how it creates two hidden classes: one with the property, one without? |
||
|
||
this._sharedCreds = tls.createSecureContext({ | ||
pfx: this.pfx, | ||
key: this.key, | ||
|
@@ -1332,7 +1335,8 @@ Server.prototype.setSecureContext = function(options) { | |
secureOptions: this.secureOptions, | ||
honorCipherOrder: this.honorCipherOrder, | ||
crl: this.crl, | ||
sessionIdContext: this.sessionIdContext | ||
sessionIdContext: this.sessionIdContext, | ||
numTickets: this.numTickets | ||
}); | ||
|
||
if (this.sessionTimeout) | ||
|
@@ -1366,6 +1370,14 @@ Server.prototype.setTicketKeys = function setTicketKeys(keys) { | |
this._sharedCreds.context.setTicketKeys(keys); | ||
}; | ||
|
||
Server.prototype.getNumTickets = function getNumTickets() { | ||
return this._sharedCreds.context.getNumTickets(); | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm.. I thought I had left a review comment on this previously but I'm not seeing it now... Stylistically, I'd much prefer these to get regular getter/setters (e.g. |
||
|
||
|
||
Server.prototype.setNumTickets = function setNumTickets(numTickets) { | ||
this._sharedCreds.context.setNumTickets(numTickets); | ||
mkrawczuk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
Server.prototype.setOptions = deprecate(function(options) { | ||
this.requestCert = options.requestCert === true; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -492,6 +492,8 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) { | |||||||||||||
env->SetProtoMethod(t, "enableTicketKeyCallback", EnableTicketKeyCallback); | ||||||||||||||
env->SetProtoMethodNoSideEffect(t, "getCertificate", GetCertificate<true>); | ||||||||||||||
env->SetProtoMethodNoSideEffect(t, "getIssuer", GetCertificate<false>); | ||||||||||||||
env->SetProtoMethodNoSideEffect(t, "getNumTickets", GetNumTickets); | ||||||||||||||
env->SetProtoMethod(t, "setNumTickets", SetNumTickets); | ||||||||||||||
|
||||||||||||||
#define SET_INTEGER_CONSTANTS(name, value) \ | ||||||||||||||
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), name), \ | ||||||||||||||
|
@@ -1558,6 +1560,32 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) { | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
void SecureContext::GetNumTickets(const FunctionCallbackInfo<Value>& args) { | ||||||||||||||
SecureContext* sc; | ||||||||||||||
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); | ||||||||||||||
|
||||||||||||||
CHECK_EQ(args.Length(), 0); | ||||||||||||||
|
||||||||||||||
uint32_t num_tickets = SSL_CTX_get_num_tickets(sc->ctx_.get()); | ||||||||||||||
args.GetReturnValue().Set(num_tickets); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
void SecureContext::SetNumTickets(const FunctionCallbackInfo<Value>& args) { | ||||||||||||||
SecureContext* sc; | ||||||||||||||
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); | ||||||||||||||
|
||||||||||||||
if (args.Length() != 1 || !args[0]->IsUint32()) { | ||||||||||||||
return THROW_ERR_INVALID_ARG_TYPE( | ||||||||||||||
sc->env(), "Number of tickets must be an unsigned 32-bit integer"); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
uint32_t numTickets = args[0].As<Uint32>()->Value(); | ||||||||||||||
|
||||||||||||||
CHECK(SSL_CTX_set_num_tickets(sc->ctx_.get(), numTickets)); | ||||||||||||||
Comment on lines
+1583
to
+1585
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) { | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,40 @@ | ||||||
'use strict'; | ||||||
const common = require('../common'); | ||||||
if (!common.hasCrypto) | ||||||
common.skip('missing crypto'); | ||||||
const fixtures = require('../common/fixtures'); | ||||||
|
||||||
const assert = require('assert'); | ||||||
const tls = require('tls'); | ||||||
|
||||||
const server = tls.createServer({ | ||||||
key: fixtures.readKey('agent1-key.pem'), | ||||||
cert: fixtures.readKey('agent1-cert.pem'), | ||||||
}); | ||||||
|
||||||
const expectedNumTickets = 1; | ||||||
// 2 is the deafult value set by OpenSSL. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
assert.strictEqual(server.getNumTickets(), 2); | ||||||
// Change the number of sent session tickets. | ||||||
server.setNumTickets(expectedNumTickets); | ||||||
// Ensure that it has indeed been changed. | ||||||
assert.strictEqual(server.getNumTickets(), expectedNumTickets); | ||||||
// Ensure that an error is thrown when attempting to set the number of tickets | ||||||
// to a negative number. | ||||||
assert.throws( | ||||||
() => server.setNumTickets(-1), | ||||||
{ | ||||||
code: 'ERR_INVALID_ARG_TYPE', | ||||||
message: 'Number of tickets must be an unsigned 32-bit integer' | ||||||
} | ||||||
); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check multiple values, e.g.: for (const expectedNumTickets of [0, 1, 2, 42, 1337, 2 ** 32 - 1]) {
// ...
} Checking that |
||||||
|
||||||
// Ensure that an error is thrown when attempting to set the number of tickets | ||||||
// to Mongolian throat singing. | ||||||
assert.throws( | ||||||
() => server.setNumTickets('HURRMMMM'), | ||||||
{ | ||||||
code: 'ERR_INVALID_ARG_TYPE', | ||||||
message: 'Number of tickets must be an unsigned 32-bit integer' | ||||||
} | ||||||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't let you set it to 0.