rabbitmq / rabbitmq-stream-dotnet-client

RabbitMQ client for the stream protocol
https://rabbitmq.github.io/rabbitmq-stream-dotnet-client/stable/htmlsingle/index.html
Other
122 stars 41 forks source link

Fix connections to server on different locales #329

Closed Noonlord closed 1 year ago

Noonlord commented 1 year ago

ToUpper is not enough, we need to use InvariantCulture to be sure that the string is converted to uppercase in a way that is consistent with the server.

Noonlord commented 1 year ago

@Zerpet I would like to have a review if possible.

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (90a6752) 92.60% compared to head (4601429) 92.49%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #329 +/- ## ========================================== - Coverage 92.60% 92.49% -0.11% ========================================== Files 113 113 Lines 9964 9946 -18 Branches 825 825 ========================================== - Hits 9227 9200 -27 - Misses 560 567 +7 - Partials 177 179 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Gsantomaggio commented 1 year ago

Thank you @Noonlord. May I ask for an example where the current implementation fails?

Noonlord commented 1 year ago

Current implemention fails when culture is set to Turkish. "Plain".ToUpper() produces PLAİN instead of PLAIN, which results in failing string comparation. Code throws "Sasl mechanism Plain is not supported by the server" IMG_7215 IMG_7216

Gsantomaggio commented 1 year ago

@Noonlord That's interesting. Can I ask for the configuration file ? I was trying with:

auth_mechanisms.1 = PLAİN
auth_mechanisms.2 = AMQPLAIN
auth_mechanisms.3 = EXTERNAL

loopback_users.guest = false

ssl_options.cacertfile = ../certs/ca_certificate.pem
ssl_options.certfile = ../certs/server_certificate.pem
ssl_options.keyfile = ../certs/server_key.pem
listeners.ssl.default = 5671
stream.listeners.ssl.default = 5551
ssl_options.verify               = verify_peer
ssl_options.fail_if_no_peer_cert = true

But PLAİN does not appear on the Mechanisms list. I don't see problems merging the PR. I am curios about reproducing the issue locally.

Thank you

Noonlord commented 1 year ago

@Gsantomaggio although I can not provide you a configuration file at the moment, I can show something else to reproduce the bug locally.

This is the current culture: resim (2)

This is the auth mechanisms response from server: resim (1)

And this is the client parameters: resim

Issue is from client parameters authentication method enumeration, Plain, converts to string as "Plain" and using TR-tr culture ToUpper() method transforms this into "PLAİN" those two strings get compared and it results to false, because of this client thinks that server does not support "Plain" authentication method.

Thanks for quick responses!

Zerpet commented 1 year ago

Thank you for the detailed explanation and the contribution!