Add SSL reverse proxy paths (#3179)

[Cinnamals]

Fixes #3179.
Adds secure_node_uri and secure_rnode_uri configuration options to support SSL proxying for backend applications. If these URIs are defined (e.g., /secure-node), mod_ood_proxy upgrades the upstream connection to HTTPS/WSS.

When secure_node_uri or secure_rnode_uri are configured, the generator automatically enables SSLProxyEngine. The idea is to allow Apache reverse proxy to communicate with backend applications that are running their own SSL termination.

This is the ood_portal.yml snippet I have used during testing for my self-signed backend application:

ssl_proxy: 
  - 'SSLProxyCheckPeerCN Off'
  - 'SSLProxyCheckPeerName Off'

secure_node_uri: '/secure-node`
secure_rnode_uri: '/secure-rnodes`

While #1306 discusses replacing mod_ood_proxy, this makes life easier for now (at least for my use case).

[johrstrom]

I'm pretty sure ports need to be dynamic. That is if there are only a handful of ports one can use, then you'll likely run into conflicts when multiple people's jobs land on the same machine unless you specify 100s.

I think it's likely better to keep the very large port range then choose the protocol in mod_ood_proxy based on a header.

[Cinnamals]

I'm pretty sure ports need to be dynamic. That is if there are only a handful of ports one can use, then you'll likely run into conflicts when multiple people's jobs land on the same machine unless you specify 100s.

Thanks for the feedback. You make a good point about port conflicts on shared nodes... My static port approach definitely assumes a containerized or exclusive-node environment. I guess that kind of need is too specific, although perfectly fine in our case.

I think it's likely better to keep the very large port range then choose the protocol in mod_ood_proxy based on a header.

I'll take a look at this suggestion. Thanks again.

[xpillons]

Just a naive suggestion. When specifying the proxy uri instead of using node or rnode we could use a secured one like nodes or rnodes. That way the port remains dynamic and the application owner can decide if SSL needs to be used or not.

[johrstrom]

Just a naive suggestion. When specifying the proxy uri instead of using node or rnode we could use a secured one like nodes or rnodes. That way the port remains dynamic and the application owner can decide if SSL needs to be used or not.

That could work too and is likely simpler in that it'll touch fewer spots in the code base.

[Cinnamals]

Just a naive suggestion. When specifying the proxy uri instead of using node or rnode we could use a secured one like nodes or rnodes. That way the port remains dynamic and the application owner can decide if SSL needs to be used or not.

I found a commit from @goodsonjr that does something like this: goodsonjr@6762cc1. I think it could be somewhat simplified to a solution that's more like Xavier described here.

What do you think, Jeff?

[johrstrom]

What do you think, Jeff?

I think that's a bit much. If I'm reading that right - it's a global config for all origins. Instead of setting the config globally and passing all those variables around - we can likely just parse the request path in mod_ood_proxy and make the toggle there.

That is, instead of pushing all this information into an existing LocationMatch, we create an entirely new LocationMatch (or two) and parse the path in lua before setting the protocol.

[goodsonjr]

I've gone through a few iterations of how to handle the configuration with this. The particular commit linked above is a fairly early one and not one that has seen actual use. It has some global configuration because you need some global configuration to get mod_proxy to actually use mod_ssl at all, so it's really a global switch to enable the feature or not unless you want it always configured by default, which would be fine.

I used the slightly more complex LocationMatch to mimic how host and port were extracted and so everything about the proxy route was extracted in one place. That commit is excessively complicated for this purpose (naive implementation) because it includes changes necessary for a couple other schemes. The SetEnvIf isn't necessary, you can just use the match directly in the Lua as with the other matches which I really should have done there.

If I was writing that commit again, I'd probably drop the env var to enable SSL proxying since it isn't necessary. I might make the change to LocationMatch conditional on ssl_proxy being enabled so that if the admins don't configure it they get an identical ood-portal.conf as now and MATCH_SECURE is never matched. I'd keep the LocationMatch modification as it is, extracting MATCH_SECURE and just handling it in Lua like the rest of the named match groups rather than using SetEnvIf to pass another variable. The Lua would then only ever see MATCH_SECURE if the portal config has ssl_proxy enabled so that would need to be handled appropriately. Maybe that is overkill, I think the way it is with the other features stripped out and direct use of the match result in Lua is simple enough.

I think duplicating LocationMatch sections and somehow communicating that different match to Lua is another fine alternative, though you need to duplicate all of your locations that might use SSL proxying, but I don't understand parsing the path in Lua. All other path parsing is already done with LocationMatch directives and if you break it out into another section you don't even need to parse it further, the information about whether you want SSL proxying is already there from being in that match block.

[Cinnamals]

I opted for creating two new LocationMatches with an added 's' to the node_uri and rnode_uri, as discussed here. These blocks set a variable that mod_ood_proxy looks for and upgrade the connection if set.

I've automated the SSL relaxation here for ease of use. However, since this applies to the whole VirtualHost, would you prefer I remove this and just document that the admin must add SSLProxyCheckPeerName Off manually?

  <%- if @nodes_uri || @rnodes_uri -%>
  # Enable SSL Proxying for Naive SSL (self-signed backends)
  SSLProxyEngine On
  SSLProxyCheckPeerCN Off
  SSLProxyCheckPeerName Off
  <%- end -%>

[johrstrom]

Probably need to make this configurable so folks can add and remove this or that. SSLProxyEngine On should probably stay, but the others should be supplied by the admin.

[goodsonjr]

My preference would be to have all directives except for SSLProxyEngine be a templated variable, something like @secure_proxy_settings analogous to the way you can insert a list of configurable Apache directives with @oidc_settings or @auth now. It could potentially default to these two if we wanted the default to be this "semi-secure" setup in the interest of making it trivial to enable without further setup and didn't integrate the full automatic job cert setup immediately. Whatever it is named should probably match up with the renaming of @[r]nodes_uri so the settings are intuitively connected.

[Cinnamals]

I kept the SSLProxyEngine On here while adding the naive_ssl_proxy option to enable the remaining two options. The name could probably be matched better, but I thought the current name would be sufficient along with its description. Since "naive SSL" likely isn't expected as a default, I opted to set the default to false.

[johrstrom]

My preference would be to have all directives except for SSLProxyEngine be a templated variable

Same. It should default to an empty array and allow for folks to supply anything they need related to SSLProxy*.

Similarly I think the variable name should likely reflect what it is, something like ssl_proxy or similar.

[Cinnamals]

My preference would be to have all directives except for SSLProxyEngine be a templated variable

Same. It should default to an empty array and allow for folks to supply anything they need related to SSLProxy*.

Similarly I think the variable name should likely reflect what it is, something like ssl_proxy or similar.

I misread goodson's comment. This sounds great.

[johrstrom]

I think nodes_uri is too close to node_uri. I'd prefer maybe secure_node_uri or tls_node_uri or similar so folks don't get tripped up on something so similar.

Applicable to rnodes_uri too.

[Cinnamals]

I absolutely agree, guess I tunnelvisioned on the name due to the discussion in this thread. :)

