search

shotover / shotover-proxy

L7 data-layer proxy
https://docs.shotover.io
Apache License 2.0
82 stars 16 forks source link

Invalid redis commands can break shotover's transform invariants #702

Open rukai opened 1 year ago

rukai commented 1 year ago

Pulled out of https://github.com/shotover/shotover-proxy/pull/645 which uncovered the issue:

By dumping the requests/responses in RedisSinkSingle and running cargo test cassandra_int_tests::test_cassandra_redis_cache we get:

ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: request: [Message { inner: Some(Modified { frame: Redis(BulkString(b"FLUSHDB")) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: result:  [Message { inner: Some(Parsed { bytes: b"-ERR unknown command `$7`, with args beginning with: \r\n", frame: Redis(Error("ERR unknown command `$7`, with args beginning with: ")) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: request: [Message { inner: Some(Modified { frame: Redis(Array([BulkString(b"DEL"), BulkString(b"test_cache_keyspace_simple.test_table:1")])) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: result:  [Message { inner: Some(Parsed { bytes: b"+OK\r\n", frame: Redis(SimpleString(b"OK")) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: request: [Message { inner: Some(Modified { frame: Redis(Array([BulkString(b"DEL"), BulkString(b"test_cache_keyspace_simple.test_table:2")])) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: result:  [Message { inner: Some(Parsed { bytes: b":0\r\n", frame: Redis(Integer(0)) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: request: [Message { inner: Some(Modified { frame: Redis(Array([BulkString(b"DEL"), BulkString(b"test_cache_keyspace_simple.test_table:3")])) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: result:  [Message { inner: Some(Parsed { bytes: b":0\r\n", frame: Redis(Integer(0)) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: request: [Message { inner: Some(Modified { frame: Redis(Array([BulkString(b"HGET"), BulkString(b"test_cache_keyspace_simple.test_table:1"), BulkString(b"id, x, name WHERE ")])) }), return_to_sender: false, meta_timestamp: None }]
ERROR request{id=5 source="CassandraSource"}: shotover_proxy::transforms::redis::sink_single: result:  [Message { inner: Some(Parsed { bytes: b":0\r\n", frame: Redis(Integer(0)) }), return_to_sender: false, meta_timestamp: None }]

The attempted FLUSHDB request is incorrect because it is sent as a single bulk string instead of an array of bulk strings. The single invalid FLUSHDB request is causing redis to generate two responses. This is quite concerning as it violates a documented transform invariant.

I confirmed the behavior occurs on redis 5, 6 and 7

According to the spec https://redis.io/docs/reference/protocol-spec/#resp-protocol-description

Redis uses RESP as a request-response protocol in the following way:

  • Clients send commands to a Redis server as a RESP Array of Bulk Strings.
  • The server replies with one of the RESP types according to the command implementation.

So it sounds like if we send anything other than an Array of Bulk Strings to the server than all bets are off on how redis interprets the request. I propose we add a check to the shotover sinks to filter out any such invalid requests so that shotover maintains its invariants even when given garbage input.

I suspect we will need to implement this within each redis sink so lets wait for https://github.com/shotover/shotover-proxy/pull/645 to land first.

rukai commented 1 year ago

There was an attempt to resolve this issue at https://github.com/shotover/shotover-proxy/pull/704 The issue is still valid but the attempt to resolve it stalled out due to redis being a low priority right now. We should continue where we left off on that PR in order to resolve this issue.