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/http/fcgi: REMOTE_USER not exposed #16546

Closed
quentinmit opened this issue Jul 30, 2016 · 7 comments
Closed

net/http/fcgi: REMOTE_USER not exposed #16546

quentinmit opened this issue Jul 30, 2016 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@quentinmit
Copy link
Contributor

http.Request is used to represent the incoming request state for net/cgi and net/fcgi. net/fcgi uses http.RequestFromMap to construct the Request to pass to the handler. RequestFromMap does nothing with the REMOTE_USER parameter, which contains the authentication information from the web server. In the net/cgi case, that's fine because the user can just access the environment variable directly, but in net/fcgi, the underlying params map is not exposed to the user.

Either the underlying params should be exposed somehow, or a RemoteUser needs to be added to the Request struct and filled in from params.

/cc @bradfitz

@quentinmit quentinmit added this to the Go1.8Maybe milestone Jul 30, 2016
@bradfitz bradfitz changed the title net/fcgi: REMOTE_USER not exposed net/http/fcgi: REMOTE_USER not exposed Jul 30, 2016
@bradfitz
Copy link
Contributor

For something as specific as this, I'd rather add something to net/http/fcgi than add something to the net/http.Request struct.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@rsc rsc modified the milestones: Unplanned, Go1.8Maybe Oct 20, 2016
@luca76
Copy link

luca76 commented Feb 17, 2017

Any news about this? We use Apache2/FCGI with Kerberos authentication to our Active Directory, and this missing REMOTE_USER is preventing to use Golang in this case.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Unplanned Feb 17, 2017
@bradfitz
Copy link
Contributor

No news. Are you interested in adding this? I'm not sure whether you're already familiar enough with Go to add this support. If so, it's all yours. Otherwise, I'm happy to review a change from anybody.

(As long as the change doesn't expand the http.Request struct.)

@meirf
Copy link
Contributor

meirf commented Mar 24, 2017

The mechanics here don't seem super complicated. Rather the most contentious aspect is probably the API. Here's a first crack at something that would be added to net/http/fcgi:

// Serve accepts incoming FastCGI connections on the listener l, creating a new goroutine for each. 
// The goroutine reads requests and then calls handler to reply to them.
// If l is nil, Serve accepts connections from os.Stdin. If handler is nil, http.DefaultServeMux is used.
func ServeWithCgiVariables(l net.Listener, handler Handler) error // 1.

// Handler responds to incoming requests which contain http data and cgi environment variables.
type Handler interface { // 2
    ServeHttpCgiParams(http.ResponseWriter, *Request) // 3.
}

// Request includes an http request and cgi environment variables that were 
// set at the time of the specific request.
type Request struct {
    HTTPRequest http.Request
    CGIEnvVars map[CGIEnvVariable]string // 4.
}

// CGIEnvVariable is a variable set 
type CGIEnvVariable string
const (
    REMOTE_USER CGI EnvVariable = "REMOTE_USER"
    //... 5.
)

Some of the documentation is redundant, but I include it here to illustrate the intention of code that I've left out.
Some of the documentation is left out, but is discussed below:

  1. This is not a great function name but we can't overload in go.
  2. I'm keeping the same name as the Handler in net package. I thought this was okay to do in go because names are package-prefixed anyway.
  3. I package the http and the params in one Request struct instead of having two separate params because I see them as part of the "input" to the handler so we can keep the spirit of the http.Handler with Serve*(output, input).
  4. Although this issue's focus is just one of the env variables, probably good to be more general.
  5. One of the biggest things this code/documentation leaves out is which variables are being included. Should it be (a) all environment variables or (b) just variables that the code doesn't make a best effort of including in the request? I'm not sure which of these it should be. In either case, the documentation should specify that a best effort is made to include the param values in the http.Request.

(I'd like to work on this issue.)

@luca76
Copy link

luca76 commented Mar 24, 2017

Thank you for your effort. Could you post anywhere a working version of the code, so we can test it?

@meirf
Copy link
Contributor

meirf commented Mar 27, 2017

For additional motivation: an SO post shows that a resolution to this exact problem has been in demand since 2015.

@gopherbot
Copy link

CL https://golang.org/cl/40012 mentions this issue.

lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
The current interface can't access all environment
variables directly or via cgi.RequestFromMap, which
only reads variables on its "white list" to be set on
the http.Request it returns. If an fcgi variable is
not on the "white list" - e.g. REMOTE_USER - the old
code has no access to its value.

This passes variables in the Request context that aren't
used to add data to the Request itself and adds a method
that parses those env vars from the Request's context.

Fixes golang#16546

Change-Id: Ibf933a768b677ece1bb93d7bf99a14cef36ec671
Reviewed-on: https://go-review.googlesource.com/40012
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Apr 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants