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

Add option to enable tlsproxy #223

Closed
wants to merge 1 commit into from
Closed

Add option to enable tlsproxy #223

wants to merge 1 commit into from

Conversation

envy
Copy link
Contributor

@envy envy commented Jan 27, 2025

Add an env var to optionally enable the tlsproxy in master.cf

This fixes #222.

@bokysan
Copy link
Owner

bokysan commented Jan 27, 2025

Hello.

Thank you for the commit and I really do appreciate the work done. However, as it stands now, I cannot accept it:

  1. Setting ENABLE_TLSPROXY to false will not disable an already enabled TLS proxy.
  2. There are no tests associated with the provided commit.
  3. The feature will not work if somebody changed the master.cf by hand and removed the line #tlsproxy.... Even if the line is just changed to # tlsproxy... this commit will fail.
  4. Variable should have a POSTFIX_ prefix, at least, to prevent polluting the global namespace.
  5. If we're implementing functionality to change master.cf we should enable other features as well, not just and merely tlsproxy. So the commit should be more generic.

@envy
Copy link
Contributor Author

envy commented Jan 30, 2025

Sorry, I don't have the time to implement all that.

@envy envy closed this Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to enable tlsproxy
2 participants