search

rabbitmq / rabbitmq-auth-backend-http

HTTP-based authorisation and authentication for RabbitMQ
Other
199 stars 72 forks source link

reverse_dns_lookup causes havoc and destruction #38

Closed goulashify closed 8 years ago

goulashify commented 8 years ago

Good afternoon!:)

I'm through 1.5 days of debugging and magicking, I happened to be so brave that i've enabled reverse_dns_lookups (set to true) at the config meanwhile using both web-stomp and auth-http-backend plugins.

Since there's no record defined for Sock, by enabling reverse_dns_lookups it was perfectly possible to pass 

{authz_socket_info,{<<"73b85905f2f8">>,15674},{<<"172.19.0.1">>,49004}}

instead of

{authz_socket_info,{{172,19,0,3},15674},{{172,19,0,1},49024}}

to extract_address(), which would pass the bitstring (instead of the tuple) to the function inet_parse:ntoa(), which would throw an error since it only accepts ip addresses in a tuple format.

I have no major experience in Erlang, although i would suggest to define a record for the Sock so it can be verified that it's the hostname bitstring or the ip tuple type of socket it's passing around.

TL;DR: People of RabbitMQ, beware, don't use reverse_dns_lookups with web-stomp and auth-http-backend.

Let me know if there's been a decision or so regarding what approach should be taken in order to fix it, i might be able to create a PR for it.

This error happens when calling check_vhost_access after user_login_authentication, find the stack trace here please.

Cheers, -Dan

michaelklishin commented 8 years ago

Records in Erlang do not perform type checks for field values (Erlang is dynamically typed) but some kind of validation can be considered. Please take this to the mailing list.

goulashify commented 8 years ago

@michaelklishin Cool, thanks! In the meantime, would it be reasonable to educate users about this behaviour (by placing some kind of warning or something in readme.md maybe) that they shouldn't use this option when using this plugin?

michaelklishin commented 8 years ago

Unfortunately users do not read READMEs before using things. Again, this needs a discussion and we do not use GitHub issues for discussions. Please take this to the mailing list.

binarin commented 8 years ago

BTW I think it's a pretty actionable issue.

Callback spec https://github.com/rabbitmq/rabbitmq-common/blob/3885ca1967b1b340c10f7110b67fcb414f2dfda4/src/rabbit_authz_backend.erl#L49 needs to be updated to include (or be completely replaced with) #authz_socket_info{}. Maybe the record itself should also be covered with specs.

And then this plugin should be taught how to handle all inputs that are covered by the specs above.