tls: add getter and setter for session ticket number. - #34020
Conversation
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
@nodejs/crypto
Sorry, something went wrong.
mildsunrise
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Sorry, something went wrong.
Co-authored-by: Anna Henningsen <github@addaleax.net>
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.
Sorry, something went wrong.
| 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)); |
Sorry, something went wrong.
| options.clientCertEngine); | ||
| } | ||
|
|
||
| if (options.numTickets) { |
There was a problem hiding this comment.
This won't let you set it to 0.
Sorry, something went wrong.
| } | ||
|
|
||
| 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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
| }); | ||
|
|
||
| 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. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
|
@mkrawczuk - This PR seems to have gotten a little stuck and this requires rebase due to git conflicts. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.