The Wayback Machine - https://web.archive.org/web/20200831160354/https://github.com/nodejs/node/issues/33715
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

A possible TODO list for new (or current) contributors #33715

Open
sam-github opened this issue Jun 3, 2020 · 2 comments
Open

A possible TODO list for new (or current) contributors #33715

sam-github opened this issue Jun 3, 2020 · 2 comments

Comments

@sam-github
Copy link
Contributor

@sam-github sam-github commented Jun 3, 2020

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: #2446

  • http docs, and additional apis: #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...

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?

process

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

child_process

remove maxBuffer default

  • #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:
    • 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:

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

servername must not be an IP address:

  • #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

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

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

  • #10846 for attempt to doc current API

external users of undocumented TLS APIs:

tls requires a subject even when altNames are defined

crypto

expose expected iv/key sizes for crypto algs:

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

sys random: #5798

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

  • #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:

  • #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()

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

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

key update is SSL_key_update

request a certificate from the client post-handshake

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: })

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.

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:

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

rexagod added a commit to rexagod/node that referenced this issue Jun 6, 2020
Refs: nodejs#33715
rexagod added a commit to rexagod/node that referenced this issue Jun 6, 2020
Refs: nodejs#33715
@rexagod rexagod mentioned this issue Jun 6, 2020
3 of 3 tasks complete
@rexagod rexagod mentioned this issue Jun 6, 2020
2 of 2 tasks complete
@zhangwinning zhangwinning mentioned this issue Jun 9, 2020
4 of 4 tasks complete
@sagar-jadhav
Copy link
Contributor

@sagar-jadhav sagar-jadhav commented Jun 10, 2020

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?

jasnell added a commit that referenced this issue Jun 19, 2020
Refs: #33715

PR-URL: #33765
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell added a commit that referenced this issue Jun 19, 2020
Refs: #33715

PR-URL: #33767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit that referenced this issue Jun 22, 2020
Refs: #33715

PR-URL: #33765
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit that referenced this issue Jun 22, 2020
Refs: #33715

PR-URL: #33767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit that referenced this issue Jun 30, 2020
Refs: #33715

PR-URL: #33765
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit that referenced this issue Jun 30, 2020
Refs: #33715

PR-URL: #33767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rexagod added a commit to rexagod/node that referenced this issue Jul 4, 2020
dev-script added a commit to dev-script/node that referenced this issue Jul 4, 2020
rebase master branch & resolve conflicts in stream.md file

Fixes: nodejs#33715
tniessen added a commit to tniessen/node that referenced this issue Jul 10, 2020
Refs: nodejs#33715
codebytere added a commit that referenced this issue Jul 10, 2020
Refs: #33715

PR-URL: #33765
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit that referenced this issue Jul 10, 2020
Refs: #33715

PR-URL: #33767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
baradm100 added a commit to baradm100/node that referenced this issue Jul 11, 2020
Make `Worker.prototype.kill` to be just process.kill without preforming graceful disconnect beforehand.
Refs: nodejs#33715
codebytere added a commit that referenced this issue Jul 14, 2020
Refs: #33715

PR-URL: #33765
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit that referenced this issue Jul 14, 2020
Refs: #33715

PR-URL: #33767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
tniessen added a commit that referenced this issue Jul 19, 2020
Refs: #33715

PR-URL: #34294
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
cjihrig added a commit that referenced this issue Jul 23, 2020
Refs: #33715

PR-URL: #34294
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this issue Jul 27, 2020
Refs: #33715

PR-URL: #34294
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@tarunbatra
Copy link
Contributor

@tarunbatra tarunbatra commented Aug 15, 2020

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.