feat: redis sandboxing #50
Conversation
will89
left a comment
will89
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Would you mind updating the README with this new functionality?
|
reminder to self: add this too https://github.com/redis-rb/redis-client |
mjsmithnh
left a comment
mjsmithnh
left a comment
There was a problem hiding this comment.
Looks good to me. I'm not a Redis expert by any means but I'm assuming this can't be bypassed via something like a subquery, or if that is the case we don't care?
|
@gremerritt - Mind taking a look at this PR? |
There was a problem hiding this comment.
Thanks for this! This has been on my backlog of things to think about for a while, it'll be a great addition.
A few high-level thoughts/questions:
- Have you thought about using ACLs on the server instead of manually blocking commands on the client? It would only work for redis 6+, but it (might) involve less client patching.
- I notice that scripting commands are missing from the whitelist (eg.
evalsha). We have some scripts that are effectively read-only. It's a grey area because they could just as well be mutating. I'm also inclined to say we should allowloadcommands. We have our own redis client that registers necessary scripts effectively at boot time, and we'd need a solution there to not load scripts in certain cases. redis-rbusesredis-clientinternally, so eventually we may be able to drop the::Redissupport. I'm guessing we want to supportredis-rb v4for now though.
| module Patches | ||
| module Sandbox | ||
| module RedisReadonly | ||
| READ_COMMANDS = [ |
There was a problem hiding this comment.
I'm vaguely worried about this list falling out of date as new commands are added, but I think it's fine since we're whitelisting.
There was a problem hiding this comment.
Also a nit, can we alphabetize these?
| 'zrevrangebylex', 'sunion', 'randomkey', 'zrevrank', 'xrange', 'xpending', 'hgetall', 'getrange', 'exists', | ||
| 'keys', 'georadius_ro', 'lolwut', 'hscan', 'object', 'zlexcount', 'type', 'geohash', 'touch', 'hkeys', | ||
| 'strlen', 'scard', 'substr', 'zinter', 'srandmember', 'mget', 'xinfo', 'geosearch', 'zunion', 'xread', | ||
| 'pfcount', 'xrevrange', 'sscan', 'memory', 'bitfield_ro', 'dump', 'host:', 'sinter', 'getbit', 'zcard' |
|
|
||
| def self.raise_exception_on_write_command(command, service = ::Redis) | ||
| unless READ_COMMANDS.include?(command.downcase.to_s) | ||
| raise service.to_s.constantize::CommandError.new( |
| end | ||
|
|
||
| module RedisPatch | ||
| def process(commands) |
There was a problem hiding this comment.
Does this work for pipelined/multi commands? I think this may only catch this first command being executed.
| end | ||
|
|
||
| module RedisClientPatch | ||
| def call(commands, redis_config) |
There was a problem hiding this comment.
Likewise I don't think this covers pipelines or multis?
| end | ||
| end | ||
|
|
||
| module RedisClientPatch |
There was a problem hiding this comment.
redis-client has great middleware support. It might be easier to implement this via a middleware (in the config object).
|
Re: the scripting commands, let's go forward with what you have currently (ie. blocking scripting commands). Can you add a config-level toggle to disable the new redis support though? We'll likely come back to this and add some additional scripting configuring support, but I don't want to block you from getting this out either. |
5 participants
This adds Redis sandboxing to safer_rails_console.
It allows for reading, but any attempt to call a write method raises an error