search

rabbitmq / rabbitmq-web-stomp

Provides support for STOMP over WebSockets
Other
89 stars 26 forks source link

HTTP basic auth for authentication #43

Closed laurentluce closed 8 years ago

laurentluce commented 8 years ago

This is a follow-up on #42. Similar to the plugin using the client cert DN/CN as the username, could we use the username/password from the HTTP authorization header? In case of web-stomp, it would overwrite the username/password from the connect message. This would allow a proxy sitting between the client and the broker to set those values.

michaelklishin commented 8 years ago

@essen do you see WebSocket-based systems use HTTP authentication much? Is it worth supporting?

essen commented 8 years ago

I wouldn't say "much" but it happens. Though it's generally as part of a website/web app rather than just Websocket.

I don't think it would be too difficult to add an option that enables this behavior. It sounds very specific though, I can't see anyone wanting to enable it other than this exact use case. It should definitely be disabled by default at the very least.

michaelklishin commented 8 years ago

I agree with disabling this by default.

laurentluce commented 8 years ago

@michaelklishin Thanks for considering this new feature. I am not familiar with the code base but if you give me some pointers, I can try to add it.

essen commented 8 years ago

I am planning the following changes:

This requires changes both in this and the STOMP plugins.

I am currently not sure about one point though:

essen commented 8 years ago

Reading the code it looks like SockJS passes headers to the SockJS handler, but I get an empty list. Something might need to be fixed there also.

essen commented 8 years ago

This is ready for testing and review. There are four patches, against rabbitmq-web-stomp, rabbitmq-stomp, sockjs-erlang and rabbitmq-website. Enjoy!

laurentluce commented 8 years ago

@essen Thanks for working on this new feature. Very cool! I will help with testing.

michaelklishin commented 8 years ago

When use_http_auth is set to true and no credentials are provided by the client, I think we should use STOMP plugin's default credentials, just like we do for STOMP in other cases.

essen commented 8 years ago

Then this needs more work. :-)

essen commented 8 years ago

@michaelklishin Pushed an update addressing your comments including a new test and updated documentation.

michaelklishin commented 8 years ago

Fixed in stable. @laurentluce would you be interested in testing a one-off build?

laurentluce commented 8 years ago

@michaelklishin which branch should i use? Should I follow the instructions from https://www.rabbitmq.com/build-server.html to build a debian package?

michaelklishin commented 8 years ago

@laurentluce contact me at michael in RabbitMQ domain and I will send you a build.

laurentluce commented 8 years ago

@essen @michaelklishin

Initial testing results with build: rabbitmq-server_3.6.1.150-1_all.deb. I found one issue, see item 3 below:

  1. use_http_auth set to true and Basic auth header with valid username/password set during initial handshake. CONNECT without login/passcode succeeds. Expected.
  2. use_http_auth set to true, no Basic auth header and STOMP default user not set. CONNECT without login/passcode errors out. Expected.
  3. use_http_auth set to true, no Basic auth header and STOMP default user set. CONNECT without login/passcode errors out. Not Expected since it should succeed. It works if I use plain STOMP instead of web-stomp.

Config used for test 3:

[ {rabbitmq_web_stomp, [{use_http_auth, true}]}, {rabbitmq_stomp, [{default_user, [{login, "guest"}, {passcode, "guest"}]}]} ].

STOMP frames:

Opening Web Socket...
Web Socket Opened...
>>> CONNECT
accept-version:1.1,1.0
heart-beat:10000,10000

<<< ERROR
message:Processing error
content-type:text/plain
version:1.0,1.1,1.2
content-length:16

Processing error
michaelklishin commented 8 years ago

To me it makes no sense to set the option and provide no header.

Comparing with STOMP is not fair because STOMP has no equivalent of basic HTTP auth.

On 1 abr 2016, at 22:19, Laurent Luce notifications@github.com wrote:

Initial testing results with build: rabbitmq-server_3.6.1.150-1_all.deb. I found one issue, see item 3 below:

use_http_auth set to true and Basic auth header with valid username/password set during initial handshake. CONNECT without login/passcode succeeds. Expected. use_http_auth set to true, no Basic auth header and STOMP default user not set. CONNECT without login/passcode errors out. Expected. use_http_auth set to true, no Basic auth header and STOMP default user set. CONNECT without login/passcode errors out. Not Expected since it should succeed. It works if I use plain STOMP instead of web-stomp. Config used for test 3:

[ {rabbitmq_web_stomp, [{use_http_auth, true}]}, {rabbitmq_stomp, [{default_user, [{login, "guest"}, {passcode, "guest"}]}]} ].

STOMP frames:

Opening Web Socket... Web Socket Opened...

CONNECT accept-version:1.1,1.0 heart-beat:10000,10000

<<< ERROR message:Processing error content-type:text/plain version:1.0,1.1,1.2 content-length:16

Processing error — You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

essen commented 8 years ago

I'm surprised it doesn't work actually because after you commented https://github.com/rabbitmq/rabbitmq-web-stomp/issues/43#issuecomment-198995388 I made the change to support it. :-)

laurentluce commented 8 years ago

Yes, I tested it based on that comment.

laurentluce commented 8 years ago

@michaelklishin @essen Should we support or not the following? use_http_auth set to true, no http auth then fallback to STOMP default credentials.

laurentluce commented 8 years ago

@michaelklishin @essen A more general error is that when use_http_auth is set to true and no Basic auth header is present, STOMP connect with or without login/passcode always errors out.

essen commented 8 years ago

The way it's been done is that use_http_auth expects credentials to be sent in authorization header (CONNECT credentials are always ignored). The behavior is otherwise the same, in that if there's no such header the default credentials are used (and anything in CONNECT is still ignored).

If something like "authorization; or else connect; or else default" is desirable then this needs more work, because right now it's not what it does.

laurentluce commented 8 years ago

@essen The following does not work for me with the 3.6.2 build Michael gave me:

"The behavior is otherwise the same, in that if there's no such header the default credentials are used (and anything in CONNECT is still ignored)."

Config:

[ {rabbitmq_web_stomp, [{use_http_auth, true}]}, {rabbitmq_stomp, [{default_user, [{login, "guest"}, {passcode, "guest"}]}]} ].
essen commented 8 years ago

Weird because we have a test for this: https://github.com/rabbitmq/rabbitmq-web-stomp/blob/master/test/src/rabbit_ws_test_raw_websocket.erl#L96

Note that this is only supposed to work when using /ws (raw Websocket), it will not work with /stomp (SockJS).

laurentluce commented 8 years ago

@essen I am using raw Websocket: /ws.

The test expects an error though since the login/passcode are invalid so it does not prove it can work.

I am getting the following frame back:

Opening Web Socket...
Web Socket Opened...
>>> CONNECT
accept-version:1.1,1.0
heart-beat:10000,10000

<<< ERROR
message:Processing error
content-type:text/plain
version:1.0,1.1,1.2
content-length:16

Processing error
essen commented 8 years ago

You are correct that we are missing a test case for this. I can check when I get back to work (possibly next week). Thanks.

essen commented 8 years ago

I think we'll probably need a new ticket though. Can you open one?

laurentluce commented 8 years ago

@essen Sure, will do.