[johrstrom]

As an aside - your commit does not appear to be correctly tied to your github account.

[johrstrom]

OK I think this looks good. I haven't pulled it down to verify yet so I'll do that shortly.

I'll let the tests run, but I'm quite sure they'll fail with all these updates. Reading through this, I think we're good on the logic, so I didn't want you to have to do both update test fixtures as we iterate through updating the logic. With the actual logic done I think we can start to look at updating tests.

[moffatsadeghi]

@Cinnamals Hi there, thanks so much for all your work on this! This is going to be a huge win for our community and something we have not had the bandwidth to tackle on our direct team! Just curious, how did you hear about goodsonjr's work and previous commits for this reverse node proxy?

[johrstrom]

I'll let the tests run, but I'm quite sure they'll fail with all these updates.

I can take care of updating the test cases if you wish. However, I'd like to sit on this over the weekend (or maybe more) and think about it.

While it's a good step forward for this functionality, I do wonder if it's in the right direction. That is, I'm worried about a bit of whiplash to the admin if we do this now but then devise some other scheme later.

I just want to be sure that whatever we do has a bit of staying power and isn't just temporary until we settle on something different. If it were purely internal it'd be different, but this will impact site administrators so I just want to be careful not to be overly burdensome when they update.

[Cinnamals]

