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.

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

add proper SSL support #130

Closed
62mkv opened this issue Apr 14, 2020 · 13 comments
Closed

add proper SSL support #130

62mkv opened this issue Apr 14, 2020 · 13 comments
Assignees
Labels
for/stackoverflow Questions are best asked on SO or Gitter

Comments

@62mkv
Copy link

62mkv commented Apr 14, 2020

Motivation

Was not able to identify any way of enabling SSL connection with this library.

Desired solution

Can we have a non-URI support for that, like with Spring AMQP:

spring.rabbitmq.ssl.enabled=true
spring.rabbitmq.ssl.validate-server-certificate=false

and a properly documented example?

Considered alternatives

Had to supply URI property starting with "ampqs". Which is not very convenient..

Additional context

@acogoluegnes
Copy link
Contributor

TLS is configured at the ConnectionFactory level. See the documentation for the sender and receiver on how to set the connection factory.

TLS configuration is covered in the Java client documentation and in details in the TLS guide.

I'll some links to the documentation.

@acogoluegnes acogoluegnes self-assigned this Apr 15, 2020
@acogoluegnes acogoluegnes added the for/stackoverflow Questions are best asked on SO or Gitter label Apr 15, 2020
acogoluegnes added a commit that referenced this issue Apr 15, 2020
@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

so you don't see a value in having Properties support for that? I am not a big expert in this but this bit seems to be volatile enough to put it into configuration properties to allow quick redefining as necessary for the specific environment. Or at least, I assumed that much based on prior Spring AMQP experience..

@acogoluegnes
Copy link
Contributor

I don't think it's the goal of the library to support properties-based configuration. Spring Boot provides the support for a configuration file, not Spring AMQP.

I understand your point though and having Reactor RabbitMQ ready to use in a Spring Boot application would be cool. This could be done in the existing Spring AMQP starter or in its own starter. This is something to discuss with the Spring AMQP and Spring Boot teams.

@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

the thing is: I already use Reactive RabbitMQ in Spring WebFlux app, bootstrapping properties like this:

    private Properties buildPropertiesFromSpringProperties(Map<String, String> rabbitConfig) {
        Properties properties = new Properties();
        final String prefix = "rabbitmq.";
        for (Map.Entry<String, String> entry : rabbitConfig.entrySet()) {
            properties.setProperty(prefix + entry.getKey(), entry.getValue());
        }
        return properties;
    }

So I am not asking for any special kind of Spring integration, just for adding support for SSL bootstrap via ConnectionFactory#load method:

        ConnectionFactory connectionFactory = new ConnectionFactory();
        connectionFactory.useNio();
        connectionFactory.load(buildPropertiesFromSpringProperties(properties.getRabbitmq()));

Currently I only can survive thanks to support for a "URI" property, that, when it start with "amqps://", enables SSL. So I assume all the bits are already there, just need some polishing to achieve more straight-forward experience

@acogoluegnes
Copy link
Contributor

acogoluegnes commented Apr 15, 2020

OK, so you'd like to use the TLS configuration keys from Spring Boot for Spring AMQP in ConnectionFactory#load? If so, we should move the discussion to the Java client repository.

@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

no, I am not going to re-use Spring Boot configuration keys :) Reactive RabbitMQ has ConnectionFactory#load method that accepts Properties object.

I suggest to add support for

rabbitmq.ssl.enabled={true|false}
rabbitmq.ssl.validate-server-certificate={true|false}

to that method. that's it

@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

hmmm wait a minute.. so amqp-client-5.7.3 is not Reactive RabbitMQ? I am confused

@acogoluegnes
Copy link
Contributor

OK, but supporting those 2 extra keys must be done in ConnectionFactory#load, not in Reactor RabbitMQ, right?

@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

I've been thinking it's all Reactive RabbitMQ as in that Buzz Lightyear meme 🤦 yes, correct, that should be raised against that product

@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

can you do it? I guess you can put it in a much more meaningful way...

@acogoluegnes
Copy link
Contributor

acogoluegnes commented Apr 15, 2020

I think this is a reasonable request for the Java client, the properties-based configuration already supports the most important settings, except for the TLS ones.

Still, I would support the same keys as in Spring Boot, not only the two you mentioned. For example, what to do when validate-server-certificate=true? A good default is to use the default trust store (this is what Spring AMQP does), but this won't be enough and most people will want to specify their own trust store.

@62mkv
Copy link
Author

62mkv commented Apr 15, 2020

I fully trust you on this. I just needed these two, so basically I don't even know what else they are. but certainly, feature-parity is a good thing.

@acogoluegnes
Copy link
Contributor

OK, let's follow up there: rabbitmq/rabbitmq-java-client#646

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for/stackoverflow Questions are best asked on SO or Gitter
Projects
None yet
Development

No branches or pull requests

2 participants