-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add hook to successful connect response #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question! I'm not sure I've stared at goproxy
long enough to fully understand the flow here
https.go
Outdated
@@ -177,7 +178,18 @@ func (proxy *ProxyHttpServer) handleHttps(w http.ResponseWriter, r *http.Request | |||
|
|||
case ConnectHijack: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between ConnectAccept
and ConnectHijack
? it doesn't seem like ConnectHijack
actually makes a request to the backend so I'm not sure if ConnectRespHandler
can do anything interesting. or are there fields on the *ProxyCtx
set elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you're totally right and it belongs in AcceptConnect! I got myself confused because Hijack()
is also the method you call to override the HTTP protocol on the http client and I didn't realize it was overloaded here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last question around usage!
return nil, nil | ||
} | ||
resp := &http.Response{Status: "200 OK", StatusCode: 200, Proto: "HTTP/1.0", Header: http.Header{}} | ||
err := ctx.proxy.ConnectRespHandler(ctx, resp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want the response to contain any info from the connection to the backend server, do we also need to pass the net.Conn
(targetSiteCon
) as an argument or is this available on the ctx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be available on the context! e.g. to get destination IP it should be possible to get the smokescreen context from the goproxy context UserData like this, and then get decision.resolvedAddr from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooooh nice!
Provides a hook to allow for custom modifications to the 200 CONNECT response, such as adding a header.
I've opted to simply add a function to the proxy config rather than try to fit this functionality into the actions.go OnRequest/OnResponse framework; since CONNECT requests already have special-case fields such as ConnectCopyHandler and ConnectDialContext, this seems reasonable and also avoids additional complexity or modifications to existing logic.