Created attachment 30319 [details] Patch to provider bind rmi bind address setup ability There is a patch which adds rmiBindAddress property to JmxRemoteLifecycleListener, which allows to bind RMI server to specific interface instead of 0.0.0.0. It may useful for binding RMI to localhost and avoiding firewall configuration (and then monitoring through SSH tunnel). Unfortunatly using of rmiBindAddress incompatible with rmiSSL, but i think it's not an issue because of rarely intersecting use cases.
I think the code starting at line 217 should probably be an "else" of this if clause: if (rmiSSL) { csf = new SslRMIClientSocketFactory(); ssf = new SslRMIServerSocketFactory(ciphers, protocols, clientAuth); } Otherwise, if rmiSSL and rmiBindAddress are both set, rmiBindAddress will take precedence and a) we'll create and discard objects for no reason and b) potentially open a security vulnerability because the user might think they can have both SSL and a specific address. We might even want to detect the current incompatibility (I also don't see a way to specify an interface when creating an SslRMIServerSocketFactory) and throw an error. Can you adjust your patch?
Created attachment 30320 [details] Patch to provider bind rmi bind address setup ability (updated) Just adjusted patch: added else clause + warning if both options are set
I think conflicting settings should actually fail to configure the connector, rather than playing favorites.
Well. But then which exception should be thrown here? Or there is other preferred way to fail configuring the connector?
Hmm... I thought sure that LifecycleListeners could throw checked exceptions, but it seems they can't. I'm still not happy with this not failing-fast. What happens if you throw an IllegalStateException... will Tomcat fail to start? If it doesn't, we might have to re-think things.
Created attachment 30321 [details] Patch to provider bind rmi bind address setup ability (updated, 2) Yes, with IllegalStateException Tomcat fails to start. Updated patch with it.
The patch needs to include the associated documentation changes. It would also be nice if it used StringManager for i18n.
(In reply to Mark Thomas from comment #7) > The patch needs to include the associated documentation changes. It would > also be nice if it used StringManager for i18n. Alexey, if you'd care to make those documentation patches and include them, it would be great. If not, someone else will do them but the patch will take longer to accept. Let us know if you need any instructions for how to do that: it's not hard, but just in case you needed some encouragement. Look at other classes in the same package for uses of StringManager: it's fairly simple. Basically, anything that is going to end up in a log file, shown to the user, etc. should be localized. Providing an English translation is usually sufficient, though it would be nice to provide Spanish and Japanese if you happen to be multilingual.
Yes, I already found how it's used. I'll make documentation and i18n changes in nearest days.
Created attachment 30398 [details] Patch to provider bind rmi bind address setup ability (updated, 3) Just added i18n and documentation changes.
Are there other areas in which this patch should be improved?
Overall patch looks OK. One questions: Why are SSL and rmiBindAddress mutually exclusive?
It's because I found no way to specify bind address when using SslRMIServerSocketFactory.
I see what you mean. Fair enough.
Thanks for the patch. It has been applied to trunk and 7.0.x and will be included in 7.0.42 onwards.