Skip to content

Conversation

@ldematte
Copy link
Contributor

@ldematte ldematte commented Jan 17, 2025

This PR introduces specific Inbound (listen/accept/receive) and Outbound (connect/send) network entitlements, in place of the current NetworkEntitlement with actions.

Some changes are almost 1-1, with the exception of listen (bind): the SecurityManager "listen" permission is applied to every bind function (sometimes indirectly from a ctor); on server or datagram sockets, this should be thought as an inbound operation; on client sockets, it should not. I changed the checks and policies to reflect that.

Additional changes:

  • removal of the instrumentation of HttpClientBuilderImpl#bind, as discussed during this PR review
  • addition of missing checks in a second implementation of HttpClient#send/sendAsync

Relates to ES-10355

@ldematte ldematte added :Core/Infra/Core Core issues without another label >refactoring auto-backport Automatically create backport pull requests when merged v9.0.0 v8.18.0 test-entitlements labels Jan 17, 2025
@ldematte ldematte requested review from prdoyle and rjernst January 17, 2025 14:44
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 17, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@Override
public void check$java_net_Socket$bind(Class<?> callerClass, Socket that, SocketAddress endpoint) {
policyManager.checkNetworkAccess(callerClass, NetworkEntitlement.LISTEN_ACTION);
policyManager.checkOutboundNetworkAccess(callerClass);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inbound?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooohh nice catch.

Copy link
Contributor Author

@ldematte ldematte Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this is on purpose: as I wrote in the description, I believe that bind on a client socket should be inbound outbound.
True, it was "listen" before, but "listen" was separate, a "3rd option", maybe also for this reason.
On a client socket, bind will make the OS bind :) this side of the connection to a port and interface/address, instead of using any local address/any available port.

Copy link
Contributor Author

@ldematte ldematte Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As opposed to bind on a server socket, which is basically a pre-step for accept, so it is definitely inbound. But correct me if you think this is incorrect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, totally missed your comment in the description 👍

@Override
public void check$sun_nio_ch_SocketChannelImpl$bind(Class<?> callerClass, SocketChannel that, SocketAddress local) {
policyManager.checkNetworkAccess(callerClass, NetworkEntitlement.LISTEN_ACTION);
policyManager.checkOutboundNetworkAccess(callerClass);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inbound?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ldematte ldematte merged commit cd86b3b into elastic:main Jan 20, 2025
21 checks passed
@ldematte ldematte deleted the entitlements/net-entitlements-refactoring branch January 20, 2025 17:29
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 120391

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants