vert-x3 / vertx-rabbitmq-client

Vert.x RabbitMQ Service
Apache License 2.0
73 stars 64 forks source link

Do not write url connection string in logs as password can be displayed #191

Closed ruget closed 1 year ago

ruget commented 1 year ago

Try to avoid to display password in logs.

This is the case when we configure Vertx RabbitMQ Client with

RabbitMQOptions config = new RabbitMQOptions();
config.setUri("amqp://${username}:${password}@${host}/${vhost}");
Yaytay commented 1 year ago

Hmm, I quite specifically do want to know when the connection is being attempted and what it is connecting to. However at the moment there is no way to configure the connection via a URL without putting credentials in there and you are quite right that we should not be logging those. For the new version credentials can be configured independently of the URL, allowing callers to avoid this problem (https://github.com/eclipse-vertx/vertx-rabbitmq-client/blob/main/src/main/java/io/vertx/rabbitmq/impl/RabbitMQConnectionImpl.java).

The current implementation is also inconsistent, unlike the replacement, in that it only logs if the connection uses a URI.

My feeling is that we should replace

        log.info("Connecting to " + uri);
        cf.setUri(uri);

with

        cf.setUri(uri);
        log.info("Connecting to " + cf.getHost());

and also add

      log.info("Connecting to " + addresses);

in the 'else' block.

ruget commented 1 year ago

You're right, connecting logs are always usefull. Code is updated thanks to your feedback.

If this verison is going to be replace by the new one, may be this change is not so usefull.

Yaytay commented 1 year ago

There is a problem with the PR because it "could not validate the users ECA sign-off status". I think this is because you have used two different email addresses for the commits (you can see details of the failure by clicking on the link in the failure box). I have the same problem with my email addresses :( Please can you either fix the commit records or just let me know and I'll make the same changes directly and close this PR.

Yaytay commented 1 year ago

I think the intention is for the new library to be use with vert.x 5, so this one will be supported for a while yet.

ruget commented 1 year ago

I've tried to fix the ECA status. In case it still an issue, it would be kind of you to apply changes on your side.