Change _danger-local-https to _manual-tls

[benalleng]

In #705 I attempted to invert the feature gated logic and use a new _pk-https flag to completely replace the _danger-local-https however after some review we realized that I approached the problem there incorrectly but also that this flag is possibly outdated and overstating the danger present!

The current behavior is not as dangerous as stated because there are only 2 code-paths.

1. An invalid cert goes through https and won't work
2. the cert is passed in through a test where it is still validated on locally

For the most part the logic would remain the same behind this _manual-tls flag in places like the directory and payjoin/src crate as this change mostly affects how the cli accepts certs and the code in those other places already works as it should, the flag is just a bit misleading for those code paths.

The one thing to note is that we still want to ensure that any sort of local cert validation is only done explicitly and not as a default anywhere.

This issue should supercede #451
If anyone has an idea of where it no longer became "dangerous" that would be nice for some history, I looked through the git history but could not immediately pinpoint the commit where the logic was completely skipping validation as was stated in that old issue

the _danger-local-https feature allows skipping certificate validation for testing purposes.

[0xZaddyy]

Hi! I started working on this issue. I’ve renamed most instances of _danger-local-https to _manual-tls and ran the tests all of them passed.

Is there anything else I should be aware of or do to fully address the concern, especially around making sure the insecure behavior isn’t accidentally included in production builds?

Thanks!

[benalleng]

So I think we'd like a few things

1. see if we can remove the hard coded local cert file in favor of something that pulls out the local cert generation directly from the server initializer similar to how it works in our ohttp server.

• https://github.com/payjoin/rust-payjoin/blob/master/payjoin-cli/src/app/v1.rs#L151-L181
  vs the ohttp-relay integration test
• https://github.com/payjoin/ohttp-relay/blob/5d830fdcd1851b3b41bf7efb562ab9a14116d1c3/tests/integration.rs#L413-L427
  notice that the local cert generation is not pushed into a file and instead uses a root_store to pass it in at the test_bootstrap phase, that would be another nice addition instead of hard coding the cert der. In practice this would likely look like just pulling the cert generation out from these existing functions in our cli source.

2. See how much we can simplify this logic once the cert generation is gated more sanely, perhaps its possible to just make a feature gated option argument that accepts a cert when _manual-tls is active in mod.rs this would hopefuly deduplicate this code and make it easier to read.

• rust-payjoin/payjoin-cli/src/app/mod.rs

  Lines 58 to 82 in 538dc31

  	#[cfg(feature = "_danger-local-https")]
  	fn http_agent() -> Result<reqwest::Client> { Ok(http_agent_builder()?.build()?) }
  	#[cfg(not(feature = "_danger-local-https"))]
  	fn http_agent() -> Result<reqwest::Client> { Ok(reqwest::Client::new()) }
  	#[cfg(feature = "_danger-local-https")]
  	fn http_agent_builder() -> Result<reqwest::ClientBuilder> {
  	use rustls::pki_types::CertificateDer;
  	use rustls::RootCertStore;
  	let cert_der = read_local_cert()?;
  	let mut root_cert_store = RootCertStore::empty();
  	root_cert_store.add(CertificateDer::from(cert_der.as_slice()))?;
  	Ok(reqwest::ClientBuilder::new()
  	.use_rustls_tls()
  	.add_root_certificate(reqwest::tls::Certificate::from_der(cert_der.as_slice())?))
  	}
  	#[cfg(feature = "_danger-local-https")]
  	fn read_local_cert() -> Result<Vec<u8>> {
  	let mut local_cert_path = std::env::temp_dir();
  	local_cert_path.push(LOCAL_CERT_FILE);
  	Ok(std::fs::read(local_cert_path)?)
  	}

3. lastly ensure that our cargo.toml is formatted correctly to not enable _manual-tls as a default feature anywhere and that for local certs to be enabled some features="" must be passed.

• for the most part I think this is the current behavior but its worth double checking!

[0xZaddyy]

@spacebear21 i forget to close this in the PR

[spacebear21]

Closed by #763