Provide an explicit deny URL option #38
Conversation
Camo currently ships with the option to provide a file path containing a number of hostnames to `--allow-list` which limits where images will be proxied from. This works great for restricting the source if you control the origin or have a small number of origins. What it doesn't cover very well is to explicitly deny access to a resource on a fine grained level. Take for example, you use Camo to proxy all third party assets to put a CDN in front of them and then one of the origins used via Camo is hosting malicious or inappropriate material. There currently isn't a sane way of preventing access to this particular resource once anyone has the signed HMAC URL. There are work arounds to kill the connection however they require drastic action like rolling the HMAC which unless co-ordinated well, results in a service interruption. This change aims to address the above scenario by exposing a way to define the inverse, an explicit deny list. It works on a similar principle to the `allow-list` with one notable exception; it loosely matches the outgoing URL. Instead of restricting based on hostname or asset filename, it can match anywhere using `string.Contains`. This brings with it great power and a potential to shoot yourself in the foot if misconfigured. The thought behind this is that Re2 is pretty poor at operations like negative lookaheads (specifically because it conflicts with the O(n)-time guarantees of the library[1]) and I didn't want to overload options with variants for hostname, path, etc. An added benefit to this approach is that you're able to deny a single resource on a domain and not the whole domain which is useful for shared image hosting services. [1]: https://groups.google.com/forum/#!topic/golang-nuts/7qgSDWPIh_E
jacobbednarz
force-pushed
the
explicit-deny-option
branch
from
132bb79
to
045b703
Compare
|
I’ll have to think a bit about this one, as at first glance this seems like a pretty large possible footgun. Some questions off the top of my head:
|
Is it can definitely be a big foot gun, no doubt about it. I also wouldn't be expecting everyone to use this as it seems more of a larger installation feature where abuse is more prominent.
The biggest driver was that this allowed blocking parts of the URL that needed to vary the hostname or protocol. For example, if
No static benchmarks as of yet however I'm in the process of capturing some production traffic to replay on a canary installation to better gauge the real world application.
TBH, I love that
I have considered using something like Redis to store this type of data but again, didn't want to couple other components that might have been overkill for the solution.
Definitely not something I've considered but will look into to see if it would be a better fit for our use case.
I would have proposed consolidating on a single flag to handle both allow/deny via regex however as I mentioned in the description, Re2 and lack of negative assertions would make this way more verbose than needed. |
<3
Yeah. There is also the possibility of having go-camo use a proxy as an upstream, and letting that proxy to the filtering. I think there only change required to have go-camo use an upstream proxy would be a small addition to the transport settings. As you said though, that starts to stray pretty far from "just works" category. I've looking into things like embedding tengo or lua and letting people write their own url validators, but that's a lot of overhead and then plugins can become a bit fragile or problematic on their own. I'll think about some possible solutions. I'm concerned that a list would eventually add lots of processing overhead if the list was long, but something like a trie with 'globbing' support might not be too bad. I'd have to write some code and play around with the idea though. |
|
@jacobbednarz I played around with changing how filtering works over the few days. Here is the design doc if you are interested: And here is the WIP branch (beware dragons): My thinking is this would allow filtering of both domains and urls -- for best performance of course, you wouldn't want to configure any, as it does add some overheard. Currently seems (more testing and benchmarking needed) a bit faster than regexes though, so that's something. |
|
That design document looks great! Cheers for taking the time to codify your thoughts. I'm digging the syntax and flexibility it proposes. I see in the proposed changeset, you've still got an allow and filter flag. Would this mean you wouldn't want to combine the rules into a single CLI parameter that controlled both? I could see the syntax potentially evolving to include the restriction type as a part of the line (i.e. My second sub-thought here is maybe moving towards a permission model where you needed to be explicit about proxying all domains (similar to AWS IAM's implicit deny for all policies by default). This would have the caveat of requiring the
Also avoids the issue of nasty regexes and negative assertions 🙂 |
They are indeed still separate in the current WIP design, the reason being that if you set an allow-filter, it inverts the effective acceptance model to deny-by-default unless specifically allowed. This is similar to the workflow/model that the allow-list filtering currently uses. If a single filter list were used, it may not be as clear to people how it works: an allow rule could slip in accidentally mucking things up; someone thinking the rules are operated on in order (they aren't really) so they would want an allow at the bottom; and so on. Since I'm not sure how the two modes could clearly interact in a non-confusing way in a single file (without deep a-priori knowledge), I think it is still slightly clearer and more straightforward to keep them split for now. I'm open to other ideas or thoughts on this though for sure. |
|
I agree if definitely can cause some confusion and I'm not willing to
allow better get in the way of perfect here. I think the solution you've
thrown up is better maintainability wise than my PR so I'm in favour.
We can always cross the single list bridge if either of these get too
unwieldy or start to encounter code paths too complex due to the
competing rulesets.
|
|
Well.... I changed my mind about unifying the filter lists. heh. To try to avoid the "user confusion" aspect, I'll just emit a noisy message if the user supplies both allow and deny filters -- a message that the allow filter takes precedence and the deny filters are ignored. Updated https://github.com/cactus/go-camo/blob/filtering-redux/FILTER_FORMAT.md doc as well. |
😂 The joy of persuasion.
I think this fine. The two use cases I can think of here are:
I don't think you should be using both at the same time as if you have a dedicated origin (or a few) you can block access by removing the asset. |
|
Tests on the other branch are passing now. I tried to do some load testing, and couldn't generate enough load locally to see much of a difference. I think my test env is bottlenecked elsewhere. I did some profiling with pprof and optimized a few things. Got a nice speed up in pprof results at least! |
|
Closing PR, as this is solved in a different way in cc0a2a6 now. Thanks for the feedback and ideas @jacobbednarz !! <3 |
|
No worries at all! If you want some production load testing, I’m happy to
roll out a couple of canary hosts to our fleet and report back any issues.
Will you be cutting a RC for this or should I just build from source?
|
|
I can cut an RC and sign and upload it tomorrow if you want. You can also build it from master yourself if desired. Thanks! I'd love to hear some production comparisons:
|
|
I'm happy to wait for an RC to roll to production as it makes it easier to confirm what we have is what will be released without any weirdness on my end. Do you have any specific conditions you want tested? I was thinking:
|
|
Waiting for an RC would be safest. I'll do some more testing locally in the meantime to try and make sure no wheels fall off. 👍 Those numbers look great. While testing rules keep in mind that the data new structure for rules will naturally avoid duplicates (it is trie-like), so adding the exact same rule multiple times shouldn't add any additional overhead. |
|
@jacobbednarz An alpha is posted: https://github.com/cactus/go-camo/releases/tag/v2.0.0-alpha.1 |
|
I managed to roll this out to a few canaries in our fleet this morning and have good news. Up until 10k deny rules, we see not noticeable change. At 10k deny rules, the difference is an additional 0.8% CPU utilisation (1.8 from 1.0%) and memory usage goes from 120MB to 246MB. This essentially doubles the resource utilisation in our testing but the initial footprint is already really low this is barely a concern. Below are the nitty gritty details if you're interested. Rollout with annonations Rollout w/o annonations (for clearer comparison). The spike at the end was user error, no Given we see ~ 5k rpm to this fleet, I only needed to evaluate each step for about an hour until resource utilisation steadied and we got reasonable amounts of information. Generating the deny rules using the following snippet. for i in $(seq 1 10); do echo "deny|s|example.com|i|/image${i}.jpg" >> /etc/camo/filter_ruleset.txt; donebaseline1 deny rule100 deny rules10k deny rules |
|
Hmm. Interesting results. Those filters should only match example.com and subdomains, so it should be a fairly consistent cpu overhead on the filtering rules. I did use the new data structure for some of the existing exclusions (localhost/localdomain filtering), but those should be constant overhead and present in the first canary timeslice as well. My guess is the additional cpu you are seeing with very large rulesets might be the go garbage collector visiting all the nodes in the url match tree. This is great data so far. Thanks a lot! <3 |
|
@jacobbednarz I made some changes and got the memory usage down to around 200mb or so for 100k case insensitive rules (using the same method you used to generate yours). Before my changes the 100k case insensitive rules config was using a bit over 1.4gb of memory, which was a bit too much. lol. yikes! Performance did slow down a bit with that many rules (possibly due to GC overhead, with the GC visiting all those nodes??). I'll dig into it a bit more at some point. In the meantime, I posted a new alpha build with the fixes: |
|
Awesome, I've rolled out alpha2 to the fleet and will report back any issues we encounter. |
|
@jacobbednarz How did alpha2 perform in your rollout? |
|
So far so good. It's been running over all the fleet for the last few days and there looks to be positive improvements on all fronts (looks to be about 10-12% in most cases). I haven't gone as far as to test the new build out with 100k deny rules however I did try with 1k and it remained stable. Would you like me to try the higher end numbers or are you happy with the results as is? |
|
Good to hear the performance is much more in line with expectations with the alpha2 built. 👍
I think the current results are fine. I'll cut a mainline release soon. Just a note: I really appreciate the testing you have done so far. Super awesome! ❤️ |
|
It's no issue at all :) The change has been a great team effort.
This is actually solving an issue for us so more than happy to get this
tested and confirmed working on those nasty edge cases before everyone else.
I do want to dive into the Camo internals (eventually) and see if we can
get the performance back down to before this change however at the
moment it isn't _that_ bad that I need to do it immediately. I've got a
14 hour flight coming soon so perhaps that will be where it gets some
priority :P
|
|
I just did a formal release of v2.0.0. It would be interesting to see some before/after cpu comparisons with v2.0.0 (or alpha2, which is mostly the same aside from documentation fixes/updates) vs v1.1.7. I'm curious about the final overhead difference without any filtering rules, as well as comparisons with filtering rules. Anyway, have a safe flight! |
Camo currently ships with the option to provide a file path containing a
number of hostnames to
--allow-listwhich limits where images will beproxied from. This works great for restricting the source if you control
the origin or have a small number of origins.
What it doesn't cover very well is to explicitly deny access to a
resource on a fine grained level. Take for example, you use Camo to
proxy all third party assets to put a CDN in front of them and then one
of the origins used via Camo is hosting malicious or inappropriate
material. There currently isn't a sane way of preventing access to this
particular resource once anyone has the signed HMAC URL. There are work
arounds to kill the connection however they require drastic action like
rolling the HMAC which unless co-ordinated well, results in a service
interruption.
This change aims to address the above scenario by exposing a way to
define the inverse, an explicit deny list. It works on a similar
principle to the
allow-listwith one notable exception; it looselymatches the outgoing URL. Instead of restricting based on hostname or
asset filename, it can match anywhere using
string.Contains. Thisbrings with it great power and a potential to shoot yourself in the foot
if misconfigured. The thought behind this is that Re2 is pretty poor at
operations like negative lookaheads (specifically because it conflicts
with the O(n)-time guarantees of the library) and I didn't want to
overload options with variants for hostname, path, etc. An added benefit
to this approach is that you're able to deny a single resource on a
domain and not the whole domain which is useful for shared image hosting
services.