feat: Add --unsafely-treat-insecure-origin-as-secure flag to disable SSL verification #11324

TheAifam5 · 2021-07-07T23:18:39Z

This PR corresponds to the issue #1371 which implement additional flag to the cli, tools, core, extensions and runtime allowing a user to define a domain or a list of domains which should be excluded from certificate validation while connecting or performing the HTTPS requests or disable validation completely.

The name noCheckCertificate and derives I found somewhere in issues of Deno repository as an proposal for the naming and decided to stick with it.

I will be out for few weeks and I can not continue the work on this code, but I will be able to accept PR in my repository and will be open for discussion.

The first commit is an actual feature implementation, the second is only about fixing issues reported by ./tools/format.js and ./tools/lint.js and some comment changes - all of the issues reported by those tools are fixed, except the one which I am not able to fix right now.

Those 3 tests are failing for unknown reason:

failures:
    integration::js_unit_tests
    integration::lsp::lsp_large_doc_changes
    integration::timeout_clear

Closes #5931

TODOs:

Tests
Domain name validation (and IP too?)
Cleanup

Thanks for hints @bartlomieju :)

CLAassistant · 2021-07-07T23:18:44Z

All committers have signed the CLA.

cli/flags.rs

extensions/fetch/Cargo.toml

extensions/fetch/lib.rs

extensions/net/Cargo.toml

extensions/net/ops_tls.rs

runtime/Cargo.toml

core/no_certificate_validation.rs

bartlomieju · 2021-07-08T10:41:17Z

I will be out for few weeks and I can not continue the work on this code, but I will be able to accept PR in my repository and will be open for discussion.

Thanks for opening a PR @TheAifam5; I would love to land this PR for 1.12 that is be to released on Tuesday, would you mind if I push directly to your PR?

bartlomieju · 2021-07-08T10:49:31Z

Also there are a lot of unrelated changes across multiple files (looks like clippy warnings), however they are not pointed out by current CI setup, so I will revert them to make this PR a bit slimmer.

TheAifam5 · 2021-07-08T11:05:41Z

@bartlomieju I don’t see problem with that, go ahead ;)

Do I need to add you to my fork ?

The changes were due clippy’s warnings so I decided to „fix” them :) I will next time note that to only fix errors - or how should be such cases handled?

bartlomieju · 2021-07-08T15:56:39Z

@bartlomieju I don’t see problem with that, go ahead ;)

Do I need to add you to my fork ?

There's no need, as a maintainer I can push to your fork.

The changes were due clippy’s warnings so I decided to „fix” them :) I will next time note that to only fix errors - or how should be such cases handled?

What clippy version are you using? When I run tools/lint.js I get no warnings whatsoever. If we can get the same warnings out of the script, I will be more than a happy for another PR that addresses those.

TheAifam5 · 2021-07-08T16:22:14Z

What clippy version are you using? When I run tools/lint.js I get no warnings whatsoever. If we can get the same warnings out of the script, I will be more than a happy for another PR that addresses those.

clippy 0.1.55 (d2b04f07 2021-07-07)

it may be because of the nightly I have set as default.

bartlomieju · 2021-07-08T16:23:20Z

What clippy version are you using? When I run tools/lint.js I get no warnings whatsoever. If we can get the same warnings out of the script, I will be more than a happy for another PR that addresses those.

clippy 0.1.55 (d2b04f07 2021-07-07)

it may be because of the nightly I have set as default.

That's probable, I'm running on latest stable and cargo clippy --version gives me: clippy 0.1.53 (53cb7b09 2021-06-17)

TheAifam5 · 2021-07-08T17:01:16Z

@bartlomieju What's your plan? What would you like to change here? I might have some spare time and might help a little :)

bartlomieju · 2021-07-08T17:04:14Z

@bartlomieju What's your plan? What would you like to change here? I might have some spare time and might help a little :)

@TheAifam5 for now I just rolled back unrelated changes. I will do the proper review shortly and let you know. There's some stuff that's wrong at the first glance (there shouldn't be any changes in core/), but ultimately I think it's just a matter of moving the code around. One thing that needs to be done before we can land it is adding some unit or integration tests.

cli/flags.rs

cli/tools/coverage.rs

core/bindings.rs

core/no_certificate_validation.rs

extensions/fetch/lib.rs

extensions/net/lib.rs

extensions/net/ops_tls.rs

runtime/Cargo.toml

core/no_check_certificate_deserializer.rs

extensions/fetch/Cargo.toml

core/Cargo.toml

crowlKats · 2021-07-09T11:37:14Z

shouldnt this also apply for websocket?

TheAifam5 · 2021-07-09T12:37:49Z

shouldnt this also apply for websocket?

Let me check that :)

EDIT:
@crowlKats should work now

extensions/net/lib.rs

bartlomieju

@TheAifam5 this PR looks mostly good to me - I pointed out a few things that I don't understand why they're needed. Once those point are addresses I'll help you out with refactoring the code to not change deno_core.

That said, there are tens of files that are changed that are not in any way related to this PR. Please revert those changes. (I believe these might be caused by clippy, but since CI on main branch doesn't complain, these changes should be removed)

core/allow_insecure_certificates_deserializer.rs

core/no_certificate_verification.rs

extensions/net/lib.rs

extensions/net/lib.deno_net.d.ts

extensions/net/ops_tls.rs

TheAifam5 · 2021-08-03T15:29:11Z

Will do it :)

bartlomieju · 2021-08-04T19:50:16Z

#11491 should be landed first, it will provide extensions/tls crate, which will be a good place to put shared verification code.

bartlomieju · 2021-08-06T17:57:07Z

@TheAifam5 after landing #11491 I will rebase you PR

cli/file_fetcher.rs

cli/dts/lib.deno.unstable.d.ts

core/Cargo.toml

core/lib.rs

extensions/fetch/lib.rs

extensions/net/02_tls.js

cli/program_state.rs

extensions/websocket/lib.rs

ry

LGTM - nice work @TheAifam5 !

TheAifam5 · 2021-08-09T21:51:34Z

Thank you for finishing my work @bartlomieju and @ry for such kind words.

TheAifam5 mentioned this pull request Jul 7, 2021

Add flag to ignore SSL certificate issues #1371

Closed