Skip to content
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

Feature: disable HTTPS cert verification for debugging #254

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Tokarak
Copy link
Contributor

@Tokarak Tokarak commented Sep 20, 2024

Rationale: disabling https verification allows debugging the https connection by proxying Redlib through an HTTPS sniffer. This has been discussed in #249.

Closes #249

@Tokarak
Copy link
Contributor Author

Tokarak commented Sep 20, 2024

This is technically ready to be merged, but I would like to add a command line toggle with this, to advertise this feature, and to emphasasize that it is dangerous.

Planned functionality:

  • Add help message for --no-https-verification explaining that it is behind a feature, and that it should only be enabled for debug purposes as it is a security concern.
  • With no-https-verification feature disabled:
    • Custom error message when --no-https-verification is passed
    • Always verify https
  • With no-https-verification feature enabled:
    • Clap integration to pass --no-https-verification
    • Do not verify https if --no-https-verification is passed
    • Verify https if --no-https-verification is not passed
  • Automatically enable no-https-verification feature in debug mode NOTE: cargo does not support this, AFAICT
  • Add to instance info whether the feature and the command line flag are disabled/enabled.

@sigaloid
Copy link
Member

I'd also like to have a banner message displayed if HTTPS verification is disabled, so 1. we don't accidentally enable it 2. users are warned that the traffic could be logged. I know it can just be patched out but nonetheless seems prudent. (Also ignore failing checks)

@Tokarak
Copy link
Contributor Author

Tokarak commented Sep 21, 2024

I'd also like to have a banner message displayed if HTTPS verification is disabled
Suggestion: put this in the instance info page, so that it is always available without breaking the aesthetic. This is also extendable to other debug features.

I'm putting this in "help wanted". This PR is accepting PRs.

Throws error when the flag is enabled and the eponymous feature is
disabled. Currently not linked to any underlying functionality.
@Tokarak
Copy link
Contributor Author

Tokarak commented Sep 21, 2024

Progress update: I have added the command line flag, but it has no functionality.

This pr should probably be rebased into at least two commits, since edb16f2 should be a seperate commit.

I'm stuck on passing the flag from the main thread to the instantiation of the hyper_rustls client in client.rs. I can probably create a global 'static optional bool variable, and save the outcome there in the main thread; client.rs will then unwrap that variable. But that feels hacky — any better suggestions? I think refactoring into using a Config struct and a clap::Parser pattern is what is needed. I suppose this pr can do the messy hack with a //TODO comment, and then a seperate pr can fix this.

@sigaloid
Copy link
Member

One option would be replacing the Lazy with a OnceCell and instantiating it manually in main.rs.

@sigaloid
Copy link
Member

sigaloid commented Sep 23, 2024

I implemented the functionality and edited your checklist - last is the instance info page. IMO though the feature/flag shouldn't be enabled in dev mode - some people might accidentally leave it in debug mode permanently when hosting an instance.

Anyway about the instance info: That might necessitate a OnceLock bool anyway for the actual CLI flag (so the template code can access it). In which case, we don't need the code I made about choosing it at runtime, we could simply initialize the OnceLock bool first, then lazily initialize the client, unwrapping that bool. Up to you if you want to go with that method, or just leave it as-is and bolt on the boolean.

@Tokarak
Copy link
Contributor Author

Tokarak commented Oct 20, 2024

Tests are broken right now, right? To properly test it in an applied way, #184 should be fixed.

@Tokarak Tokarak marked this pull request as ready for review October 20, 2024 18:02
@@ -44,6 +46,8 @@ pretty_env_logger = "0.5.0"
dotenvy = "0.15.7"
rss = "2.0.7"
arc-swap = "1.7.1"
rustls = { version = "0.21.12", features = ["dangerous_configuration"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this should ideally be gated under the no-https-verification feature so that it isn't even enabled unless the no-https-verification one is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

💡 Feature request: Verbose HTTPS debug session: dump all traffic to and from Reddit
2 participants