search

rabbitmq / rabbitmq-server

Open source RabbitMQ: core server and tier 1 (built-in) plugins
https://www.rabbitmq.com/
Other
12.2k stars 3.91k forks source link

rabbit_auth_mechanism_amqplain:handle_response/2 expects credentials to be encoded as long strings #1914

Closed thedrow closed 5 years ago

thedrow commented 5 years ago

According to the 4.2.1 in the spec shortstr is supported inside tables. However, when I add the following code to the table serialization: https://github.com/celery/py-amqp/pull/264/commits/6c627986c312cdb7575ab0616a63f56cf40c0be7

Authentication fails with the following message:

2019-03-14 08:38:37.843 [info] <0.621.0> accepting AMQP connection <0.621.0> (172.17.0.1:35468 -> 172.17.0.2:5672)
2019-03-14 08:38:40.782 [error] <0.618.0> closing AMQP connection <0.618.0> (172.17.0.1:35464 -> 172.17.0.2:5672):
{handshake_error,starting,0,{error,function_clause,'connection.start_ok',[{rabbit_binary_parser,parse_table,[<<117,101,115,116,8,80,65,83,83,87,79,82,68,115,5,103,117,101,115,116>>],[{file,"src/rabbit_binary_parser.erl"},{line,50}]},{rabbit_binary_parser,parse_table,1,[{file,"src/rabbit_binary_parser.erl"},{line,62}]},{rabbit_auth_mechanism_amqplain,handle_response,2,[{file,"src/rabbit_auth_mechanism_amqplain.erl"},{line,45}]},{rabbit_reader,auth_phase,2,[{file,"src/rabbit_reader.erl"},{line,1356}]},{rabbit_reader,handle_method0,3,[{file,"src/rabbit_reader.erl"},{line,1122}]},{rabbit_reader,handle_input,3,[{file,"src/rabbit_reader.erl"},{line,1036}]},{rabbit_reader,recvloop,4,[{file,"src/rabbit_reader.erl"},{line,477}]},{rabbit_reader,run,1,[{file,"src/rabbit_reader.erl"},{line,459}]}]}}
2019-03-14 08:38:40.845 [error] <0.621.0> closing AMQP connection <0.621.0> (172.17.0.1:35468 -> 172.17.0.2:5672):
{handshake_error,starting,0,{error,function_clause,'connection.start_ok',[{rabbit_binary_parser,parse_table,[<<117,101,115,116,8,80,65,83,83,87,79,82,68,115,5,103,117,101,115,116>>],[{file,"src/rabbit_binary_parser.erl"},{line,50}]},{rabbit_binary_parser,parse_table,1,[{file,"src/rabbit_binary_parser.erl"},{line,62}]},{rabbit_auth_mechanism_amqplain,handle_response,2,[{file,"src/rabbit_auth_mechanism_amqplain.erl"},{line,45}]},{rabbit_reader,auth_phase,2,[{file,"src/rabbit_reader.erl"},{line,1356}]},{rabbit_reader,handle_method0,3,[{file,"src/rabbit_reader.erl"},{line,1122}]},{rabbit_reader,handle_input,3,[{file,"src/rabbit_reader.erl"},{line,1036}]},{rabbit_reader,recvloop,4,[{file,"src/rabbit_reader.erl"},{line,477}]},{rabbit_reader,run,1,[{file,"src/rabbit_reader.erl"},{line,459}]}]}}

This is what I'm serializing/deserializing:

