tls: add getter and setter for session ticket number.#34020
tls: add getter and setter for session ticket number.#34020mkrawczuk wants to merge 2 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
c++
addaleax
added
semver-minor
Co-authored-by: Anna Henningsen <github@addaleax.net>
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
I'm missing two things from the documentation:
-
Why you would want to change the default. (I'm aware openssl lets you but besides compliance testing I have no idea why you would.)
-
No mention that the setting only applies to the initial handshake. For resumption, it's fixed at 1.
| uint32_t numTickets = args[0].As<Uint32>()->Value(); | ||
|
|
||
| CHECK(SSL_CTX_set_num_tickets(sc->ctx_.get(), numTickets)); |
There was a problem hiding this comment.
| uint32_t numTickets = args[0].As<Uint32>()->Value(); | |
| CHECK(SSL_CTX_set_num_tickets(sc->ctx_.get(), numTickets)); | |
| uint32_t num_tickets = args[0].As<Uint32>()->Value(); | |
| CHECK_EQ(1, SSL_CTX_set_num_tickets(sc->ctx_.get(), num_tickets)); |
| options.clientCertEngine); | ||
| } | ||
|
|
||
| if (options.numTickets) { |
There was a problem hiding this comment.
This won't let you set it to 0.
| } | ||
|
|
||
| if (options.numTickets) | ||
| this.numTickets = options.numTickets; |
There was a problem hiding this comment.
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.
Hey @bnoordhuis could you please clarify how it creates two hidden classes: one with the property, one without?
| }); | ||
|
|
||
| const expectedNumTickets = 1; | ||
| // 2 is the deafult value set by OpenSSL. |
There was a problem hiding this comment.
| // 2 is the deafult value set by OpenSSL. | |
| // 2 is the default value set by OpenSSL. |
| code: 'ERR_INVALID_ARG_TYPE', | ||
| message: 'Number of tickets must be an unsigned 32-bit integer' | ||
| } | ||
| ); |
There was a problem hiding this comment.
Can you check multiple values, e.g.:
for (const expectedNumTickets of [0, 1, 2, 42, 1337, 2 ** 32 - 1]) {
// ...
}Checking that 2 ** 32 throws would be good, too.
|
|
||
| Server.prototype.getNumTickets = function getNumTickets() { | ||
| return this._sharedCreds.context.getNumTickets(); | ||
| }; |
There was a problem hiding this comment.
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.numTickets = 1) rather than separate functions like this.
|
@mkrawczuk - This PR seems to have gotten a little stuck and this requires rebase due to git conflicts. |
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
9 participants
This is a TLS API extension enabling to control the number of session tickets that server sends to the client. Usually it is 2, but sometime it makes sens to set it to 1, or even 0.
make -j4 test(UNIX), orvcbuild test(Windows) passes