@Cinnamals Hi there, thanks so much for all your work on this! This is going to be a huge win for our community and something we have not had the bandwidth to tackle on our direct team! Just curious, how did you hear about goodsonjr's work and previous commits for this reverse node proxy?

Initially, I found the Discourse thread in the linked issue #3179. After implementing the hotfix discussed there, I went and looked for an issue on this repository and found the aforementioned issue along with goodonjr's work in the issue thread.

[Cinnamals]

I'll let the tests run, but I'm quite sure they'll fail with all these updates.

I can take care of updating the test cases if you wish.

I could try to do it myself. The diffs in the failing tests seem to be mostly new whitespaces (where the new code resides, with the options turned off). I assume some kind of test has to be created for when the option(s) are enabled, too.

However, I'd like to sit on this over the weekend (or maybe more) and think about it.

Sure thing. I know that we are quite a few people waiting for some kind of upstream solution in the shorter term. The people I have spoken with the same issue has gone for the hack from the Discourse thread.

[johrstrom]

I'll let the tests run, but I'm quite sure they'll fail with all these updates.

I can take care of updating the test cases if you wish.

I could try to do it myself.

Sure thing, just let me know if you need any help and yea it may likely be just adding whitespace for now.

[Cinnamals]

I'll let the tests run, but I'm quite sure they'll fail with all these updates.

I can take care of updating the test cases if you wish.

I could try to do it myself.

Sure thing, just let me know if you need any help and yea it may likely be just adding whitespace for now.

I seemingly fixed the failing tests. Would you like me to add tests for these new settings as well?

[johrstrom]

I seemingly fixed the failing tests. Would you like me to add tests for these new settings as well?

If you could, yes please and thank you!

[goodsonjr]

In other parts of mod_ood_proxy, it tends to record environment variables in the handler function and pass them on to other Lua functions as arguments. I don't have a strong feeling here, but for consistency reading OOD_SECURE_UPSTREAM would probably move to the proxy handlers which would pass through a value to set_reverse_proxy through an argument.

[Cinnamals]

In other parts of mod_ood_proxy, it tends to record environment variables in the handler function and pass them on to other Lua functions as arguments. I don't have a strong feeling here, but for consistency reading OOD_SECURE_UPSTREAM would probably move to the proxy handlers which would pass through a value to set_reverse_proxy through an argument.

Yeah, I went back and forth on this a little bit. I do agree it's more consistent that way.
I added another commit on top and will let you guys decide which is preferable. I could amend the first commit or remove this latest one, depending on your decision.

[johrstrom]

I think I actually preferred the first way you had it. This way you still have to use SetEnv directives, but then on top of that, we're modifying the function interface.

[Cinnamals]

I think I actually preferred the first way you had it. This way you still have to use SetEnv directives, but then on top of that, we're modifying the function interface.

I removed the commit on top where the proxy handler is changed, reverting to the previous solution. Everything should be in order now, assuming I understood you correctly.

[CSC-swesters]

Nice work on this feature! I took a quick glance at the template, and found a few potential typos in the httpd config comments.

[Cinnamals]

Nice work on this feature! I took a quick glance at the template, and found a few potential typos in the httpd config comments.

Thanks. Ill carefully revise the comments and clean up the commits as soon as Jeff has come to a conclusion on whether this feature is worth implementing!

[johrstrom]

I'm still looking at this, just FYI. I think we're good as far as the code goes, I just want to test it out manually to verify that it does in fact work. I just need to toggle to other PRs at the moment.

Things I'm thinking at this current time -

• I wonder if /secure-... is the right URL. Seems like they have to match the configurations and the configuration sort of necessitates that we have that name. But I'm not really in love with the URLs, so I'll have to think on that.