Use IP certificates #1688

fortuna · 2026-01-02T15:55:45Z

No description provided.

- Add `acme-client` dependency to `src/shadowbox`. - Implement `CertificateManager` to automate ACME HTTP-01 challenges on port 80. - Integrate `CertificateManager` into `main.ts` for zero-downtime certificate renewal via `setSecureContext`. - Update `install_server.sh` to ensure write permissions for certificate updates. - Update Node.js engine requirement to `>=18.0.0` for `crypto.X509Certificate` support.

- Refactor `CertificateManager` to use a factory function (`createCertificateManager`) for dependency injection of the ACME client. - Update `main.ts` to use the factory and initialize certificate management asynchronously. - Remove unused dependencies (`node-fetch`, `mkdirp`) from `src/shadowbox`. - Ensure correct ACME client usage (v5 API). - Update `install_server.sh` to grant write permissions to the persisted state directory for certificate updates. - Update Node.js engine requirement to `>=18.0.0` for `crypto.X509Certificate` support.

- Refactor `CertificateManager` to use a factory function (`createCertificateManager`) for dependency injection of the ACME client. - Update `main.ts` to use `async/await` within an IIFE for cleaner initialization of the certificate manager, handling errors gracefully without blocking startup. - Remove unused dependencies (`node-fetch`, `mkdirp`) from `src/shadowbox`. - Ensure correct ACME client usage (v5 API). - Update `install_server.sh` to grant write permissions to the persisted state directory for certificate updates. - Update Node.js engine requirement to `>=18.0.0` for `crypto.X509Certificate` support.

src/shadowbox/server/certificate_manager.ts

