feat: implement https server support #602

bliuchak · 2025-10-08T10:51:02Z

This PR implements HTTPS server support to proxy-chain.

Also fixed:

a datarace for error event when we might log same events
fix for usage statistics trackings

Otherwise my changes should be fully compatible with HTTP server and all the handlers.

Readiness checklist:

- Fix a datarace for error handler - Add a regression test that verify datarace fix - Add TLS defaults for better security

jirimoravcik · 2025-10-09T09:16:00Z

Also fixed:

a datarace for error event when we might log same events

fix for usage statistics trackings

Could you please point me to the changes that are related to the fixes? Thanks. Also, what was wrong with the statistics?

bliuchak · 2025-10-09T09:32:13Z

Also fixed:

a datarace for error event when we might log same events

fix for usage statistics trackings

Could you please point me to the changes that are related to the fixes? Thanks. Also, what was wrong with the statistics?

Also, what was wrong with the statistics?

Don't remember right now for 100%, but few tests failed for https scenarios. I believe there was some issues related with undefined values for statistics.

jirimoravcik

Looks good, had a few comments.
In addition to that, could you please:

Bump the package version
Describe all the new things in README.md (which serves as the primary user-facing documentation)
Thanks

test/utils/certificate_generator.js

src/server.ts

tsconfig.json

src/server.ts

Aslo revert changes in eslint and tsconfig

bliuchak · 2025-10-21T15:29:19Z

@jirimoravcik @lewis-wow Guys, I've added main logic for TLS overhead bytes. Please take a look 🙏

Gonna polish tests in meantime and push 'em ASAP.

jirimoravcik · 2025-10-21T16:16:04Z

src/server.ts

+
+        // Check once per connection for socket._parent availability.
+        if (this.serverType === 'https') {
+            const rawSocket = socket._parent;


It may be worth asking in https://github.com/nodejs/node about the safety of using this private property.

I guess I know the answer :) as long as unit tests cover the eventuality this will get removed, I think we're good

It’s not just about the presence of the _parent property, but about the overall usability for stats tracking. I mean, if you consider yourself an expert on the TLS implementation in Node.js, that’s great. :)

fyi: nodejs/help#5111 🤞

lewis-wow

Nice! Have nothing to add.

jirimoravcik

Found a few more points for discussion

jirimoravcik · 2025-10-27T08:53:37Z

src/server.ts

+        if (options.serverType === 'https') {
+            if (!options.httpsOptions) {
+                throw new Error('httpsOptions is required when serverType is "https"');
+            }
+
+            // Apply secure TLS defaults (user options can override)
+            // This prevents users from accidentally configuring insecure TLS settings
+            const secureDefaults: https.ServerOptions = {
+                ...HTTPS_DEFAULTS,
+                honorCipherOrder: true, // Server chooses cipher (prevents downgrade attacks)
+                ...options.httpsOptions, // User options override defaults
+            };
+
+            this.server = https.createServer(secureDefaults);
+            this.serverType = 'https';
+        } else {
+            this.server = http.createServer();
+            this.serverType = 'http';
+        }


Maybe validate if options.serverType is one of http, https? It would make it consistent with the type. I'd also set it to http by default in the constructor parameter. That way you could just do this.serverType = options.serverType

Good point. Gonna add this.

jirimoravcik · 2025-10-27T08:55:49Z

src/server.ts

