only allow code to bind to the user's configured port numbers/ranges

[rmuir]

Random code shouldn't be listening on sockets elsewhere.

Today its the wild west, but we only need to grant access to what the user configured.

This means e.g. multicast plugin has to declare its intentions in its security.policy

[bleskes]

The code in NettyTransport does much more with it's settings. See for example NettyTransport.createFallbackSettings and how it's used . I agree what it does is probably an overkill, but can we make the methods there static and use the results setting here (using static strings for settings)?

[rmuir]

that code is not relevant. we don't care about addresses, only port numbers.

only the port number is checked. see http://docs.oracle.com/javase/8/docs/api/java/lang/SecurityManager.html#checkListen-int-

[bleskes]

I was referring to the fact that the setting netty uses are the result of a merge of multiple things:

 // merge fallback settings with default settings with profile settings so we have complete settings with default values
                    Settings mergedSettings = settingsBuilder()
                            .put(fallbackSettings)
                            .put(defaultSettings)
                            .put(profileSettings)
                            .build();

If we can make that static and available we can now for sure they use the same thing, even if people change stuff.

[rmuir]

I don't want our security code entagled in this profile code.

[rmuir]

and for the record, if the parameters to configure which port we bind to are so complex, that we can't write readable logic to explain it, thats a much bigger problem.

I insist on not entangling this with that broken code. I assigned #14553 and will nuke it, despite any complaints to the contrary. its holding back everyone and breaking everything.

but it does not need to hold back improvements on security.

[rmuir]

However, this strikes me as very fragile and subtle and I would want a minimum a test to ensure this behavior is preserved.

That is why instead I would just prefer to keep the security code contained and not "reach out" to other code. Even if its at the expense of a few constants or helper methods. Thats just my opinion.

We know we make this tradeoff by allowing things like paths and ports to be configurable in elasticsearch.yml, the other option is to remove that flexibility, then we could initialize SM on JVM startup?

[bleskes]

hoped for a different answer as the previous code used NettyTransport.DEFAULT_*. Should we move these constants somewhere else then?

[jasontedor]

That is why instead I would just prefer to keep the security code contained and not "reach out" to other code.

@rmuir Completely agree (I think I made a simultaneous edit to my comment to express this while you were writing the comment to which this is a reply).

[rmuir]

hoped for a different answer as the previous code used NettyTransport.DEFAULT_*. Should we move these constants somewhere else then?

These constants are OK, but in general it would be far better to have these constants somewhere else that does not also call methods in a static block. I see that as separate though.

The current constant use will not cause any netty classes to be loaded. You can see this with -verbose:class

[s1monw]

guys take this offline if you want - let move here.

[bleskes]

I think this is great (though I'm not an expert on the security config code). Left one comment ...

[rmuir]

I added a comment. just to be clear here, when binding, code just calls http://docs.oracle.com/javase/8/docs/api/java/lang/SecurityManager.html#checkListen-int- passing the port number. so the "localhost" you see in the permission is not really relevant (the actual address being bound to is not checked by java security, only the port number).

[s1monw]

LGTM please push as is