+  } else {
+    logging.info('Generating new ACME account key...');
+    accountKey = await acme.crypto.createPrivateKey();
+    fs.writeFileSync(accountKeyPath, accountKey);


Copilot

Pull request overview

This PR adds automatic TLS certificate management using the ACME protocol to support IP-based certificates from Let's Encrypt. The implementation introduces a new CertificateManager class that handles certificate issuance, renewal, and hot-reloading without server downtime.

Key changes:

Implements automated certificate lifecycle management with periodic renewal checks
Adds HTTP-01 challenge server for ACME validation
Integrates with existing server infrastructure for seamless certificate updates

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
src/shadowbox/server/certificate_manager.ts	New module implementing ACME-based certificate management with automatic renewal and HTTP-01 challenge handling
src/shadowbox/server/main.ts	Integrates certificate manager with async initialization pattern that doesn't block server startup
src/shadowbox/package.json	Adds acme-client dependency and updates Node.js types to v18 along with testing dependencies
src/server_manager/install_scripts/install_server.sh	Ensures state directory has proper write permissions for certificate storage
package.json	Relaxes Node.js version constraint from exact 18.x.x to >=18.0.0
package-lock.json	Updates lock file with new dependencies including ACME client and cryptographic libraries

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-02T16:01:43Z

src/shadowbox/server/certificate_manager.ts

+  } else {
+    logging.info('Generating new ACME account key...');
+    accountKey = await acme.crypto.createPrivateKey();
+    fs.writeFileSync(accountKeyPath, accountKey);


The ACME account key file is created with default permissions using fs.writeFileSync, which may result in world-readable permissions depending on the umask. Private cryptographic keys should be protected with restricted permissions (e.g., 0600) to prevent unauthorized access. Use fs.writeFileSync with appropriate mode option or call fs.chmodSync after writing to ensure the key file is only readable by the owner.

Suggested change

fs.writeFileSync(accountKeyPath, accountKey);

fs.writeFileSync(accountKeyPath, accountKey, {mode: 0o600});

Copilot · 2026-01-02T16:01:44Z

src/shadowbox/server/certificate_manager.ts

+      // Save to disk
+      logging.info('Certificate obtained successfully. Saving...');
+      fs.writeFileSync(this.certFile, cert);
+      fs.writeFileSync(this.keyFile, key);


The private key file is written with default permissions using fs.writeFileSync, which may result in world-readable permissions depending on the umask. Private keys must be protected with restricted permissions (e.g., 0600) to prevent unauthorized access and maintain server security. Use fs.writeFileSync with the mode option set to 0o600 or call fs.chmodSync immediately after writing.

Suggested change

fs.writeFileSync(this.keyFile, key);

fs.writeFileSync(this.keyFile, key, { mode: 0o600 });

Copilot · 2026-01-02T16:01:44Z

src/shadowbox/server/certificate_manager.ts

+      const order = await this.client.createOrder({
+        identifiers: [{ type: 'ip', value: this.hostname }],


There's no check to verify that the 'hostname' parameter is a valid IP address before attempting to create an ACME order with type 'ip'. If a hostname (DNS name) is passed instead of an IP address, the ACME order will likely fail. Consider adding validation to ensure the hostname is a valid IP address format, or provide clearer error messaging when the ACME order fails due to invalid identifier type.

Copilot · 2026-01-02T16:01:44Z

src/shadowbox/server/certificate_manager.ts

+    setInterval(() => {
+      this.checkAndRenew().catch((e) => {
+        logging.error(`Scheduled certificate renewal failed: ${e}`);
+      });
+    }, CHECK_INTERVAL_MS);


The setInterval call is not stored in a variable, which means there's no way to clear this interval if the CertificateManager needs to be stopped or shut down gracefully. This creates a resource leak where the interval continues running even if the manager is no longer needed. Consider storing the interval ID and providing a stop/cleanup method to clear it when needed.

Copilot · 2026-01-02T16:01:45Z

src/shadowbox/server/certificate_manager.ts

+      challengeServer.close();
+      logging.info('ACME challenge server closed.');


The challengeServer.close() call in the finally block doesn't wait for the server to fully close before continuing. The http.Server.close() method is asynchronous and takes an optional callback. If an error occurs during certificate renewal and the server hasn't fully closed yet, this could leave the port bound or cause issues with subsequent renewal attempts. Consider wrapping this in a promise to ensure the server is fully closed before exiting the function.

Suggested change

challengeServer.close();

logging.info('ACME challenge server closed.');

await new Promise<void>((resolve) => {

challengeServer.close(() => {

logging.info('ACME challenge server closed.');

resolve();

});

});

Copilot · 2026-01-02T16:01:45Z

src/shadowbox/server/certificate_manager.ts

+        // IP certificates usually don't need SANs if CN is IP, or maybe they do?
+        // Let's Encrypt for IP requires the IP in SANs.


The comment suggests uncertainty about whether IP certificates need Subject Alternative Names (SANs), followed by a statement that Let's Encrypt requires them. This creates confusion about the implementation. Either remove the uncertain first sentence or rephrase to clearly state the requirement without expressing doubt, as the code already implements the correct behavior.

Suggested change

// IP certificates usually don't need SANs if CN is IP, or maybe they do?

// Let's Encrypt for IP requires the IP in SANs.

// For Let's Encrypt IP certificates, the IP address must be included in the Subject Alternative Names (SANs).

Copilot · 2026-01-02T16:01:45Z

src/shadowbox/server/certificate_manager.ts

+    try {
+      // Start server on port 80
+      await new Promise<void>((resolve, reject) => {
+        challengeServer.listen(80, '0.0.0.0', () => {


The challenge server binds to 0.0.0.0 which makes it accessible from all network interfaces. While this is necessary for ACME HTTP-01 challenges to work, it temporarily exposes the server on port 80 to the public internet during certificate renewal. Consider adding rate limiting or request validation to prevent potential abuse during the challenge period, or document this security consideration.

Copilot · 2026-01-02T16:01:45Z

src/shadowbox/server/certificate_manager.ts

+      if (e.code === 'EADDRINUSE') {
+        logging.error('Port 80 is in use. Cannot perform HTTP-01 challenge.');
+      }


The error handler only checks for the 'code' property on the error object, but TypeScript errors may not have this property. The code assumes 'e.code' exists without proper type checking. Consider using type guards or optional chaining (e?.code) to safely access this property, or cast the error to a NodeJS.ErrnoException type that has the code property.

Copilot · 2026-01-02T16:01:46Z

src/shadowbox/server/certificate_manager.ts

+      } as any);
+      /* eslint-enable @typescript-eslint/no-explicit-any */
+
+      const authorizations = await this.client.getAuthorizations(order);


The code assumes that authorizations[0] will always exist without checking if the authorizations array is non-empty. If the ACME server returns an empty authorizations array, this will cause a runtime error when trying to access properties on undefined. Add a check to verify the array has at least one element before accessing authorizations[0].

Suggested change

const authorizations = await this.client.getAuthorizations(order);

const authorizations = await this.client.getAuthorizations(order);

if (!authorizations || authorizations.length === 0) {

throw new Error('No authorizations returned from ACME server');

}

google-labs-jules bot added 3 commits December 29, 2025 18:48

github-actions bot added the size/M label Jan 2, 2026

fortuna requested a review from Copilot January 2, 2026 15:56

Copilot started reviewing on behalf of fortuna January 2, 2026 15:56 View session

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

Copilot AI reviewed Jan 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use IP certificates #1688

Use IP certificates #1688

Uh oh!

fortuna commented Jan 2, 2026

Uh oh!

Check failure

Copilot AI left a comment

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	fs.writeFileSync(accountKeyPath, accountKey);
	fs.writeFileSync(accountKeyPath, accountKey, {mode: 0o600});

	fs.writeFileSync(this.keyFile, key);
	fs.writeFileSync(this.keyFile, key, { mode: 0o600 });

		const order = await this.client.createOrder({
		identifiers: [{ type: 'ip', value: this.hostname }],

		challengeServer.close();
		logging.info('ACME challenge server closed.');

		// IP certificates usually don't need SANs if CN is IP, or maybe they do?
		// Let's Encrypt for IP requires the IP in SANs.

	// IP certificates usually don't need SANs if CN is IP, or maybe they do?
	// Let's Encrypt for IP requires the IP in SANs.
	// For Let's Encrypt IP certificates, the IP address must be included in the Subject Alternative Names (SANs).

Use IP certificates #1688

Are you sure you want to change the base?

Use IP certificates #1688

Uh oh!

Conversation

fortuna commented Jan 2, 2026

Uh oh!

Check failure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants