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.

Sign up for GitHub

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

Jump to bottom

One of ConnectionManager constructors ignores TLS settings #120

Closed
yuriyostapenko opened this issue Mar 11, 2024 · 3 comments · Fixed by #121
Closed

One of ConnectionManager constructors ignores TLS settings #120

yuriyostapenko opened this issue Mar 11, 2024 · 3 comments · Fixed by #121

Comments

@yuriyostapenko
Copy link
Contributor

yuriyostapenko commented Mar 11, 2024 •
edited
Loading

Thanks for the great library!

I am currently trying to make it work over TLS and encountered the following problem:

var bus = Configure.With(activator)
    .Logging(l => l.Console())
    .Transport(t => t.UseRabbitMq($"amqps://{username}:{password}@host:port/", "test-queue"))
    .Start();

fails with error:

Exception thrown: 'RabbitMQ.Client.Exceptions.BrokerUnreachableException' in RabbitMQ.Client.dll: 'None of the specified endpoints were reachable'
 Innermost exception 	 System.IO.IOException : connection.start was never received, likely due to a network timeout
...

This happens due to ConnectionManager not configuring endpoints correctly. In this case, original connection string has the right schema and host name, yet an AmqpTcpEndpoint created here

Rebus.RabbitMq/Rebus.RabbitMq/Internals/ConnectionManager.cs

Line 129 in 240baa5

return new AmqpTcpEndpoint(new Uri(uriString));

does not get its SslOptions initialized.

The other constructor initializes them properly

Rebus.RabbitMq/Rebus.RabbitMq/Internals/ConnectionManager.cs

Line 76 in 240baa5

return new AmqpTcpEndpoint(new Uri(endpoint.ConnectionString), ToSslOption(endpoint.SslSettings));

so if another UseRabbitMq overload is used (the one taking IList<ConnectionEndpoint> endpoints), everything seems to work.

Workaround:

var uri = new Uri($"amqps://{username}:{password}@host:port/");
var bus = Configure.With(activator)
    .Logging(l => l.Console())
    .Transport(t => t.UseRabbitMq(
                        new[] {
                            new ConnectionEndpoint()
                            {
                                ConnectionString = uri.ToString(),
                                SslSettings = new SslSettings(true, uri.Host /* required for SNI to work */)
                            }
                        },
                        "test-queue")
                    )
    .Start();

I would gladly submit a PR to fix this, but I am a bit puzzled by the problematic constructor supporting multiple endpoints and I do not understand how it's supposed to be used, so as to not break that flow. I also am not sure how that has to co-exist with customizer. Any customizations to connection settings seem to be ignored by current code.
@mookid8000
Copy link
Member

If you mean Rebus' way of allowing multiple endpoints to be passed to the UseRabbitMq method, it's just supposed to be used to seed the client's list of endpoints, which enables it to do a failover in case the current one becomes unreachable.

My guess is that your own code that creates SslSettings from the URI string should be able to fix it, although I would expect it to require a scheme check (i.e. should probably not pass SslSettings if scheme is amqp).

A PR would be much appreciated 🙂

@yuriyostapenko
Copy link
Contributor Author

The complication is that there are potentially more useful advanced configuration options, besides just "tls on/off", such as protocol version, SNI hostname, trust settings and so on that are not encodable in the uri string, but can be set up in customizer.

And such customizations, while unlikely, may need to be different per endpoint.
But yes, at least a basic scheme based condition will fix 90% TLS support scenarios.

@yuriyostapenko yuriyostapenko mentioned this issue Mar 13, 2024
Merged
@mookid8000
Copy link
Member

It's fixed in Rebus.RabbitMq 9.1.0, which is on NuGet.org now 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Configure SslOptions for endpoints using amqps scheme yuriyostapenko/Rebus.RabbitMq
2 participants
@mookid8000 @yuriyostapenko