Non-transparent SSL Bumping by mmorev · Pull Request #1332 · opnsense/core

mmorev · 2017-01-11T06:08:56Z

In some circumstances transparent proxying is not applicable, but SSL traffic filtering is still needed. This small patch allows use SSL bumping in non-transparent mode through standard 3128 port.

Copied ssl-bump parameters from https-intercept listeners to usual http listener

Removed help strings that noticed transparent mode requirement for SSL bumping.

AdSchellevis · 2017-01-11T10:20:27Z

@mmorev isn't the outcome of this the same as sslbumpport to port 3128?

mmorev · 2017-01-11T10:34:11Z

@AdSchellevis sslbumpport is used within intercept-mode https listener on loopback interface. User cannot connect to this listener directly if not redirected by firewall. Moreover, not all web browsers support different proxy servers for HTTP and HTTPS sites (like Firefox does), so using port 3128 for both http and ssl is the best way of handling user traffic.

AdSchellevis · 2017-01-11T11:27:34Z

@mmorev your problem is with the listener, I see. I'm not sure what the downside is with enabling this by default, if there is non, I'm fine with it enabling sslbump on the default listener as well.
It looks like the http_port tags are the same as the ones in the sslbump_httpsconfig macro, can you change them to use the macro and keep/change the help text, I don't think it's a good idea to remove the forward reference which most people use.

fichtner · 2017-01-11T11:31:10Z

but please don't remove the redirect rule and the associated feature. it looks like changing a feature into something else, not an addition.

mmorev · 2017-01-11T11:57:53Z

@AdSchellevis okay, I just did'nt pay attention to that macro, sorry. Will review this evening. My python isn't so good as my english :).
@fichtner of cause I will not remove anything. These are different features, which, I think, should not depend each other.

mmorev · 2017-01-11T12:00:55Z

@AdSchellevis btw a part of code that I have modified should use this macro to create IPv4 and IPv6 listeners by design, isn't it? Currently it creates IPv4 listener only.

AdSchellevis · 2017-01-11T12:06:42Z

@mmorev it should indeed create both listeners, but if I'm reading it correctly, it doesn't appear to be doing so with our standard rules at the moment. So sticking to what's already there is fine, adding IPv6 (for both) is also good.

Slightly extended sslbump_httpsconfig macro to make usable in regular listener strings; renamed macro to listener_config.

mmorev · 2017-01-12T07:48:34Z

@AdSchellevis i have made some changes, please, review

AdSchellevis

@mmorev changes look good, only two small comments. I like what you've done with the macro here.

Thanks!

AdSchellevis · 2017-01-30T20:38:02Z

src/opnsense/mvc/app/controllers/OPNsense/Proxy/forms/main.xml

-                  <br/><br/>
-                  Transparent HTTP proxy needs to be enabled and you need nat rules to reflect your traffic
-                  for this feature to work.<br/>
-                  <a href="/firewall_nat_edit.php?template=transparent_proxy&https=1"> Add a new firewall rule </a>


can we leave some comment in there about adding firewall rules? Most people use this in transparant mode.

This link was duplicated from proxy.forward.transparentMode field comment, where it's really necessary. Looks like someone considered these two options dependent, but they aren't.

You're right, sorry about the noise here.

so what happens to our transparent_proxy https rule template? it's now unused.

@fichtner look above in file, it's used on line 221

@fichtner I see it is nontrivial to create such link for adding two rules simultaneously. I'll think if we can create rules another way.

@mmorev we have pluggable rules now, they could be used instead. though I liked the simplicity of the approach: rule for http transparent use, another one for https transparent use as it gives the flexibility to do the one or the other or both. plus the user could change the rule, set it for a specific range of sources or destinations. If we use a checkbox from the proxy, this flexibility will go away.

@fichtner @mmorev should we add the link for now? I also forgot about the template action here....

I don't think there is a way around adding it back until we have a better solution

I agree, I will add it back.

AdSchellevis · 2017-01-30T20:41:17Z

src/opnsense/service/templates/OPNsense/Proxy/squid.conf

+{%         set listener_type = 'http_port' %}
+{%     endif %}
+{%     if helpers.exists('OPNsense.proxy.forward.sslbump') and OPNsense.proxy.forward.sslbump == '1' %}
+{%         set sslparams = 'ssl-bump cert=/var/squid/ssl/ca.pem dynamic_cert_mem_cache_size=10MB generate-host-certificates=on' %}


you may need to set sslparams to an empty string it not in sslbump mode, I haven't test this, but I'm not sure either it will like an uninitialised string.

Fixed that. Actually, template engine treats undefined variable as an empty string when printed (http://jinja.pocoo.org/docs/dev/templates/#variables), so it wasn't necessary

mmorev · 2017-02-01T07:37:49Z

@AdSchellevis, @fichtner Thank you! Hope I'll find some time to make more improvements )

fichtner · 2017-02-09T08:05:30Z

when "Enable Transparent HTTP proxy" is on and "Enable SSL inspection" is off squid does not start anymore:

squid: Bungled /usr/local/etc/squid/squid.conf line 9: https_port 127.0.0.1:3129 intercept

mmorev · 2017-02-09T08:45:25Z

@fichtner See it, here is a fix. Tested in all combinations of these checkboxes.

fichtner · 2017-02-09T08:48:43Z

@mmorev many thanks, tested and committed 👍

mmorev · 2017-02-09T08:52:09Z

@fichtner Glad to help) I like your work and product very much)

fichtner · 2017-02-09T08:56:44Z

thanks and likewise we love contributions. please do keep them coming! :)

mmorev added 2 commits January 11, 2017 11:46

Update squid.conf for non-transparent SSL bumping

f3c35f3

Copied ssl-bump parameters from https-intercept listeners to usual http listener

Update main.xml to correspont squid.conf changes

1126922

Removed help strings that noticed transparent mode requirement for SSL bumping.

mmorev added 2 commits January 12, 2017 12:04

Update main.xml

c9e1423

Updated listeners generation

53e4be2

Slightly extended sslbump_httpsconfig macro to make usable in regular listener strings; renamed macro to listener_config.

AdSchellevis reviewed Jan 30, 2017

View reviewed changes

sslparams string coud be used without init

2e70704

AdSchellevis merged commit 6d0f4b4 into opnsense:master Feb 1, 2017

AdSchellevis added a commit that referenced this pull request Feb 1, 2017

add link again, #1332

cdedd9a

Conversation

mmorev commented Jan 11, 2017

Uh oh!

AdSchellevis commented Jan 11, 2017

Uh oh!

mmorev commented Jan 11, 2017

Uh oh!

AdSchellevis commented Jan 11, 2017

Uh oh!

fichtner commented Jan 11, 2017

Uh oh!

mmorev commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmorev commented Jan 11, 2017

Uh oh!

AdSchellevis commented Jan 11, 2017

Uh oh!

mmorev commented Jan 12, 2017

Uh oh!

AdSchellevis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmorev commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fichtner commented Feb 9, 2017

Uh oh!

mmorev commented Feb 9, 2017

Uh oh!

fichtner commented Feb 9, 2017

Uh oh!

mmorev commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fichtner commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mmorev commented Jan 11, 2017 •

edited

Loading

mmorev commented Feb 1, 2017 •

edited

Loading

mmorev commented Feb 9, 2017 •

edited

Loading