Skip to content

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

New issue
New issue
Closed
#35372
Closed
A possible TODO list for new (or current) contributors#33715
#35372
Labels
cryptoIssues and PRs related to the crypto subsystem.good first issueIssues that are suitable for first-time contributors.
@sam-github

Description

@sam-github
sam-github
opened on 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: 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...

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

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:

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

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:

accept PEM&DER everywhere possible:

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    cryptoIssues and PRs related to the crypto subsystem.good first issueIssues that are suitable for first-time contributors.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions