Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit reason field to 255 characters in authResponse, while sending close method to amqp clients #84

Merged
merged 1 commit into from
Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion authproto/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ The service will receive POST request from amqpprox proxy with following fields
### Response
The service should respond to amqpprox proxy with following fields in [protobuf format](./authresponse.proto)
- Auth Result - This will decide, whether to allow or deny clients. It is a enum type, takes ALLOW or DENY value only.
- Reason - Reason for the returned auth result. It is an optional field, but good to specify.
- Reason - Reason for the returned auth result. It is an optional field, but good to specify. The reason(reply-text) field is defined as short string in AMQP 0.9.1 protocol implementation. So it will be truncated before sending to the amqp clients, if the size of the string is more than 255 characters.
- SASL auth data - It is an optional field. In case of absence, the START-OK AMQP connection method, received from clients will be sent to the broker without any modification during handshake.
- Broker mechanism - Authentication mechanism field for START-OK AMQP connection method. This will be injected into START-OK AMQP connection method to send to the broker during handshake. E.g. PLAIN
- Credentials - Response field for START-OK AMQP connection method. This will also be injectd into START-OK AMQP connection method to send to the broker during handshake. E.g.'\0user\0password'
Expand Down
2 changes: 2 additions & 0 deletions authproto/authresponse.proto
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ message AuthResponse {
DENY = 1;
}
AuthResult result = 1;

// Maximum 255 characters
string reason = 2;
SASL authData = 3;
}
2 changes: 2 additions & 0 deletions libamqpprox/amqpprox_constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ class Constants {
{
return "authentication_failure_close";
}

static constexpr std::size_t shortStringLimit() { return 255; }
};

}
Expand Down
11 changes: 10 additions & 1 deletion libamqpprox/amqpprox_session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -582,9 +582,18 @@ void Session::disconnectUnauthClient(const FieldTable &clientProperties,
&fv, Constants::authenticationFailureClose()) &&
fv.type() == 't') {
bool authenticationFailureClose = fv.value<bool>();

// Truncate short string longer than 255 characters.
// reply-text(reason) field is defined as short string in AMQP
// 0.9.1 protocol.
const size_t sendSize =
std::min(reason.length(), Constants::shortStringLimit());

if (authenticationFailureClose) {
d_connector.synthesizeCustomCloseError(
true, Reply::Codes::access_refused, reason);
true,
Reply::Codes::access_refused,
reason.substr(0, sendSize));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice that this is substr on string_view 👍

sendSyntheticData();
}
}
Expand Down
3 changes: 2 additions & 1 deletion libamqpprox/amqpprox_types.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <amqpprox_types.h>

#include <amqpprox_buffer.h>
#include <amqpprox_constants.h>
#include <amqpprox_fieldtable.h>
#include <amqpprox_fieldvalue.h>
#include <amqpprox_logging.h>
Expand Down Expand Up @@ -86,7 +87,7 @@ bool Types::encodeShortString(Buffer &buffer, const std::string &string)
return false;
}

if (string.size() > 255) {
if (string.size() > Constants::shortStringLimit()) {
return false;
}

Expand Down