Bug 55017 - Ability to configure RMI bind address
Summary: Ability to configure RMI bind address
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-24 14:50 UTC by Alexey Noskov
Modified: 2013-07-01 09:53 UTC (History)
1 user (show)



Attachments
Patch to provider bind rmi bind address setup ability (3.85 KB, patch)
2013-05-24 14:50 UTC, Alexey Noskov
Details | Diff
Patch to provider bind rmi bind address setup ability (updated) (4.06 KB, patch)
2013-05-24 19:48 UTC, Alexey Noskov
Details | Diff
Patch to provider bind rmi bind address setup ability (updated, 2) (4.26 KB, patch)
2013-05-24 21:38 UTC, Alexey Noskov
Details | Diff
Patch to provider bind rmi bind address setup ability (updated, 3) (5.93 KB, patch)
2013-06-05 15:45 UTC, Alexey Noskov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Noskov 2013-05-24 14:50:10 UTC
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.
Comment 1 Christopher Schultz 2013-05-24 19:28:03 UTC
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?
Comment 2 Alexey Noskov 2013-05-24 19:48:39 UTC
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
Comment 3 Christopher Schultz 2013-05-24 20:19:09 UTC
I think conflicting settings should actually fail to configure the connector, rather than playing favorites.
Comment 4 Alexey Noskov 2013-05-24 20:24:28 UTC
Well. But then which exception should be thrown here? Or there is other preferred way to fail configuring the connector?
Comment 5 Christopher Schultz 2013-05-24 21:13:00 UTC
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.
Comment 6 Alexey Noskov 2013-05-24 21:38:32 UTC
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.
Comment 7 Mark Thomas 2013-06-03 19:57:08 UTC
The patch needs to include the associated documentation changes. It would also be nice if it used StringManager for i18n.
Comment 8 Christopher Schultz 2013-06-04 19:46:50 UTC
(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.
Comment 9 Alexey Noskov 2013-06-04 19:55:02 UTC
Yes, I already found how it's used.

I'll make documentation and i18n changes in nearest days.
Comment 10 Alexey Noskov 2013-06-05 15:45:11 UTC
Created attachment 30398 [details]
Patch to provider bind rmi bind address setup ability (updated, 3)

Just added i18n and documentation changes.
Comment 11 Alexey Noskov 2013-06-16 19:54:31 UTC
Are there other areas in which this patch should be improved?
Comment 12 Mark Thomas 2013-06-20 15:54:43 UTC
Overall patch looks OK.

One questions: Why are SSL and rmiBindAddress mutually exclusive?
Comment 13 Alexey Noskov 2013-06-20 15:59:53 UTC
It's because I found no way to specify bind address when using SslRMIServerSocketFactory.
Comment 14 Mark Thomas 2013-06-20 19:48:09 UTC
I see what you mean. Fair enough.
Comment 15 Mark Thomas 2013-07-01 09:53:55 UTC
Thanks for the patch.

It has been applied to trunk and 7.0.x and will be included in 7.0.42 onwards.