+            socket.proxyChainErrorHandled = true;
+
+            // Log errors only if there are no user-provided error handlers
+            if (this.listenerCount('error') === 0) {


There was === 1 before, why is it === 0 now?

If I understand it correctly for previous condition this.listenerCount('error') === 1 (which count server-listeners, not socket) app log error only when there is one server handler. Right?

If I'm correct, then in this case the error will be handled by that one server-handler. For us it might be useful to log here when there are no other server-handlers this.listenerCount('error') === 0.

What do you think?

jirimoravcik · 2025-10-27T09:08:51Z

src/server.ts

+        // Socket contains application bytes only.
+        let srcTxBytes = socket.bytesWritten ?? 0;
+        let srcRxBytes = socket.bytesRead ?? 0;
+
+        if (this.serverType === 'https' && socket.tlsOverheadAvailable) {
+            /* eslint no-underscore-dangle: ["error", { "allow": ["_parent"] }] */
+            // Access underlying raw socket to get total bytes (app + TLS overhead).
+            const rawSocket = socket._parent;
+            if (rawSocket && typeof rawSocket.bytesWritten === 'number' && typeof rawSocket.bytesRead === 'number') {
+                if (rawSocket.bytesWritten >= socket.bytesWritten && rawSocket.bytesRead >= socket.bytesRead) {
+                    srcTxBytes = rawSocket.bytesWritten;
+                    srcRxBytes = rawSocket.bytesRead;
+                } else {
+                    // This should never happen, log for debugging.
+                    this.log(connectionId, `Warning: TLS overhead count error.`);
+                }
+            }
+        }
+
        const targetStats = getTargetStats(socket);

-        const result = {
-            srcTxBytes: socket.bytesWritten,
-            srcRxBytes: socket.bytesRead,
-            trgTxBytes: targetStats.bytesWritten,
-            trgRxBytes: targetStats.bytesRead,
+        return {
+            srcTxBytes, // HTTP: app only, HTTPS: total (app + TLS overhead)
+            srcRxBytes, // HTTP: app only, HTTPS: total (app + TLS overhead)
+            trgTxBytes: targetStats?.bytesWritten,
+            trgRxBytes: targetStats?.bytesRead,
        };


Just an idea. Why don't we store the _parent socket in this.connections? Looking at the logic here in getConnectionsStats, you just fall back to the original socket for stats. That makes me question why would we ever want to use the TLS socket for connection tracking?
That gives me another idea, if you do this.server.on('connection') for HTTPS you should be able to reach the original Socket without using _parent, right?

Just an idea. Why don't we store the _parent socket in this.connections? Looking at the logic here in getConnectionsStats, you just fall back to the original socket for stats. That makes me question why would we ever want to use the TLS socket for connection tracking?

tl;dr: It's possible, however it creates more isusues than it solves.

HTTPS servers create two separate JavaScript objects:

Raw socket (TCP layer) - created first

TLS socket (wraps raw) - created after handshake

Nodejs HTTP events (request, connect) always give us the TLS socket (for HTTPS server). This is the same object handlers receive and attach metadata to (proxyChainId, Symbols).

If we stored raw socket in connections:

Different object identity (raw vs TLS socket) results in all metadata lookups failing (proxyChainId undefined, Symbol not found)

Need to maintain raw to TLS mapping because of Symbol lookup in getConnectionStats() (this func will get raw socket from this.connections, but stats are tracked for TLS socket in handlers)

Event handlers are attached to raw socket in registerConnection() (close, timeout) and onConnection() (error). However nodejs for https will emit these events on TLS socket.

We store the socket that HTTP handlers actually use. Accessing _parent once for stats is simpler than complex socket bridging everywhere else.

That gives me another idea, if you do this.server.on('connection') for HTTPS you should be able to reach the original Socket without using _parent, right?

Yes, we can obtain raw socket directly via connection event, but we still need both sockets since TLS socket is used in handlers. In this case, instead of accessing tlsSocket._parent once for stats, we'll need to create and maintain bidirectional mapping between raw and TLS sockets.

bliuchak added 4 commits October 8, 2025 10:52

feat: add https server implementation

313f535

fix: add onTLSClientError handler

e6adb19

- Fix a datarace for error handler - Add a regression test that verify datarace fix - Add TLS defaults for better security

fix: add https edge case tests

0158e0c

fix: resolve broken test with raw TCP and TLS

44fc388

bliuchak requested review from jirimoravcik and lewis-wow October 8, 2025 10:51

github-actions bot assigned bliuchak Oct 8, 2025

github-actions bot added this to the 125th sprint - Core Services team milestone Oct 8, 2025

github-actions bot added t-core-services Issues with this label are in the ownership of the core services team. tested Temporary label used only programatically for some analytics. labels Oct 8, 2025

bliuchak added the t-unblocking Issues with this label are in the ownership of the unblocking team. label Oct 8, 2025

bliuchak added 2 commits October 8, 2025 14:36

fix: skip fixture integrity checks for older node versions

8893eca

fix: beter dockerfile

57cb93e

jirimoravcik requested changes Oct 9, 2025

View reviewed changes

test/utils/certificate_generator.js Show resolved Hide resolved

src/server.ts Outdated Show resolved Hide resolved

src/server.ts Show resolved Hide resolved

lewis-wow requested changes Oct 9, 2025

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

src/server.ts Outdated Show resolved Hide resolved

bliuchak added 5 commits October 10, 2025 15:40

chore: bump minor version

9956192

chore: extract https defaults into separate const

e5a1171

docs: add info about https server and other related stuff

8a90e7e

refactor: rewrite https example to js for consistency

de12161

Aslo revert changes in eslint and tsconfig

fix: export few user facing interfaces

4c036a0

bliuchak requested review from jirimoravcik and lewis-wow October 13, 2025 14:27

jirimoravcik approved these changes Oct 14, 2025

View reviewed changes

lewis-wow approved these changes Oct 14, 2025

View reviewed changes

fix: add tls overhead bytes into final statistic

934693a

bliuchak requested review from jirimoravcik and lewis-wow October 21, 2025 15:27

fix: better comment

597b942

jirimoravcik reviewed Oct 21, 2025

View reviewed changes

lewis-wow approved these changes Oct 22, 2025

View reviewed changes

jirimoravcik requested changes Oct 27, 2025

View reviewed changes

lewis-wow mentioned this pull request Oct 31, 2025

feat: add requestFinished event for per request tracking #604

Draft

feat: implement https server support #602

Are you sure you want to change the base?

feat: implement https server support #602

Uh oh!

Conversation

bliuchak commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jirimoravcik commented Oct 9, 2025

Uh oh!

bliuchak commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jirimoravcik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bliuchak commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bliuchak Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lewis-wow left a comment

Choose a reason for hiding this comment

Uh oh!

jirimoravcik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bliuchak Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bliuchak commented Oct 8, 2025 •

edited

Loading

bliuchak commented Oct 9, 2025 •

edited

Loading

bliuchak Oct 23, 2025 •

edited

Loading

bliuchak Nov 3, 2025 •

edited

Loading