add danger clause for ignoring invalid certificates

[da-kami]

fixes #2464

needed to make ether-halight e2e test pass on macOS Catalina.

Questionable if this is a good fix, since it is a security concern.

[mergify]

Are you sure the changelog does not need updating?

[mergify]

Are you sure the changelog does not need updating?

[mergify]

Are you sure the changelog does not need updating?

[mergify]

Are you sure the changelog does not need updating?

[thomaseizinger]

As this only happens on MacOS, can we do this under a conditional-compilation flag for macos?

Also a comment in the code would be good to explain the problem and possible link a ticket that we can watch for resolution.

[da-kami]

As this only happens on MacOS, can we do this under a conditional-compilation flag for macos?

@thomaseizinger I duplicated the function and added conditionals. Is that what you had in mind, or should I rather us https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-macro ?

[thomaseizinger]

As this only happens on MacOS, can we do this under a conditional-compilation flag for macos?

@thomaseizinger I duplicated the function and added conditionals. Is that what you had in mind, or should I rather us doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-macro ?

Yes that is what I meant although I would have probably only applied to the particular line like this:

let mut builder = reqwest::Client::builder().add_root_certificate(cert).default_headers(default_headers);

#[cfg(target_os = "macos")]
{
	// a very long and good comment on what is happening and why we need this
    builder.danger_accept_invalid_certs(true);
}

builder.build()?

[da-kami]

As this only happens on MacOS, can we do this under a conditional-compilation flag for macos?

@thomaseizinger I duplicated the function and added conditionals. Is that what you had in mind, or should I rather us doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-macro ?

Yes that is what I meant although I would have probably only applied to the particular line like this:

let mut builder = reqwest::Client::builder().add_root_certificate(cert).default_headers(default_headers);

#[cfg(target_os = "macos")]
{
	// a very long and good comment on what is happening and why we need this
    builder.danger_accept_invalid_certs(true);
}

builder.build()?

@thomaseizinger unfortunately that cannot be changed that easy. running into lifetime (use of moved value...) with the builder when doing this.

[da-kami]

Seems there is an additional problem with the lnd_sanity test.
Not sure how and if this is related. Gonna look into it tomorrow.
Locally lnd_sanity is passing - all e2e tests green 🤷‍♂️

[D4nte]

Please custom block size + add explanatory comment.

[D4nte]

Also, what was the result of trying with rustls-tls? Looks like you have to pass .use_rustls_tls() on the client Builder to force its use.

[da-kami]

Also, what was the result of trying with rustls-tls? Looks like you have to pass .use_rustls_tls() on the client Builder to force its use.

#2464 (comment)

I did try fixing it with rustls-tls first, but was not able to. The fix would have had similar behaviour (ignore validation) as the one on native-tls.

[thomaseizinger]

I hope it is not too much bike-shedding but I found this: https://doc.rust-lang.org/std/macro.cfg.html

So I think we could do:

// The generated, self-signed lnd certificate is deemed invalid on macOS
// Catalina because of new certificate requirements in macOS Catalina: https://support.apple.com/en-us/HT210176
// By using this conditional compilation step for macOS we accept invalid
// certificates. This is only a minimal security risk because by default the
// certificate that lnd generates is configured to only allow connections
// from localhost. Ticket that will resolve that issue: https://github.com/lightningnetwork/lnd/issues/4201
let accept_invalid_certificates = if cfg!(target_os = "macos") {
	true
} else {
	false
};    

let client = reqwest::Client::builder()
	.danger_accept_invalid_certs(accept_invalid_certificates)
    .add_root_certificate(cert)
    .default_headers(default_headers)
    .build()?;

[thomaseizinger]

Up to you if you like it better :)

[D4nte]

Where is the 🚲🏠 comment reaction when we need it?

[da-kami]

beautiful, but saw it too late. Feel free to push a follow up :)

[mergify]

bors r+

[bors]

Build succeeded:

• lint
• static_analysis
• build (ubuntu-latest)
• e2e_test (ubuntu-latest)