serialization.py           259 DEBUG    Deserialized the following stream:
b'\x00\n\x00\n\x00\t\x00\x00\x01\xc8\x0ccapabilitiesF\x00\x00\x00\xc7\x12publisher_confirmst\x01\x1aexchange_exchange_bindingst\x01\nbasic.nackt\x01\x16consumer_cancel_notifyt\x01\x12connection.blockedt\x01\x13consumer_prioritiest\x01\x1cauthentication_failure_closet\x01\x10per_consumer_qost\x01\x0fdirect_reply_tot\x01\x0ccluster_nameS\x00\x00\x00\x13rabbit@46d74ec1e19f\tcopyrightS\x00\x00\x00.Copyright (C) 2007-2019 Pivotal Software, Inc.\x0binformationS\x00\x00\x005Licensed under the MPL.  See http://www.rabbitmq.com/\x08platformS\x00\x00\x00\x11Erlang/OTP 21.2.7\x07productS\x00\x00\x00\x08RabbitMQ\x07versionS\x00\x00\x00\x063.7.13\x00\x00\x00\x0eAMQPLAIN PLAIN\x00\x00\x00\x05en_US'
Into these values:
[0,
 9,
 {'capabilities': {'authentication_failure_close': True,
                   'basic.nack': True,
                   'connection.blocked': True,
                   'consumer_cancel_notify': True,
                   'consumer_priorities': True,
                   'direct_reply_to': True,
                   'exchange_exchange_bindings': True,
                   'per_consumer_qos': True,
                   'publisher_confirms': True},
  'cluster_name': 'rabbit@46d74ec1e19f',
  'copyright': 'Copyright (C) 2007-2019 Pivotal Software, Inc.',
  'information': 'Licensed under the MPL.  See http://www.rabbitmq.com/',
  'platform': 'Erlang/OTP 21.2.7',
  'product': 'RabbitMQ',
  'version': '3.7.13'},
 'AMQPLAIN PLAIN',
 'en_US']
connection.py              369 DEBUG    Start from server, version: 0.9, properties: {'capabilities': {'publisher_confirms': True, 'exchange_exchange_bindings': True, 'basic.nack': True, 'consumer_cancel_notify': True, 'connection.blocked': True, 'consumer_priorities': True, 'authentication_failure_close': True, 'per_consumer_qos': True, 'direct_reply_to': True}, 'cluster_name': 'rabbit@46d74ec1e19f', 'copyright': 'Copyright (C) 2007-2019 Pivotal Software, Inc.', 'information': 'Licensed under the MPL.  See http://www.rabbitmq.com/', 'platform': 'Erlang/OTP 21.2.7', 'product': 'RabbitMQ', 'version': '3.7.13'}, mechanisms: [b'AMQPLAIN', b'PLAIN'], locales: ['en_US']
serialization.py           343 DEBUG    Serialized the following values:
({'capabilities': {'authentication_failure_close': True,
                   'connection.blocked': True,
                   'consumer_cancel_notify': True},
  'product': 'py-amqp',
  'product_version': '2.4.2'},
 b'AMQPLAIN',
 b'\x05LOGINs\x05guest\x08PASSWORDs\x05guest',
 'en_US')
Into:
b'\x00\x00\x00\x87\x07products\x07py-amqp\x0fproduct_versions\x052.4.2\x0ccapabilitiesF\x00\x00\x00M\x16consumer_cancel_notifyt\x01\x12connection.blockedt\x01\x1cauthentication_failure_closet\x01\x08AMQPLAIN\x00\x00\x00\x1d\x05LOGINs\x05guest\x08PASSWORDs\x05guest\x05en_US'

With str everything works normally. Other AMQP clients have disabled it from the get go.

I'm positive this is a bug in RabbitMQ.

thedrow commented 5 years ago

Yes, it's expecting a longstr. There's no reason why the username and password could also be a shortstr.

michaelklishin commented 5 years ago

@thedrow short strings are supported in attribute tables, just were not expected by the authentication mechanism. https://github.com/thedrow/rabbitmq-server/commit/5e97011b9869a0ff579290df6e3bf8ab315b5c6e looks reasonable. Please submit a PR?

thedrow commented 5 years ago

Yes, I'm working on it. I have to stress I don't know any Erlang at all.

michaelklishin commented 5 years ago

No worries. You thinking is on point, we are fine with tweaking the small details on your behalf.

Can you please post a script that we can use to reproduce?

thedrow commented 5 years ago

You can close that commit I linked to. Run pip install tox tox-docker and then tox -e 3.7-integration-rabbitmq -- -k test_connect.

michaelklishin commented 5 years ago

I tried the following:

It outputs

collecting ...
 t/integration/test_integration.py::test_connection.test_connect ✓                                                                                                                                                                                                10% █
 t/integration/test_integration.py::test_connection.test_connect_no_capabilities ✓                                                                                                                                                                                20% ██
 t/integration/test_integration.py::test_connection.test_connect_missing_capabilities ✓                                                                                                                                                                           30% ███
 t/integration/test_integration.py::test_connection.test_connection_close ✓                                                                                                                                                                                       40% ████
 t/integration/test_integration.py::test_connection.test_connection_closed_by_broker ✓                                                                                                                                                                            50% █████
 t/integration/test_integration.py::test_channel.test_connection_methods[method0-_on_blocked] ✓                                                                                                                                                                   60% ██████
 t/integration/test_integration.py::test_channel.test_connection_methods[method1-_on_unblocked] ✓                                                                                                                                                                 70% ███████
 t/integration/test_integration.py::test_channel.test_connection_methods[method2-_on_secure] ✓                                                                                                                                                                    80% ████████
 t/integration/test_integration.py::test_channel.test_connection_methods[method3-_on_close_ok] ✓                                                                                                                                                                  90% █████████
 t/integration/test_rmq.py::test_connect ✓                                                                                                                                                                                                                       100% ██████████

