-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(fetch): TLS client certificates (mutual authentication) for fetch() #11721
feat(fetch): TLS client certificates (mutual authentication) for fetch() #11721
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is updating the test_util/wpt
submodule intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanwykes Thanks for the PR. Deno.createHttpClient is already unstable, so there doesn't need to be any additional checks for certChain
and privateKey
.
@littledivy no, it's a mistake. Eliminated from PR. |
Co-authored-by: Luca Casonato <lucacasonato@yahoo.com>
…kes/deno into fetch-client-certificates
ext/tls/lib.rs
Outdated
@@ -156,6 +162,52 @@ pub fn create_client_config( | |||
Ok(tls_config) | |||
} | |||
|
|||
fn load_certs(reader: &mut dyn BufRead) -> Result<Vec<Certificate>, AnyError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are all copied over from ext/net
. Can you export them in ext/tls
and remove them from ext/net
(instead importing from ext/tls
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, can do! Should I bump tls version (0.1.1 -> 0.1.2) and update net's dependency on tls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, not necessary (but thanks!). We'll do the releasing + publishing just before the next release, during the release process :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot @seanwykes, great feature to have.
@lucacasonato thanks for the help and suggestions. And of course, thanks to @eric who did the real work in getting TLS client-certificates in. |
@cryptographix @lucacasonato what would be the path to land this on |
Implementation of TLS client-certificate for fetch(), largely based on work by @eric in #9426 - so all kudos to him for the hard work.
Needs patching to put feature behind the --unstable flag, see ext/fetch/lib.rs:547 for commented (incorrect) code
Implements #10516.