having `_danger-local-https` may result in insecure payjoin-cli build if built with `--all-features`

[nothingmuch]

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

we should make sure it's not possible, or at least not easy to inadvertently build a payjoin-cli binary that has this feature enabled

note: i did not verify that --all-features doesn't skip features with leading underscores

possible approaches:

• remove need for bypassing certificate checks in tests?
• hide this behind a dev dependency somehow?
• ...?

[DanGould]

Another approach would be to make _danger-local-https behavior the behavior without a specific feature, then have pki-https be a feature that's on by default which overrides this behavior to use certificate authorities properly w/o local certs. So --all-features would be secure and you'd still need to make explicit that you want to compile without the default feature set to get _danger-local-https behavior

[DanGould]

More complexity was added to this feature gate with #528. It'd be really nice to remove this, perhaps with proper cert checks.

[benalleng]

To unwind the _danger-local-https feature flag we have a few things we need to accomplish.

1. Merge fetch_ohttp_keys() and fetch_ohttp_keys_with_cert() this will allow us to mostly remove the dependency for this flag in v2
2. Pull the logic out of the cli source for start_http_server() such that we can do any local certs in the v1 test and do not need this in v1.
3. Pull this out of the directory, I haven't looked too closely at this so I'm not sure what this will entail here.
4. With none of the core logic needing the _danger-local-https flag we can safely remove this flag from the codebase

Unwinding each of these parts one at a time does add some complexity in just making sure everything builds at each intermediate step.

This will start with v2 in #705 I will add any additional insights from that PR here as our tracking issue

[benalleng]

I'm assuming we would ultimately want to avoid having something like an optional cert_der like we had before https://github.com/payjoin/rust-payjoin/blob/f7cb8b7533e4ff316f54f724ec7449a52e3436e9/payjoin/src/io.rs?

[DanGould]

I'd like to start by saying that this issue is about an insecure default, not necessarily about getting rid of the ability to bypass cert verification, though that is related.

1. Merge fetch_ohttp_keys() and fetch_ohttp_keys_with_cert() this will allow us to mostly remove the dependency for this flag in v2

I still want the ergonomics of fetch_ohttp_keys without having to pass a certificate. Exposing a second function with (perhaps by default) ability to pass a certificate for testing, as we have, is fine. Perhaps now fetch_ohttp_keys is what becomes the additional feature (see #451 (comment)).

2. pull the logic out of the cli source for start_http_server() such that we can do any local certs in the v1 test and do not need this in v1.

That'd be great

Pull this out of the directory, I haven't looked too closely at this so I'm not sure what this will entail here.

If I recall correctly, thepayjoin-directory/_danger-local-https feature is not actually dangerous like it is for payjoin-cli. In Payjoin-cli, the feature disables certificate validation. In payjoin-directory it just lets you pass in a certificate manually.

With none of the core logic needing the _danger-local-https flag we can safely remove this flag from the codebase

If we're keeping any pass-a-certificatae functionality that we don't want to be exposed in the public interface, especially if that interface introduces new dependencies (not just transient ones, which would be fine), then they should still be feature gated. Keep in mind the core of this issue: --all-features creates a dangerous default now. The issue is not that _danger-local-https exists (though in payjoin-cli it's an ugly hack vs passing in a cert), it's that it introduces default behavior when using --all-features.

[benalleng]

Ok! That's a good reminder of the final goals here.

[spacebear21]

@benalleng did #729 close this issue?

[benalleng]

I don't think so, that PR was mostly limited to the name change specifically, the crux of this issue still exists as I believe you can access _manual-tls features when building with --all-features

[spacebear21]

I guess the issue in question is more "is it still considered dangerous to enable _manual_tls?" IIRC there were other changes involved outside of a simple renaming.

[benalleng]

Yes, there still exists the ability for the cli to build with insecure cert validation when built with --all-features Can take another look at #705 and some of Dan's comments to make a more refined PR

[nothingmuch]

This was addressed by #913