A possible TODO list for new (or current) contributors

[sam-github]

I'm not exactly sure how to manage this, so I thought I'd start with an issue, since they are easy, and can be closed if not useful.

I keep a file where I dump quick notes on things that I intend to look into tomorrow. But, tomorrow never comes.

I just triaged my list, I believe that from a 1st pass, these are all still current. They run the full gamut from possible bugs, to missing features (some big, some trivial), to missing docs, to research on possible features. A good set of the features are quite small to implement, like js bindings into OpenSSL that have minimal interaction with any feature around them (see tls.sessionInfo as an example) and would make good first contributions. Some could be much more complex, like process.stdout.reopen().

I don't have the time to break these up into dozens of individual bug reports, and doubt that would really be helpful anyway! Still, if there is a better place to put this, I'm open to suggestions.

And if any collaborator feels like editing this to remove things that are already done, or perhaps to open a specific issue or PR for it, please feel free.

---

doc

• tls.renegotiate(): doc that callback is added as a listener to the 'secure' event

pretty sure the 4x increase is just due to 4-thread work pool

• https://nodejs.org/en/docs/guides/simple-profiling/

• doc all HPE errors from http_parser in docs/errors.js (only HPE_HEADER_OVERFLOW
  is there now)

• udp: it would be helpful for each method that cannot be called until the
  socket is listening to explicitly mention that in its documentation.

• https://github.com/nodejs/node/pull/14631/files

  • describe better how file position works, about unix fd model, OCBs, etc.,
    current docs assume a basic familiarity with unix i/o

• IncomingMessage.connection is not documented, .socket is not documented
  either as to whether it is a Socket or a TLSSocket

• closed PR to doc the process.platform: doc: Update docs for os.platform() #2446

• http docs, and additional apis: http.ClientRequest poor documentation #2461 (comment)

• cluster.fork() asserts in child, but should have a message, false == true is not so useful

• cluster.setupMaster and child_process.fork both support execArgv, but it is
  not documented for either, is this intentional?

