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

feat: add auto-tls service #2798

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

feat: add auto-tls service #2798

wants to merge 11 commits into from

Conversation

achingbrain
Copy link
Member

Adds an optional service that requests a Let's Encrypt-style TLS certificate when publicly dialable addresses are detected.

This will allow transports such as WebSockets to upgrade themselves to be the secure version.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Adds an optional service that requests a Let's Encrypt-style TLS
certificate when publicly dialable addresses are detected.

This will allow transports such as WebSockets to upgrade themselves
to be the secure version.
@achingbrain achingbrain mentioned this pull request Nov 3, 2024
3 tasks
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just looking through the initial impl

packages/auto-tls/package.json Show resolved Hide resolved
packages/auto-tls/src/auto-tls.ts Outdated Show resolved Hide resolved
Comment on lines 145 to 155
const response = await this.clientAuth.authenticatedFetch(`https://${this.forgeEndpoint}/v1/_acme-challenge`, {
method: 'POST',
headers: {
'Content-Type': 'application/json'
},
body: JSON.stringify({
value: keyAuthorization,
addresses
}),
...options
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is @libp2p/http-fetch and not native fetch, but native fetch, if provided a bad forgeEndpoint, would throw. Will @libp2p/http-fetch?

should we catch that thrown error?

this.clientAuth = new ClientAuth(this.privateKey)
this.started = false
this.fetching = false
this.fetchCertificates = debounce(this._fetchCertificates.bind(this), init.delay ?? 5000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 3 different fetchCertificate functions going on here.

  1. fetchCertificates - debounced _fetchCertificates defined in constructor
  2. _fetchCertificates - function which guards against multiple fetch calls and ensures certs are not fetched if not appropriate
  3. fetchCertificate - private method on class which does the actual work of fetching a certificate (i.e. called by _fetchCertificates), and calls _fetchCertificate after renewalTimeout

I think the _fetchCertificates and the debounced version make sense, but I think it could help clarify things a bit for future devs if we use better names to differentiate between all of these functions.

packages/auto-tls/src/auto-tls.ts Outdated Show resolved Hide resolved
Comment on lines +112 to +117
/**
* How long before the expiry of the certificate to renew it in ms
*
* @default 60000
*/
renewThreshold?: number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This situation is a little different than domain cert renewal, but cert renewal should allow for resolving any errors that pop up when a renewal is attempted. Should we set the renewal time to a full day or week before expiry instead of only one minute?

packages/interface/src/index.ts Show resolved Hide resolved
'certificate:provision': CustomEvent<TLSCertificate>

/**
* This event notifies listeners that a TLS certificate is available for use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This event notifies listeners that a TLS certificate is available for use
* This event notifies listeners that a new TLS certificate is available for use

const crypto = new Crypto()
x509.cryptoProvider.set(crypto)

type CertificateEvent = 'certificate:provision' | 'certificate:renew'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type CertificateEvent = 'certificate:provision' | 'certificate:renew'
type CertificateEvent = 'certificate:provision' | 'certificate:renew' | `certificate:load`

add event for loading a cert during startup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing something like this but it wasn't clear to me what the actual difference between certificate:provision and certificate:load would be, they both just mean "a certificate is available and one wasn't before", whereas certificate:renew means "a certificate is available but you may need to undo what you did before to start using it".

@@ -0,0 +1,5 @@
describe('auto-tls', () => {
it('should fetch a TLS certificate', async () => {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you are aware, but a callout here for some tests. Some I can think of:

  • invalid forgeEndpoint expectations
  • certifical renewal time
  • certificate loading on startup (not implemented yet)
  • event emitting
  • triggering fetch of cert only on certain listening maddrs

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

Successfully merging this pull request may close these issues.

3 participants