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(ext/net): extract TLS key and certificate from interfaces #23327

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

bartlomieju
Copy link
Member

Relands #23325

/** Cert chain in PEM format */
key: string;
}
export type ServeTlsOptions = ServeOptions & TlsCertifiedKeyOptions;
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be changed back to be an interface. We might have to split it into multiple interfaces to support proper overloads

@bartlomieju bartlomieju added this to the 1.43 milestone Apr 11, 2024
@iuioiua
Copy link
Collaborator

iuioiua commented Apr 15, 2024

Previously, in 1.41, Deno.listenTls({ port: 443 }) would throw with:

Uncaught (in promise) Error: `cert` is not specified.

Now, this same snippet causes a panic because loadTlsKeyPair() returns op_tls_key_null(). Perhaps this should be addressed in this PR. Note: this is what's currently blocking #23271 and #23270.

@iuioiua
Copy link
Collaborator

iuioiua commented Apr 16, 2024

To add to my previous comment, perhaps we should just error within loadTlsKeyPair() instead of returning op_tls_key_null().

@mmastrac
Copy link
Contributor

To add to my previous comment, perhaps we should just error within loadTlsKeyPair() instead of returning op_tls_key_null().

We need to have an option to be successful if no key is specified for connectTls, however (key is optional there).

@mmastrac
Copy link
Contributor

mmastrac commented Apr 17, 2024

Previously, in 1.41, Deno.listenTls({ port: 443 }) would throw with:

Uncaught (in promise) Error: `cert` is not specified.

Now, this same snippet causes a panic because loadTlsKeyPair() returns op_tls_key_null(). Perhaps this should be addressed in this PR. Note: this is what's currently blocking #23271 and #23270.

I'm not sure if this is true -- I will add an extra layer of checking around it, but I'm seeing it correctly detect that a certificate and/or key is missing:

> Deno.listenTls()
Uncaught TypeError: Cannot destructure property 'port' of 'undefined' as it is undefined.
    at Object.listenTls (ext:deno_net/02_tls.js:230:3)
    at <anonymous>:1:27
> Deno.listenTls({})
Uncaught TypeError: A key and certificate are required for `listenTls`
    at Object.listenTls (ext:deno_net/02_tls.js:241:11)
    at <anonymous>:1:27
> Deno.listenTls({ cert: "" })
Uncaught TypeError: If `cert` is specified, `key` must be specified as well for `Deno.listenTls`.
    at both (ext:deno_net/02_tls.js:177:13)
    at loadTlsKeyPair (ext:deno_net/02_tls.js:189:3)
    at Object.listenTls (ext:deno_net/02_tls.js:243:19)
    at <anonymous>:1:27

options:
| ServeOptions
| ServeTlsOptions
| (ServeTlsOptions & TlsCertifiedKeyOptions),
handler: ServeHandler,
Copy link
Contributor

Choose a reason for hiding this comment

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

@marvinhagemeister @bartlomieju What do you guys think of this? We can make ServeTlsOptions an empty interface which will allow for older code using ServeTlsOptions to continue to typecheck.

@iuioiua
Copy link
Collaborator

iuioiua commented Apr 17, 2024

Previously, in 1.41, Deno.listenTls({ port: 443 }) would throw with:

Uncaught (in promise) Error: `cert` is not specified.

Now, this same snippet causes a panic because loadTlsKeyPair() returns op_tls_key_null(). Perhaps this should be addressed in this PR. Note: this is what's currently blocking #23271 and #23270.

I'm not sure if this is true -- I will add an extra layer of checking around it, but I'm seeing it correctly detect that a certificate and/or key is missing:

> Deno.listenTls()
Uncaught TypeError: Cannot destructure property 'port' of 'undefined' as it is undefined.
    at Object.listenTls (ext:deno_net/02_tls.js:230:3)
    at <anonymous>:1:27
> Deno.listenTls({})
Uncaught TypeError: A key and certificate are required for `listenTls`
    at Object.listenTls (ext:deno_net/02_tls.js:241:11)
    at <anonymous>:1:27
> Deno.listenTls({ cert: "" })
Uncaught TypeError: If `cert` is specified, `key` must be specified as well for `Deno.listenTls`.
    at both (ext:deno_net/02_tls.js:177:13)
    at loadTlsKeyPair (ext:deno_net/02_tls.js:189:3)
    at Object.listenTls (ext:deno_net/02_tls.js:243:19)
    at <anonymous>:1:27

To clarify, I saw the error for Deno.listenTls({ port: 443 }) specifically. I can confirm this is the case for the latest stable. Either way, adding checking would be great 🙂

@mmastrac mmastrac merged commit 6a09a16 into denoland:main Apr 18, 2024
17 checks passed
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.

4 participants