• doc: process.on(disconnect or process.on(message causes process to be refed

• _write is linked in stream docs, _read isn't, fix in
  https://nodejs.org/api/stream.html#stream_buffering

• tls newSession should have backwards compat note about when callback was
  introduced

doc https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback

• write(chunk, encoding, cb) encoding can be null
• undocumented... probably defaults to utf8

OCSP docs...

• https://nodejs.org/api/tls.html#tls_event_ocsprequest
• docs unclear, I think the "issuer" is actually the root CA, not the direct
  issuer/intermediate CA
• step 5 likely involves OCSPResponse being emitted as an event, not said
• says asn1.js can be used to parse... but is it possible that the OCSP info
  in https://nodejs.org/api/tls.html#tls_certificate_object is all that is
  needed, and is already available?
• https://nodejs.org/api/tls.html#tls_new_tls_tlssocket_socket_options
• requestOCSP : not clear if it does anything when called on the
  server side, probably not. should say that presence of the extension will
  cause OCSPRequest event on server side (if server side is node)
• https://nodejs.org/api/tls.html#tls_event_ocspresponse
• lacking any info on what user is supposed to do with the response
• client is supposed to be able to return a value in the cb to fail hankshake:
  • https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_tlsext_status_cb.html

dns

src/cares_wrap.cc: looks like instead of .code, what would be the code is
thrown as the message.

http

HTTP EPIPE by loopback, can core do anything about this?

• Missing response for certain POST requests #12339

process

implement process.std(out/in/err).reopen(): so that it's possible, as in many
languages

• 'process.stdout cannot be closed': process.stdout should behave like a standard stream #7606 (comment)

child_process

remove maxBuffer default

• maxBuffer default too small #9829 (comment)

• document how to make a pipe on non-stdio... then make it easier

net

• net.connect() - no args does ECONNREFUSED but should be invalid usage

  • I think I commented on this, and fixed it in backports, and it was thought
    that net.connect() should be same as net.connect({port:undefined}), but I
    don't agree, it never makes sense to connect when you don't say what you are
    connecting to, it has no use case. also strange that undefined works like no
    args were passed, but null is like {port: null} was passed. This all
    seems ugly and messy. Check the tests.

• close handling bizarre: .... this may have been cleaned up now

  • server.close(cb), cb gets an error if net is already closed... this is
    bizarre and inconsistent:
    • doc: server close event does not have an argument #1025
  • not clear why close callback is not .once('close'), very unusual! changing
    might be backwards incompat, need to consider how it should work. Why
    doesn't close event get an err arg? there are already conditions for that,
    unread data, etc, I think

    t=tls.createServer({});
    t.close(function(){
    console.log('close cb',arguments)
    });
    t.on('close',function(){console.log('close ev', arguments) });
    close cb { '0': [Error: Not running] }
    close ev {}

tls

use SSL_OP_NO_RENEGOTIATION to disable renegotiation(), instead of
the info callback thing we are doing internally

• Prior to SSL_OP_NO_RENEGOTIATION (new in the same release that added 1.3)

finish and land this, simple API consistency:

• crypto: add ticketKeys option in createSecureContext #20916

deprecate tlsSocket.getSession() once TLS1.3 is more common

• 16.x?

Server.prototype.addContext should be case insensitive, but it probably is not,
confirm and fix if necessary

• https://tools.ietf.org/html/rfc6066#section-3
• https://tools.ietf.org/html/rfc5890#section-2.3.2.4

servername must not be an IP address:

• errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988
• should have negative tests
• should throw better errors
• should actually check that arg is not an IP address (semver-major)

perhaps done now?

• c51b7b2
• TODO(shigeki) Change this to EVP_PKEY_X25519 and add EVP_PKEY_X448 after
  upgrading to 1.1.1.

make addr for SNI deprecation an actual thrown error

• Don't set SNI by default if hostname is not dNS name; fixes #8083 openssl/openssl#8175 (comment)
• needs first to have a runtime deprecation that checks the SNI arg
• needs to check client and server side

I believe sessions created by renegotiation will never cause newSession to be
emitted server side, since the ClientHello parser won't see them. Maybe I'm
wrong, or maybe nobody has noticed or wanted the feature? Could be that people
are getting poorer performance and not noticing.

X509ToObject should pull DH key info out, it ignores it now

would be nice to have tls.constants... not just crypto.constants

DOC authorized is false on server if there was no client cert requested, but
there will be no authorizationError, because no cert was evaluated

server.addContext takes the options for createSecureContext, but not an actual
SecureContext, which appears to be just an oversight because a user-provided
SNICallback can return a SecureContext.

test letsencrypt with node, seems to be some problems

• SSL core dump in v6.9.1 #9551 (comment)

tls.TLSSocket creation needs to do full setup, to implement documented API
fix tls.Socket constructor, so that it behaves as documented, in that it
connects up the events

• doc: tls API for direct TLS socket use #10846 for attempt to doc current API

external users of undocumented TLS APIs:

• https://github.com/eleith/emailjs/blob/master/smtp/smtp.js#L263-L289

• docs and code don't appear to be correct mattcg/starttls#3

• DTLS:

  • DTLS Discussion #2398
  • https://github.com/Rantanen/node-dtls

tls requires a subject even when altNames are defined

• tls requires a subject even when altNames are defined #22906 (comment)

crypto

expose expected iv/key sizes for crypto algs:

• benchmark: replace deprecated cipher in cipher-stream.js #26612 (comment)

allow key object args to key object create functions, returning identity (done?)

sys random: #5798

• Was blocked on ossl 1.1.1, should be possible to do very soon:
  Use system random generator in crypto #5798 (comment)
• open question: can the OpenSSL internal PRNG be replaced?
• http://man7.org/linux/man-pages/man2/getrandom.2.html
• https://bugs.ruby-lang.org/issues/9569
• https://media.ccc.de/v/32c3-7441-the_plain_simple_reality_of_entropy
• https://bugzilla.kernel.org/show_bug.cgi?id=71211
• Make RAND_bytes a transparent wrapper for /dev/urandom openssl/openssl#898
• https://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/
• AEADs like AES-GCM and ChaCha20-Poly1305A
• https://www.imperialviolet.org/posts-index.html
  • https://www.imperialviolet.org/2011/02/04/oppractices.html

IV should be optional if unused (ECB), not force a zero-length buffer:

• Initialization Vector length in createCipheriv/createeciperiv for aes-128-ecb #10263 (comment)
  • If the cipher does not need an initialization vector, iv may be null.
    crypto.createCipheriv("AES-128-ECB", "xxxxxxxxxxxxxxxx", Buffer.alloc(0))
    crypto.createCipheriv("AES-128-ECB", "xxxxxxxxxxxxxxxx")
    Especially now that createCipher() is deprecated.

accept PEM&DER everywhere possible:

• DiffieHellman/ECDH Generates invalid keypairs #14628 (comment)
• difficulty needs research, I think there were suggestions its hard to guess
  the format of whatever is passed, but I'm not sure that is true

PFX api in node, so that CA certs can be parsed from a PFX/p12 file
-crypto does not expose the ossl p12 API

Layne Miller when i wanted to do similar (create .p12) i could not find a
suitable module, ended up with exec('openssl ...', callback);

• ouch!

Sign.maximumSignatureSize()

• keyObject.asymmetricKeySize returns incorrect values for EC and Ed keys #26631 (comment)

consistently rename socket to tlsSock in lib/_tls_wrap.js

Return other info for ::GetCipher(), like

• SSL_CIPHER_is_aead(), SSL_CIPHER_standard_name, ...

research 0-RTT

• 0-RTT ... is it supported by openssl? can it be disabled? it allows replay

  • should not allow non-GET with 0-rtt, how to tell from API? How to reject?
  • https://blog.cloudflare.com/introducing-0-rtt/
  • https://tools.ietf.org/html/rfc8446#section-8
  • https://tools.ietf.org/html/rfc8446#section-8.1
    • how do we get the ticket, to store, and ensure not used multiple times?
      maybe store it in the ticketcbs, as we encrypt it, and check it when we
      decrypt it? necessary if not using 0-rtt?
  • https://tools.ietf.org/html/rfc8446#section-8.2
    • how to implement this?

• forward secrecy

• https://blog.cloudflare.com/rfc-8446-aka-tls-1-3/

• groups: might need configuring, do we support that?

  • https://wiki.openssl.org/index.php/TLS1.3#Groups

• certificate transparency (CT)/serverinfo API, do we support? should we?

  • https://wiki.openssl.org/index.php/TLS1.3#Custom_Extensions_and_Certificate_Transparency
  • https://www.openssl.org/docs/manmaster/man3/SSL_CTX_use_serverinfo.html
  • https://en.wikipedia.org/wiki/Certificate_Transparency#Certificate_authority_implementation

• could support disabling middlebox compatibility mode, but probably don't want
  to unless someone explicitly asks for this

  • https://wiki.openssl.org/index.php/TLS1.3#Middlebox_Compatibility_Mode

• node clients should DEFAULT to using SNI: does node with https? (yes) tls.connect? (no)

  • https://wiki.openssl.org/index.php/TLS1.3#Server_Name_Indication
  • should doc this

• x25519: can it not be used for ECDSA? its not showing up in
  tls/Client_Hello/Signature_Algorithms

fix test/parallel/test-tls-client-getephemeralkeyinfo.js to do TLSv1.3

Renegotiation: https://wiki.openssl.org/index.php/TLS1.3#Renegotiation

Q: can the existing renegotiate() API be partially implemented in terms of
these new APIs, or should there just be new APIs? Its hard for a user, because
they would need to first check the protocol that was negotiated, then decide
what APIs they have to call. This problem even seems to have occurred to
OpenSSL :-(

- https://github.com/openssl/openssl/blob/fff1470cd/ssl/ssl_lib.c#L2104-L2106

Perhaps things like ca/etc in renegotiate() can be set as they are now, and
then key update and/or cert req can be made depending on the options?

I haven't seen code that calls renegotiate on the server side, but its supported

• https://www.openssl.org/docs/manmaster/man3/SSL_renegotiate.html
• Confirm that both sides allow it for TLS1.2.

key update is SSL_key_update

• https://www.openssl.org/docs/manmaster/man3/SSL_key_update.html
• can be called from either side
• I assume it would trigger new session tickets to be sent?
  • XXX verify this!
• https://www.mail-archive.com/tls@ietf.org/msg10202.html
  Therefore KeyUpdate messages are not currently viable on the web, at least
  when client initiated.

request a certificate from the client post-handshake

• SSL_verify_client_post_handshake
• https://www.openssl.org/docs/manmaster/man3/SSL_verify_client_post_handshake.html
• specifically mentioned in node docs:
  • https://nodejs.org/api/tls.html#tls_tlssocket_renegotiate_options_callback
• XXX what does this look like from the API? How do we know a verification has
  occurred? I assume some cert cbs will be re-called...
  • XXX verify this!

Implement a new tls.sessionInfo(sess):

• getSessionInfo() doesn't work anymore, this replacement is needed
• hasTicket: SSL_SESSION_has_ticket
• resumable: SSL_SESSION_is_resumable
• ticket: SSL_SESSION_get0_ticket
• ticketLifetime: SSL_SESSION_get_ticket_lifetime_hint
• id: get_session_id
• ...: ?

expose key lifetime control

implement createServer({numTickets: })

• https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_num_tickets.html

for setTicketKeys change to non-fixed size keys

SSL_OP_NO_TICKET doesn't disable tickets in TLS1.3, it does "stateful" tickets -
what are these for? Do they "work"? It seems they should trigger
newSession/resumeSession callbacks, but I haven't checked that they do.

• https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_options.html

ticket key management callbacks

• Currently, only one set of ticket crypto keys is supported at a time, but
  this means roll over will trigger rehandshake. Could/should make this
  callback based.

• https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_tlsext_ticket_key_cb

  Postfix keeps two session ticket keys in memory, one that's used to
  both encrypt new tickets and decrypt freshly issued tickets, and other
  that's used only decrypt unexpired tickets that were isssued just before the
  new key was introduced. This maintains session ticket continuity across a
  single key change. The key change interval is either equal to or is twice the
  maximum ticket lifetime, ensuring that tickets are only invalidated by
  expiration, not key rotation.

  cloudfare does something similar:

  • https://blog.cloudflare.com/tls-session-resumption-full-speed-and-secure/

optionally need ticket key callback for named ticket keys

• Not having them means accidental invalidation of tickets after setTicketKeys
• SSL_CTX_set_tlsext_ticket_key_cb

merge tests with test-tls-auth.js, or otherwise clean them up?

• rename test-tls-client-auth to something more indicative
• test/parallel/test-tls-ca-concat.js
• test/parallel/test-tls-cert-chains-concat.js
• test/parallel/test-tls-cert-chains-in-ca.js
• cert.split(/(?=-----BEGIN CERTIFICATE-----)/)
• maybe test/parallel/test-tls-cert-regression.js
  • ^--- add tests for buffer format args

cluster:

make kill NOT do a disconnect, just be childprocess.kill

• discussion: Cluster worker.kill(signal) does not pass the signal parameter node-v0.x-archive#6042
• kill rename:
  • cluster: Rename destroy() to kill(signal=SIGTERM) node-v0.x-archive#4908
  • cluster: rename worker.destroy() to worker.kill() again node-v0.x-archive#4133
• discussion for rename: cluster: rename worker.destroy() to worker.kill() again node-v0.x-archive#4133 (comment)
• worker.kill(SIG)... SIG is not normally received
  • cluster/child_process exit event does not pass signal if child is killed node-v0.x-archive#5832 (comment)
  • Cluster worker.kill(signal) does not pass the signal parameter node-v0.x-archive#6042
  • Cluster API: worker process can't get 'SIGTERM' signal when being killed node-v0.x-archive#5766

[sagar-jadhav]

Hello, @sam-github I want to work on the following issue:

merge tests with test-tls-auth.js, or otherwise clean them up?

rename test-tls-client-auth to something more indicative
test/parallel/test-tls-ca-concat.js
test/parallel/test-tls-cert-chains-concat.js
test/parallel/test-tls-cert-chains-in-ca.js
cert.split(/(?=-----BEGIN CERTIFICATE-----)/)
maybe test/parallel/test-tls-cert-regression.js
^--- add tests for buffer format args

Let me tell you what I have understood:

• Merge all tests from test/parallel/test-tls-ca-concat.js, test/parallel/test-tls-cert-chains-concat.js & test/parallel/test-tls-cert-chains-in-ca.js into single test file test-tls-client-auth

But I didn't understand the following:

cert.split(/(?=-----BEGIN CERTIFICATE-----)/)
maybe test/parallel/test-tls-cert-regression.js
^--- add tests for buffer format args

@sam-github Can you please guide me here?

[tarunbatra]

@sam-github I want to take up #9829 (comment) issue to remove maxBuffer default in child_process. Can you confirm if we want to remove the circuit breaker entirely or set it to a very high value?

[PoojaDurgad]

@aduh95 - is all the items in this issue are covered or not? looks like there are many issues which needs to cover. I'm reopening this issue .let me know if that is not the case.

[VikiD]

Following the topic, i could suggest you to read this great article
https://os-system.com/blog/12-types-of-node-js-applications-with-examples/