beobal commented on code in PR #3655:
URL: https://github.com/apache/cassandra/pull/3655#discussion_r1858640750
##########
src/java/org/apache/cassandra/transport/CQLMessageHandler.java:
##########
@@ -182,6 +190,13 @@ protected boolean
processOneContainedMessage(ShareableBytes bytes, Limit endpoin
// max CQL message size defaults to 256mb, so should be safe to
downcast
int messageSize = Ints.checkedCast(header.bodySizeInBytes);
+ if (authMessageTooBig(messageSize))
Review Comment:
Any message being handled here *must* be < 128KiB or else it would not fit
in a single frame, so I would argue we don't need this check at all. Do you
think there's a use case where operators might set the max size of an auth
message smaller than this?
##########
src/java/org/apache/cassandra/transport/CQLMessageHandler.java:
##########
@@ -518,10 +533,23 @@ protected boolean
processFirstFrameOfLargeMessage(IntactFrame frame, Limit endpo
// max CQL message size defaults to 256mb, so should be safe to
downcast
int messageSize = Ints.checkedCast(header.bodySizeInBytes);
receivedBytes += buf.remaining();
+
+ if (authMessageTooBig(messageSize))
Review Comment:
I would prefer we handle this by disallowing "auth" messages extending
beyond a single frame. That means strictly that `STARTUP, CREDENTIALS, OPTIONS,
AUTH_RESPONSE` message received here can be considered too large and rejected,
regardless of their actual size. This would also have the advantage that we can
identify the message type by inspecting the `Header` and we would no longer
need to hold a reference to the `ServerConnection`.
##########
src/java/org/apache/cassandra/transport/CQLMessageHandler.java:
##########
@@ -518,10 +533,23 @@ protected boolean
processFirstFrameOfLargeMessage(IntactFrame frame, Limit endpo
// max CQL message size defaults to 256mb, so should be safe to
downcast
int messageSize = Ints.checkedCast(header.bodySizeInBytes);
receivedBytes += buf.remaining();
+
+ if (authMessageTooBig(messageSize))
+ {
+ // we raise a fatal error and close the connection,
+ // so it does not make sense to continue frames processing
+ ClientMetrics.instance.markRequestDiscarded();
+ return false;
+ }
LargeMessage largeMessage = new LargeMessage(header);
-
- if (throwOnOverload)
+ if (messageSize > MAX_CQL_MESSAGE_SIZE)
Review Comment:
This is a good defence against accepting a mutation bigger than the max size
that the commitlog will accept, but I think we can go further to protect
against runaway resource consumption.
I had imagined that we would override `onIntactFrame` and the `tooBig` flag
would have the effect that the `LargeMessage` would not hold on to the
individual frame buffers, knowing that it was not going process the message (it
could also do the same if the message is marked overloaded).
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -822,6 +822,25 @@ else if (conf.commitlog_segment_size.toMebibytes() >= 2048)
else if (conf.commitlog_segment_size.toKibibytes() < 2 *
conf.max_mutation_size.toKibibytes())
throw new ConfigurationException("commitlog_segment_size must be
at least twice the size of max_mutation_size / 1024", false);
+ if (conf.native_transport_max_message_size == null)
+ {
+ conf.native_transport_max_message_size = new
DataStorageSpec.LongBytesBound(
+ Math.min(conf.max_mutation_size.toBytes(),
+ Math.min(
+
conf.native_transport_max_request_data_in_flight.toBytes(),
+
conf.native_transport_max_request_data_in_flight_per_ip.toBytes()
Review Comment:
These two settings can be modified at runtime, but
`native_transport_max_messages_size` can not. Even if it could, it's memoized
in `CQLMessageHandler.MAX_CQL_MESSAGE_SIZE` which is final.
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -269,6 +269,8 @@ public MemtableOptions()
public int native_transport_max_threads = 128;
@Replaces(oldName = "native_transport_max_frame_size_in_mb", converter =
Converters.MEBIBYTES_DATA_STORAGE_INT, deprecated = true)
public DataStorageSpec.IntMebibytesBound native_transport_max_frame_size =
new DataStorageSpec.IntMebibytesBound("16MiB");
+ public volatile DataStorageSpec.LongBytesBound
native_transport_max_message_size = null;
+ public volatile DataStorageSpec.LongBytesBound
native_transport_max_auth_message_size = new
DataStorageSpec.LongBytesBound("128KiB");
Review Comment:
Rather than having two separate settings for max message and max auth
message size, why don't we enforce a rule that any message received before the
server is ready must fit in a single frame?
That would naturally limit those messages to 128KiB and would not need
operators to have to think about what is a reasonable max size for auth
messages. If we really want to accommodate an authentication scheme which does
require large messages, we can add a boolean setting to allow large messages,
which I think would still be conceptually simpler, in which case auth messages
would still be subject to the `MAX_CQL_MESSAGE_SIZE` check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