Results (0.34s):
      10 passed
      31 deselected
3.7-integration-rabbitmq docker: remove 'ed1914501a' (forced)
3.7-integration-rabbitmq run-test-post: commands[0] | ./rabbitmq_logs.sh
ERROR: InvocationError for command could not find executable './rabbitmq_logs.sh'
___________________________________________________________________________________________________________________________________ summary ____________________________________________________________________________________________________________________________________
ERROR:   3.7-integration-rabbitmq: commands failed

What I am not sure about is why do the tests pass? Do they already use the patched version? Also, is there a way to prevent the container from being force removed so that I can inspect the logs manually?

I'd be happy to run the tests without Docker against a local RabbitMQ node, in fact, that would be the easiest way to compare e.g. 3.7.13 with the patched version. Is there a document that describes how to do that?

michaelklishin commented 5 years ago

Not sure what happened to my comment :( Running tests locally on 3.7 and 2.7 with Tox suggests that they also pass with a 3.7.13 node running locally.

So I suspect there is no failing test case on that branch just yet? Any chance I can run a basic script that assumes that the library is on PYTHONPATH?

michaelklishin commented 5 years ago 
#!/usr/bin/env python3

from amqp.connection import Connection

c = Connection()
c.connect()
c.close()

also succeeds. Getting closer.

michaelklishin commented 5 years ago

I tried https://github.com/celery/py-amqp/commit/6c627986c312cdb7575ab0616a63f56cf40c0be7 and discovered that it assumes that s type designator is implemented exact as in the spec. Sadly that's not the case.

From https://www.rabbitmq.com/amqp-0-9-1-errata.html:

A1, A2: Notice how the types CONFLICT here. In Qpid and Rabbit,
         's' means a signed 16-bit integer; in 0-9-1, it means a
         short string.
thedrow commented 5 years ago

So there are no short strings in RabbitMQ?

michaelklishin commented 5 years ago

There are. When in doubt, please consult Java client, .NET client, Ruby's amq-protocol and Pika. With all respect to the Rust client mentioned in one of the issues, it is not a reference or even a mature implementation.

thedrow commented 5 years ago

Both don't implement shortstr inside tables...

michaelklishin commented 5 years ago

FWIW this is the first time I recall someone bring this up since 2010 or so :)

thedrow commented 5 years ago

I'm trying to improve the existing implementation. I'll take the errata in mind. I think we should introduce a symbol for shortstr somehow. Shell I open an issue about it?

michaelklishin commented 5 years ago

@thedrow I don't know if Qpid has implemented it since then but I don't see a reason to have short strings and long strings separately. They are not encoded meaningfully different on the wire. I kind of doubt the team would bother updating a half a dozen client libraries we maintain just to implement this feature. Unfortunately the spec has ambiguities and unfortunate decisions. We've corrected some problems that keep coming up. This one is not it.

thedrow commented 5 years ago

Idk, it saves some bytes on the wire :) What about integers? It seems like the clients you wrote don't use all the types. Why is that?

michaelklishin commented 5 years ago

If we encode strings as S and short strings as, say, s (or W or whatever), how does that save any bytes on the wire?

michaelklishin commented 5 years ago

@thedrow this is not a discussion venue. Please post your questions to the mailing list.

To answer this last one: not all types make sense for all clients. Dynamically typed languages generally don't have a way to represent unsigned types. Some statically typed languages don't have the same fine grained types as, say, C. So the types Java client does implement is what Java can represent and hopefully some other types that it is most likely to deal with.

See https://github.com/streadway/amqp/issues/391 where a user of a client that's capable of representing more types than most decided to basically not bother. The benefit is not worth the revision of every reasonably popular client. In case someone cares enough they can review every client we have a tutorial for and document the gaps with some recommendations. In some cases it won't be an easy decisions, e.g. does a JavaScript user ever care about 32-bit floats?