search

envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25.02k stars 4.82k forks source link

Running Lua filter as fail closed #14980

Open emike922 opened 3 years ago

emike922 commented 3 years ago

Is there a way to run Lua filters as "fail closed"? I have observed that the requested resources are served normally if an error/exception occurs in the Lua script. I would rather see 503 (at least for some scripts, s.a. a Lua auth filter). Can this be done/configured, or do I need to be super defensive (paranoid) when writing the filter?

antoniovicente commented 3 years ago

cc @dio

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

emike922 commented 3 years ago

Still interested

pcmoritz commented 3 years ago

I'm also interested in this! Especially because lua filters are advertised as filters, passing the filter if there is an error in the lua program seems like a very dangerous default to me.

twghu commented 3 years ago

@emike922 Should the default state return 503 or 500? While it could be argued the service is unavailable (ie it's broke) is a fair assessment and maybe a 503 error would make better sense from a security perspective but it seems to contradict the 500 error.

It is likely safer to fail/close on default, I suspect this might break some existing installations but it does provide more safety.

emike922 commented 3 years ago

@twghu I think we should aim to emulate the Wasm filter, which does a similar thing based on its fail_open setting: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/wasm/v3/wasm.proto#extensions-wasm-v3-pluginconfig

twghu commented 3 years ago

@emike922 ack, good call.

twghu commented 3 years ago

Added commit. Checking configuration option, maybe for now a runtime guard may be sufficient?

dio commented 3 years ago

@twghu are you going to raise a PR on this? Either API or runtime guard seems fine. But yeah I agree this should be fail/close on by default for now (however, runtime guard promotes running the new behavior by default (if the new behavior is "fail/open") so be careful when introducing this as a breaking change).