Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: Add ControlContext to Dialer #55301

Closed
pquerna opened this issue Sep 21, 2022 · 11 comments
Closed

net: Add ControlContext to Dialer #55301

pquerna opened this issue Sep 21, 2022 · 11 comments

Comments

@pquerna
Copy link

pquerna commented Sep 21, 2022

The net.Dialer struct contains a the .Control field, a callback. The original intention of this method was to "Permit setting socket options", as noted in the Go 1.11 release notes. One additional use case for this function is a defensive purpose: Preventing some types of Server Side Request Forgery (SSRF) -- as detailed in this blog post:

https://www.agwa.name/blog/post/preventing_server_side_request_forgery_in_golang

In security focused use case, having a variant of the .Control method which received the context.Context would be very helpful:

  1. It would allow propagation of logging contexts, metric context or other metadata down to the callback.
  2. Potentially, the callback could be doing I/O, such as checking with an external service, if a destination is allowed. Today without a context.Context, you don't get cancellation or other context.Values that could be helpful.

Proposed addition to the Dialer struct:

type Dialer struct {
    ControlContext (ctx context.Context, network string, address string, c syscall.RawConn) error
}

Similar to other *Context variants in the standard library, it would only be used if set, and used in preference to the non-context variant.

@gopherbot gopherbot added this to the Proposal milestone Sep 21, 2022
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Sep 25, 2022
@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

/cc @neild

@neild
Copy link
Contributor

neild commented Sep 28, 2022

Using the Control callback for security controls feels like scope creep; Control was intended for injecting socket options, which is why it's called after the socket is created.

That said, I can think of other cases where it might be useful to pass a Context down to the Control func. We've got a package inside Google which passes socket-level network QoS settings via the Context, for example, and I could imagine having a Control func that sets the QoS based on the context value.

I don't have a strong opinion on whether we should add a ControlContext, but it doesn't seem unreasonable.

@rsc rsc moved this from Incoming to Active in Proposals Sep 28, 2022
@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 5, 2022

The filtering sounds maybe questionable, but there are other good reasons to have a ControlContext form.

@rsc rsc moved this from Active to Likely Accept in Proposals Oct 6, 2022
@rsc
Copy link
Contributor

rsc commented Oct 6, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals Oct 12, 2022
@rsc rsc changed the title proposal: net: Add ControlContext to Dialer net: Add ControlContext to Dialer Oct 12, 2022
@rsc rsc modified the milestones: Proposal, Backlog Oct 12, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/444955 mentions this issue: net: Add ControlContext to Dialer

@database64128
Copy link
Contributor

It might be too late to post this, but I think it would be nice if the new control function has netip.AddrPort instead of string as the address type.

@database64128
Copy link
Contributor

database64128 commented Nov 5, 2022

Control is ignored if ControlContext is not nil.

This change breaks forward compatibility for types that wrap a net.Dialer and manipulate the user-specified control function. For example, tfo-go implements TCP Fast Open support on Linux by providing a Dialer type that wraps a net.Dialer and dials by replacing Dialer.Control with a function that does the setup and calls the user-specified control function. If the user upgrades Go and starts using ControlContext, this would be broken.

@ianlancetaylor
Copy link
Contributor

That is true, but it's OK (not ideal, but OK) if a program that uses a new library feature fails to work as expected when using an existing package that is not aware of that new library feature. It should be straightforward for those packages to be updated for the new release.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/451420 mentions this issue: doc/go1.20: add release notes for net package

gopherbot pushed a commit that referenced this issue Nov 18, 2022
For #50101
For #51152
For #53482
For #55301
For #56515

Change-Id: I11edeb4be0a7f80fb72fd7680a3407d081f83b8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/451420
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
komuw added a commit to komuw/ong that referenced this issue Feb 2, 2023
- Use net.Dialer.ControlContext instead of use net.Dialer.Control
  Using Dialer.ControlContext instead of Dialer.Control allows;
   - propagation of logging contexts, metric context or other metadata down to the callback.
   - cancellation if the callback potentially does I/O.
- Fixes: #149
- golang/go#55301
@dmitshur dmitshur removed this from the Backlog milestone Jun 4, 2023
@dmitshur dmitshur added this to the Go1.20 milestone Jun 4, 2023
@rsc rsc removed this from Proposals Nov 10, 2023
@golang golang locked and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants