Issue 76540043: code review 76540043: net/http: add BasicAuth method to *http.Request
Descriptionnet/http: add BasicAuth method to *http.Request
The net/http package supports setting the HTTP Authorization header
using the Basic Authentication Scheme as defined in RFC 2617, but does
not provide support for extracting the username and password from an
authenticated request using the Basic Authentication Scheme.
Add BasicAuth method to *http.Request that returns the username and
password from authenticated requests using the Basic Authentication
Scheme.
Fixes issue 6779.
Patch Set 1 #Patch Set 2 : diff -r cfbe0887d23b https://code.google.com/p/go #Patch Set 3 : diff -r cfbe0887d23b https://code.google.com/p/go #
Total comments: 7
Patch Set 4 : diff -r cfbe0887d23b https://code.google.com/p/go #Patch Set 5 : diff -r cfbe0887d23b https://code.google.com/p/go #
Total comments: 5
Patch Set 6 : diff -r c37fc54f7e208062baa619ecc43197dff1448a2c https://code.google.com/p/go #Patch Set 7 : diff -r c37fc54f7e208062baa619ecc43197dff1448a2c https://code.google.com/p/go #Patch Set 8 : diff -r c37fc54f7e208062baa619ecc43197dff1448a2c https://code.google.com/p/go #
Total comments: 1
Patch Set 9 : diff -r e5c87cefb57fffc5767209542be5c4e58123e3c6 https://code.google.com/p/go #MessagesTotal messages: 29
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
r=bradfitz Not sure whether this is too late for Go 1.3. Took a first pass over it anyway, just in case. All yours now, Brad. https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.go File src/pkg/net/http/request.go (right): https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:479: // BasicAuth returns the request's username and password if this is Maybe something like: // BasicAuth returns the userid and password provided in the request's Authorization header, // if the request uses HTTP Basic Authentication. https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:482: func (r *Request) BasicAuth() (username, password string, ok bool) { The RFC uses "userid" instead of "username" in section 2. Not sure whether that's worth mirroring. Perhaps not; SetBasicAuth uses username. Brad? It might be more useful to return an error ("no Authorization header", "authorization scheme is %v, not Basic", "failed to parse base64: %v", etc.) than just a bool. https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:483: return parseBasicAuth(r.Header.Get("Authorization")) Why the indirection? I assumed that it was for ease of testing, but I don't see parseBasicAuth used in the tests. https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:486: // parseBasicAuth parses a HTTP Basic Authentication string. s/a/an/ https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:498: if len(s2) != 2 { What if the password contains ":"? The RFC only specifies that the username must not contain ":". (Be sure to add a test for this.) https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:501: return s2[0], s2[1], true I find the numbered local variables here impede readability, I think because of the indices. https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request_t... File src/pkg/net/http/request_test.go (right): https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request_t... src/pkg/net/http/request_test.go:381: // Unauthenticated request. A few other negative tests were would be good -- malformed header, other auth schemes, malformed base64, : in password, etc.
Sign in to reply to this message.
I think we should stick to "username" vs "userid" since that is what is used for the SetBasicAuth method. Unless we want to change it there as well.
Sign in to reply to this message.
On 2014/03/16 18:51:15, josharian wrote: > r=bradfitz > > Not sure whether this is too late for Go 1.3. Took a first pass over it anyway, > just in case. All yours now, Brad. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.go > File src/pkg/net/http/request.go (right): > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:479: // BasicAuth returns the request's username and > password if this is > Maybe something like: > > // BasicAuth returns the userid and password provided in the request's > Authorization header, > // if the request uses HTTP Basic Authentication. I'll make the change after we settle on userid vs username. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:482: func (r *Request) BasicAuth() (username, > password string, ok bool) { > The RFC uses "userid" instead of "username" in section 2. Not sure whether > that's worth mirroring. Perhaps not; SetBasicAuth uses username. Brad? I mirrored SetBasicAuth vs the RFC. > > It might be more useful to return an error ("no Authorization header", > "authorization scheme is %v, not Basic", "failed to parse base64: %v", etc.) > than just a bool. I was not sure which route to take for this. I started with an error, but switched to a bool after reviewing the implementation for ParseHTTPVersion. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:483: return > parseBasicAuth(r.Header.Get("Authorization")) > Why the indirection? I assumed that it was for ease of testing, but I don't see > parseBasicAuth used in the tests. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:486: // parseBasicAuth parses a HTTP Basic > Authentication string. > s/a/an/ Yep. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:498: if len(s2) != 2 { > What if the password contains ":"? The RFC only specifies that the username must > not contain ":". (Be sure to add a test for this.) > I'm using strings.SplitN which takes care of that case. The username will include everything before the first ":". Everything else is consider part of the password. I'll add a test as suggested to make this clear. > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:501: return s2[0], s2[1], true > I find the numbered local variables here impede readability, I think because of > the indices. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request_t... > File src/pkg/net/http/request_test.go (right): > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request_t... > src/pkg/net/http/request_test.go:381: // Unauthenticated request. > A few other negative tests were would be good -- malformed header, other auth > schemes, malformed base64, : in password, etc. Good suggestion, I'll add those.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/03/16 23:39:30, hightower wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:josharian@gmail.com, > mailto:bradfitz@golang.org (cc: mailto:golang-codereviews@googlegroups.com), > > Please take another look. This API is wrong, just as wrong as SetBasicAuth - which should be deprecated, since it leads to very unexpected results. net/url.URL already has a *UserInfo field, which sets the credentials used by the http client when fetching a URL and overrides any previously set authentication header from SetBasicAuth. net/http.Request has URL field, which includes the request URL as received by the server but its UserInfo field is currently never set. The right way to handle this is populate that field in incoming requests which include a basic Authorization header. I tried to clean up this mess and submitted this CL https://codereview.appspot.com/8325045/ almost a year ago, with the intention to submit another one implementing the API I described for incoming requests, but it got forgotten/ignored, so I added this functionality to our internal web framework and called it a day. And now that we're at it, someone should also take the time to review this https://codereview.appspot.com/9766046/ and fix that security issue which has been around forever (well, at least since the http client was implemented). Regards, Alberto
Sign in to reply to this message.
On 2014/03/17 13:20:26, Hierro wrote: > On 2014/03/16 23:39:30, hightower wrote: > > Hello mailto:golang-codereviews@googlegroups.com, mailto:josharian@gmail.com, > > mailto:bradfitz@golang.org (cc: mailto:golang-codereviews@googlegroups.com), > > > > Please take another look. > > This API is wrong, just as wrong as SetBasicAuth - which should be deprecated, > since it leads to very unexpected results. net/url.URL already has a *UserInfo > field, which sets the credentials used by the http client when fetching a URL > and overrides any previously set authentication header from SetBasicAuth. > net/http.Request has URL field, which includes the request URL as received by > the server but its UserInfo field is currently never set. The right way to > handle this is populate that field in incoming requests which include a basic > Authorization header. Thanks for the feedback. But I think your comment is more about RFC 2396 vs RFC 2617. I've lightly scanned RFC 2396 and it certainly applies to what you have written. For userinfo passed in the URL I agree that *UserInfo should be populated. However, this CL is about the use of the Authorization header described in RFC 2617, specifically support for the Basic Authentication scheme. The primary goal here is to fix issue 6779, and bring parity to the existing API. Since we support setting the Authorization header using the Basic Authentication scheme, we should also support the inverse. To your final point, I don't think this CL prevents populating *UserInfo based on data found in the Authorization header. > > I tried to clean up this mess and submitted this CL > https://codereview.appspot.com/8325045/ almost a year ago, with the intention to > submit another one implementing the API I described for incoming requests, but > it got forgotten/ignored, so I added this functionality to our internal web > framework and called it a day. And now that we're at it, someone should also > take the time to review this https://codereview.appspot.com/9766046/ and fix > that security issue which has been around forever (well, at least since the http > client was implemented). > > Regards, > Alberto
Sign in to reply to this message.
On 2014/03/17 14:39:16, hightower wrote: > On 2014/03/17 13:20:26, Hierro wrote: > > On 2014/03/16 23:39:30, hightower wrote: > > > Hello mailto:golang-codereviews@googlegroups.com, > mailto:josharian@gmail.com, > > > mailto:bradfitz@golang.org (cc: mailto:golang-codereviews@googlegroups.com), > > > > > > Please take another look. > > > > This API is wrong, just as wrong as SetBasicAuth - which should be deprecated, > > since it leads to very unexpected results. net/url.URL already has a *UserInfo > > field, which sets the credentials used by the http client when fetching a URL > > and overrides any previously set authentication header from SetBasicAuth. > > net/http.Request has URL field, which includes the request URL as received by > > the server but its UserInfo field is currently never set. The right way to > > handle this is populate that field in incoming requests which include a basic > > Authorization header. > > Thanks for the feedback. But I think your comment is more about RFC 2396 vs RFC > 2617. I've lightly scanned RFC 2396 and it certainly applies to what you have > written. For userinfo passed in the URL I agree that *UserInfo should be > populated. > > However, this CL is about the use of the Authorization header described in RFC > 2617, specifically support for the Basic Authentication scheme. The primary goal > here is to fix issue 6779, and bring parity to the existing API. Since we > support setting the Authorization header using the Basic Authentication scheme, > we should also support the inverse. > What I'm saying is that the API you're trying to improve is broken and it should be deprecated. Check this example http://play.golang.org/p/vIi6REz7Ji - SetBasicAuth is ignored in both cases, because the other API takes higher priority. There shouldn't be two ways of doing something which interfere which each other and there's no point in having a SetBasicAuth which is buggy, confusing, incorrectly documented and replaces another one liner: req.SetBasicAuth("foo", "bar") => req.URL.User = url.UserPassword("foo", "bar") // minus the bug and ambiguity As per the Go1 compatibility promise, SetBasicAuth can't be removed nor changed to override req.URL.User, because that could break existing programs. However, it could and should be deprecated. > To your final point, I don't think this CL prevents populating *UserInfo based > on data found in the Authorization header. > It doesn't prevent it, but if the *UserInfo field in the URL is populated there's no point in having a BasicAuth method in Request. There's no good reason to add two methods which provide the same functionality. Regards, Alberto > > > > I tried to clean up this mess and submitted this CL > > https://codereview.appspot.com/8325045/ almost a year ago, with the intention > to > > submit another one implementing the API I described for incoming requests, but > > it got forgotten/ignored, so I added this functionality to our internal web > > framework and called it a day. And now that we're at it, someone should also > > take the time to review this https://codereview.appspot.com/9766046/ and fix > > that security issue which has been around forever (well, at least since the > http > > client was implemented). > > > > Regards, > > Alberto
Sign in to reply to this message.
Sorry, I sent the wrong playground link - this is the correct one http://play.golang.org/p/ZgB95Wn_CM Regards, Alberto
Sign in to reply to this message.
On 2014/03/17 15:10:20, Hierro wrote: > On 2014/03/17 14:39:16, hightower wrote: > > On 2014/03/17 13:20:26, Hierro wrote: > > > On 2014/03/16 23:39:30, hightower wrote: > > > > Hello mailto:golang-codereviews@googlegroups.com, > > mailto:josharian@gmail.com, > > > > mailto:bradfitz@golang.org (cc: > mailto:golang-codereviews@googlegroups.com), > > > > > > > > Please take another look. > > > > > > This API is wrong, just as wrong as SetBasicAuth - which should be > deprecated, > > > since it leads to very unexpected results. net/url.URL already has a > *UserInfo > > > field, which sets the credentials used by the http client when fetching a > URL > > > and overrides any previously set authentication header from SetBasicAuth. > > > net/http.Request has URL field, which includes the request URL as received > by > > > the server but its UserInfo field is currently never set. The right way to > > > handle this is populate that field in incoming requests which include a > basic > > > Authorization header. > > > > Thanks for the feedback. But I think your comment is more about RFC 2396 vs > RFC > > 2617. I've lightly scanned RFC 2396 and it certainly applies to what you have > > written. For userinfo passed in the URL I agree that *UserInfo should be > > populated. > > > > However, this CL is about the use of the Authorization header described in RFC > > 2617, specifically support for the Basic Authentication scheme. The primary > goal > > here is to fix issue 6779, and bring parity to the existing API. Since we > > support setting the Authorization header using the Basic Authentication > scheme, > > we should also support the inverse. > > > > What I'm saying is that the API you're trying to improve is broken and it should > be deprecated. Check this example http://play.golang.org/p/vIi6REz7Ji - > SetBasicAuth is ignored in both cases, because the other API takes higher > priority. There shouldn't be two ways of doing something which interfere which > each other and there's no point in having a SetBasicAuth which is buggy, > confusing, incorrectly documented and replaces another one liner: > > req.SetBasicAuth("foo", "bar") => req.URL.User = url.UserPassword("foo", "bar") > // minus the bug and ambiguity > > As per the Go1 compatibility promise, SetBasicAuth can't be removed nor changed > to override req.URL.User, because that could break existing programs. However, > it could and should be deprecated. > > > To your final point, I don't think this CL prevents populating *UserInfo based > > on data found in the Authorization header. > > > > It doesn't prevent it, but if the *UserInfo field in the URL is populated > there's no point in having a BasicAuth method in Request. There's no good reason > to add two methods which provide the same functionality. > > Regards, > Alberto > > > > > > > I tried to clean up this mess and submitted this CL > > > https://codereview.appspot.com/8325045/ almost a year ago, with the > intention > > to > > > submit another one implementing the API I described for incoming requests, > but > > > it got forgotten/ignored, so I added this functionality to our internal web > > > framework and called it a day. And now that we're at it, someone should also > > > take the time to review this https://codereview.appspot.com/9766046/ and fix > > > that security issue which has been around forever (well, at least since the > > http > > > client was implemented). > > > > > > Regards, > > > Alberto Let me recap to make sure I understand you correctly. * We should attempt to standardize on using the *UserInfo object to represent client auth regardless if credentials are set via URL (RFC 2396) or the Authorization header using the Basic Auth scheme (RFC 2617). * Update the current docs to make point 1 explicit. Questions: * Should the standard library ever support the Digest Auth scheme also outlined in RFC 2617? * If Digest Auth was supported would we extend *UserInfo to make the data available? Maybe we should move this discussion to the dev mailing list?
Sign in to reply to this message.
On 2014/03/16 18:51:15, josharian wrote: > r=bradfitz > > Not sure whether this is too late for Go 1.3. Took a first pass over it anyway, > just in case. All yours now, Brad. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.go > File src/pkg/net/http/request.go (right): > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:479: // BasicAuth returns the request's username and > password if this is > Maybe something like: > > // BasicAuth returns the userid and password provided in the request's > Authorization header, > // if the request uses HTTP Basic Authentication. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:482: func (r *Request) BasicAuth() (username, > password string, ok bool) { > The RFC uses "userid" instead of "username" in section 2. Not sure whether > that's worth mirroring. Perhaps not; SetBasicAuth uses username. Brad? > > It might be more useful to return an error ("no Authorization header", > "authorization scheme is %v, not Basic", "failed to parse base64: %v", etc.) > than just a bool. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:483: return > parseBasicAuth(r.Header.Get("Authorization")) > Why the indirection? I assumed that it was for ease of testing, but I don't see > parseBasicAuth used in the tests. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:486: // parseBasicAuth parses a HTTP Basic > Authentication string. > s/a/an/ > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:498: if len(s2) != 2 { > What if the password contains ":"? The RFC only specifies that the username must > not contain ":". (Be sure to add a test for this.) > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:501: return s2[0], s2[1], true > I find the numbered local variables here impede readability, I think because of > the indices. > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request_t... > File src/pkg/net/http/request_test.go (right): > > https://codereview.appspot.com/76540043/diff/40001/src/pkg/net/http/request_t... > src/pkg/net/http/request_test.go:381: // Unauthenticated request. > A few other negative tests were would be good -- malformed header, other auth > schemes, malformed base64, : in password, etc. Thanks for the detailed review. I've made all the suggested changes except the username vs userid.
Sign in to reply to this message.
On 2014/03/17 15:45:38, hightower wrote: > On 2014/03/17 15:10:20, Hierro wrote: > > On 2014/03/17 14:39:16, hightower wrote: > > > On 2014/03/17 13:20:26, Hierro wrote: > > > > On 2014/03/16 23:39:30, hightower wrote: > > > > > Hello mailto:golang-codereviews@googlegroups.com, > > > mailto:josharian@gmail.com, > > > > > mailto:bradfitz@golang.org (cc: > > mailto:golang-codereviews@googlegroups.com), > > > > > > > > > > Please take another look. > > > > > > > > This API is wrong, just as wrong as SetBasicAuth - which should be > > deprecated, > > > > since it leads to very unexpected results. net/url.URL already has a > > *UserInfo > > > > field, which sets the credentials used by the http client when fetching a > > URL > > > > and overrides any previously set authentication header from SetBasicAuth. > > > > net/http.Request has URL field, which includes the request URL as received > > by > > > > the server but its UserInfo field is currently never set. The right way to > > > > handle this is populate that field in incoming requests which include a > > basic > > > > Authorization header. > > > > > > Thanks for the feedback. But I think your comment is more about RFC 2396 vs > > RFC > > > 2617. I've lightly scanned RFC 2396 and it certainly applies to what you > have > > > written. For userinfo passed in the URL I agree that *UserInfo should be > > > populated. > > > > > > However, this CL is about the use of the Authorization header described in > RFC > > > 2617, specifically support for the Basic Authentication scheme. The primary > > goal > > > here is to fix issue 6779, and bring parity to the existing API. Since we > > > support setting the Authorization header using the Basic Authentication > > scheme, > > > we should also support the inverse. > > > > > > > What I'm saying is that the API you're trying to improve is broken and it > should > > be deprecated. Check this example http://play.golang.org/p/vIi6REz7Ji - > > SetBasicAuth is ignored in both cases, because the other API takes higher > > priority. There shouldn't be two ways of doing something which interfere which > > each other and there's no point in having a SetBasicAuth which is buggy, > > confusing, incorrectly documented and replaces another one liner: > > > > req.SetBasicAuth("foo", "bar") => req.URL.User = url.UserPassword("foo", > "bar") > > // minus the bug and ambiguity > > > > As per the Go1 compatibility promise, SetBasicAuth can't be removed nor > changed > > to override req.URL.User, because that could break existing programs. However, > > it could and should be deprecated. > > > > > To your final point, I don't think this CL prevents populating *UserInfo > based > > > on data found in the Authorization header. > > > > > > > It doesn't prevent it, but if the *UserInfo field in the URL is populated > > there's no point in having a BasicAuth method in Request. There's no good > reason > > to add two methods which provide the same functionality. > > > > Regards, > > Alberto > > > > > > > > > > I tried to clean up this mess and submitted this CL > > > > https://codereview.appspot.com/8325045/ almost a year ago, with the > > intention > > > to > > > > submit another one implementing the API I described for incoming requests, > > but > > > > it got forgotten/ignored, so I added this functionality to our internal > web > > > > framework and called it a day. And now that we're at it, someone should > also > > > > take the time to review this https://codereview.appspot.com/9766046/ and > fix > > > > that security issue which has been around forever (well, at least since > the > > > http > > > > client was implemented). > > > > > > > > Regards, > > > > Alberto > > Let me recap to make sure I understand you correctly. > > * We should attempt to standardize on using the *UserInfo object to represent > client auth regardless if credentials are set via URL (RFC 2396) or the > Authorization header using the Basic Auth scheme (RFC 2617). From the server's PoV, there's no difference - the bytes on the wire are the same. Allowing credentials in the URL is only a shorthand for specifying a location and the Authentication info in just one string. Just to be clear, I completely agree with you in these 2 points: - A server side API for reading HTTP basic authentication should be provided by the std lib. - The server side API should look like the client side API, to make it easier to understand. Unfortunately, there are two conflicting client APIs in the std lib (SetBasicAuth and req.URL.User). Setting the User field in the URL (either via the initial URL string or changing req.URL.User) overrides any Authorization headers set by other means, so we should consider it the "main" API. That can't be changed without breaking already existing programs. So, in order to provide a server side API without increasing the current mess, the best solution is: - Make the User field in URL the preferred API. Make this clear in the documentation by deprecating SetBasicAuth and clarifying that the User field in net/url.URL overrides any Authentication headers set by other means. - Build the server side API as closely as possible as the client API - id set, set the User field in the incoming Request's URL. > * Update the current docs to make point 1 explicit. Exactly, the current situation is confusing an error prone. There are two ways to do the same thing, but one of them is buggy and saves nothing, since the calling code will be one line anyway. > > Questions: > > * Should the standard library ever support the Digest Auth scheme also outlined > in RFC 2617? I don't think it should. Digest is rarely used because, in practice, it's not more secure than Basic, while adding extra complexity. > * If Digest Auth was supported would we extend *UserInfo to make the data > available? No, since that would be implemented in a 3rd party package. > > Maybe we should move this discussion to the dev mailing list? That sounds like a good idea. Regards, Alberto
Sign in to reply to this message.
The length of this thread (which I haven't read yet), suggests that this is too late for Go 1.3. Somebody should do this for Go 1.4, when the tree re-opens in 3 months. On Mon, Mar 17, 2014 at 10:37 AM, <alberto.garcia.hierro@gmail.com> wrote: > On 2014/03/17 15:45:38, hightower wrote: > >> On 2014/03/17 15:10:20, Hierro wrote: >> > On 2014/03/17 14:39:16, hightower wrote: >> > > On 2014/03/17 13:20:26, Hierro wrote: >> > > > On 2014/03/16 23:39:30, hightower wrote: >> > > > > Hello mailto:golang-codereviews@googlegroups.com, >> > > mailto:josharian@gmail.com, >> > > > > mailto:bradfitz@golang.org (cc: >> > mailto:golang-codereviews@googlegroups.com), >> > > > > >> > > > > Please take another look. >> > > > >> > > > This API is wrong, just as wrong as SetBasicAuth - which should >> > be > >> > deprecated, >> > > > since it leads to very unexpected results. net/url.URL already >> > has a > >> > *UserInfo >> > > > field, which sets the credentials used by the http client when >> > fetching a > >> > URL >> > > > and overrides any previously set authentication header from >> > SetBasicAuth. > >> > > > net/http.Request has URL field, which includes the request URL >> > as received > >> > by >> > > > the server but its UserInfo field is currently never set. The >> > right way to > >> > > > handle this is populate that field in incoming requests which >> > include a > >> > basic >> > > > Authorization header. >> > > >> > > Thanks for the feedback. But I think your comment is more about >> > RFC 2396 vs > >> > RFC >> > > 2617. I've lightly scanned RFC 2396 and it certainly applies to >> > what you > >> have >> > > written. For userinfo passed in the URL I agree that *UserInfo >> > should be > >> > > populated. >> > > >> > > However, this CL is about the use of the Authorization header >> > described in > >> RFC >> > > 2617, specifically support for the Basic Authentication scheme. >> > The primary > >> > goal >> > > here is to fix issue 6779, and bring parity to the existing API. >> > Since we > >> > > support setting the Authorization header using the Basic >> > Authentication > >> > scheme, >> > > we should also support the inverse. >> > > >> > >> > What I'm saying is that the API you're trying to improve is broken >> > and it > >> should >> > be deprecated. Check this example >> > http://play.golang.org/p/vIi6REz7Ji - > >> > SetBasicAuth is ignored in both cases, because the other API takes >> > higher > >> > priority. There shouldn't be two ways of doing something which >> > interfere which > >> > each other and there's no point in having a SetBasicAuth which is >> > buggy, > >> > confusing, incorrectly documented and replaces another one liner: >> > >> > req.SetBasicAuth("foo", "bar") => req.URL.User = >> > url.UserPassword("foo", > >> "bar") >> > // minus the bug and ambiguity >> > >> > As per the Go1 compatibility promise, SetBasicAuth can't be removed >> > nor > >> changed >> > to override req.URL.User, because that could break existing >> > programs. However, > >> > it could and should be deprecated. >> > >> > > To your final point, I don't think this CL prevents populating >> > *UserInfo > >> based >> > > on data found in the Authorization header. >> > > >> > >> > It doesn't prevent it, but if the *UserInfo field in the URL is >> > populated > >> > there's no point in having a BasicAuth method in Request. There's no >> > good > >> reason >> > to add two methods which provide the same functionality. >> > >> > Regards, >> > Alberto >> > >> > > > >> > > > I tried to clean up this mess and submitted this CL >> > > > https://codereview.appspot.com/8325045/ almost a year ago, with >> > the > >> > intention >> > > to >> > > > submit another one implementing the API I described for incoming >> > requests, > >> > but >> > > > it got forgotten/ignored, so I added this functionality to our >> > internal > >> web >> > > > framework and called it a day. And now that we're at it, someone >> > should > >> also >> > > > take the time to review this >> > https://codereview.appspot.com/9766046/ and > >> fix >> > > > that security issue which has been around forever (well, at >> > least since > >> the >> > > http >> > > > client was implemented). >> > > > >> > > > Regards, >> > > > Alberto >> > > Let me recap to make sure I understand you correctly. >> > > * We should attempt to standardize on using the *UserInfo object to >> > represent > >> client auth regardless if credentials are set via URL (RFC 2396) or >> > the > >> Authorization header using the Basic Auth scheme (RFC 2617). >> > > From the server's PoV, there's no difference - the bytes on the wire are > the same. Allowing credentials in the URL is only a shorthand for > specifying a location and the Authentication info in just one string. > > Just to be clear, I completely agree with you in these 2 points: > > - A server side API for reading HTTP basic authentication should be > provided by the std lib. > - The server side API should look like the client side API, to make it > easier to understand. > > Unfortunately, there are two conflicting client APIs in the std lib > (SetBasicAuth and req.URL.User). Setting the User field in the URL > (either via the initial URL string or changing req.URL.User) overrides > any Authorization headers set by other means, so we should consider it > the "main" API. That can't be changed without breaking already existing > programs. So, in order to provide a server side API without increasing > the current mess, the best solution is: > > - Make the User field in URL the preferred API. Make this clear in the > documentation by deprecating SetBasicAuth and clarifying that the User > field in net/url.URL overrides any Authentication headers set by other > means. > - Build the server side API as closely as possible as the client API - > id set, set the User field in the incoming Request's URL. > > > * Update the current docs to make point 1 explicit. >> > > Exactly, the current situation is confusing an error prone. There are > two ways to do the same thing, but one of them is buggy and saves > nothing, since the calling code will be one line anyway. > > > > Questions: >> > > * Should the standard library ever support the Digest Auth scheme also >> > outlined > >> in RFC 2617? >> > > I don't think it should. Digest is rarely used because, in practice, > it's not more secure than Basic, while adding extra complexity. > > > * If Digest Auth was supported would we extend *UserInfo to make the >> > data > >> available? >> > > No, since that would be implemented in a 3rd party package. > > > > Maybe we should move this discussion to the dev mailing list? >> > > That sounds like a good idea. > > Regards, > Alberto > > https://codereview.appspot.com/76540043/ >
Sign in to reply to this message.
On 2014/03/17 18:18:02, bradfitz wrote: > The length of this thread (which I haven't read yet), suggests that this is > too late for Go 1.3. I think this is a bit unfortunate, because most of this thread is not about the code under review. I chose to work on this issue because the related ticket was accepted which lead me to believe that the approach taken by this patch is the right one. But I do understand that this CL is really close or past the deadline for Go 1.3. > > Somebody should do this for Go 1.4, when the tree re-opens in 3 months. > > > > On Mon, Mar 17, 2014 at 10:37 AM, <mailto:alberto.garcia.hierro@gmail.com> wrote: > > > On 2014/03/17 15:45:38, hightower wrote: > > > >> On 2014/03/17 15:10:20, Hierro wrote: > >> > On 2014/03/17 14:39:16, hightower wrote: > >> > > On 2014/03/17 13:20:26, Hierro wrote: > >> > > > On 2014/03/16 23:39:30, hightower wrote: > >> > > > > Hello mailto:golang-codereviews@googlegroups.com, > >> > > mailto:josharian@gmail.com, > >> > > > > mailto:bradfitz@golang.org (cc: > >> > mailto:golang-codereviews@googlegroups.com), > >> > > > > > >> > > > > Please take another look. > >> > > > > >> > > > This API is wrong, just as wrong as SetBasicAuth - which should > >> > > be > > > >> > deprecated, > >> > > > since it leads to very unexpected results. net/url.URL already > >> > > has a > > > >> > *UserInfo > >> > > > field, which sets the credentials used by the http client when > >> > > fetching a > > > >> > URL > >> > > > and overrides any previously set authentication header from > >> > > SetBasicAuth. > > > >> > > > net/http.Request has URL field, which includes the request URL > >> > > as received > > > >> > by > >> > > > the server but its UserInfo field is currently never set. The > >> > > right way to > > > >> > > > handle this is populate that field in incoming requests which > >> > > include a > > > >> > basic > >> > > > Authorization header. > >> > > > >> > > Thanks for the feedback. But I think your comment is more about > >> > > RFC 2396 vs > > > >> > RFC > >> > > 2617. I've lightly scanned RFC 2396 and it certainly applies to > >> > > what you > > > >> have > >> > > written. For userinfo passed in the URL I agree that *UserInfo > >> > > should be > > > >> > > populated. > >> > > > >> > > However, this CL is about the use of the Authorization header > >> > > described in > > > >> RFC > >> > > 2617, specifically support for the Basic Authentication scheme. > >> > > The primary > > > >> > goal > >> > > here is to fix issue 6779, and bring parity to the existing API. > >> > > Since we > > > >> > > support setting the Authorization header using the Basic > >> > > Authentication > > > >> > scheme, > >> > > we should also support the inverse. > >> > > > >> > > >> > What I'm saying is that the API you're trying to improve is broken > >> > > and it > > > >> should > >> > be deprecated. Check this example > >> > > http://play.golang.org/p/vIi6REz7Ji - > > > >> > SetBasicAuth is ignored in both cases, because the other API takes > >> > > higher > > > >> > priority. There shouldn't be two ways of doing something which > >> > > interfere which > > > >> > each other and there's no point in having a SetBasicAuth which is > >> > > buggy, > > > >> > confusing, incorrectly documented and replaces another one liner: > >> > > >> > req.SetBasicAuth("foo", "bar") => req.URL.User = > >> > > url.UserPassword("foo", > > > >> "bar") > >> > // minus the bug and ambiguity > >> > > >> > As per the Go1 compatibility promise, SetBasicAuth can't be removed > >> > > nor > > > >> changed > >> > to override req.URL.User, because that could break existing > >> > > programs. However, > > > >> > it could and should be deprecated. > >> > > >> > > To your final point, I don't think this CL prevents populating > >> > > *UserInfo > > > >> based > >> > > on data found in the Authorization header. > >> > > > >> > > >> > It doesn't prevent it, but if the *UserInfo field in the URL is > >> > > populated > > > >> > there's no point in having a BasicAuth method in Request. There's no > >> > > good > > > >> reason > >> > to add two methods which provide the same functionality. > >> > > >> > Regards, > >> > Alberto > >> > > >> > > > > >> > > > I tried to clean up this mess and submitted this CL > >> > > > https://codereview.appspot.com/8325045/ almost a year ago, with > >> > > the > > > >> > intention > >> > > to > >> > > > submit another one implementing the API I described for incoming > >> > > requests, > > > >> > but > >> > > > it got forgotten/ignored, so I added this functionality to our > >> > > internal > > > >> web > >> > > > framework and called it a day. And now that we're at it, someone > >> > > should > > > >> also > >> > > > take the time to review this > >> > > https://codereview.appspot.com/9766046/ and > > > >> fix > >> > > > that security issue which has been around forever (well, at > >> > > least since > > > >> the > >> > > http > >> > > > client was implemented). > >> > > > > >> > > > Regards, > >> > > > Alberto > >> > > > > Let me recap to make sure I understand you correctly. > >> > > > > * We should attempt to standardize on using the *UserInfo object to > >> > > represent > > > >> client auth regardless if credentials are set via URL (RFC 2396) or > >> > > the > > > >> Authorization header using the Basic Auth scheme (RFC 2617). > >> > > > > From the server's PoV, there's no difference - the bytes on the wire are > > the same. Allowing credentials in the URL is only a shorthand for > > specifying a location and the Authentication info in just one string. > > > > Just to be clear, I completely agree with you in these 2 points: > > > > - A server side API for reading HTTP basic authentication should be > > provided by the std lib. > > - The server side API should look like the client side API, to make it > > easier to understand. > > > > Unfortunately, there are two conflicting client APIs in the std lib > > (SetBasicAuth and req.URL.User). Setting the User field in the URL > > (either via the initial URL string or changing req.URL.User) overrides > > any Authorization headers set by other means, so we should consider it > > the "main" API. That can't be changed without breaking already existing > > programs. So, in order to provide a server side API without increasing > > the current mess, the best solution is: > > > > - Make the User field in URL the preferred API. Make this clear in the > > documentation by deprecating SetBasicAuth and clarifying that the User > > field in net/url.URL overrides any Authentication headers set by other > > means. > > - Build the server side API as closely as possible as the client API - > > id set, set the User field in the incoming Request's URL. > > > > > > * Update the current docs to make point 1 explicit. > >> > > > > Exactly, the current situation is confusing an error prone. There are > > two ways to do the same thing, but one of them is buggy and saves > > nothing, since the calling code will be one line anyway. > > > > > > > > Questions: > >> > > > > * Should the standard library ever support the Digest Auth scheme also > >> > > outlined > > > >> in RFC 2617? > >> > > > > I don't think it should. Digest is rarely used because, in practice, > > it's not more secure than Basic, while adding extra complexity. > > > > > > * If Digest Auth was supported would we extend *UserInfo to make the > >> > > data > > > >> available? > >> > > > > No, since that would be implemented in a 3rd party package. > > > > > > > > Maybe we should move this discussion to the dev mailing list? > >> > > > > That sounds like a good idea. > > > > Regards, > > Alberto > > > > https://codereview.appspot.com/76540043/ > >
Sign in to reply to this message.
On Mon, Mar 17, 2014 at 11:35 AM, <kelsey.hightower@gmail.com> wrote: > On 2014/03/17 18:18:02, bradfitz wrote: > >> The length of this thread (which I haven't read yet), suggests that >> > this is > >> too late for Go 1.3. >> > > I think this is a bit unfortunate, because most of this thread is not > about the code under review. I chose to work on this issue because the > related ticket was accepted which lead me to believe that the approach > taken by this patch is the right one. But I do understand that this CL > is really close or past the deadline for Go 1.3. But the accepted issue isn't marked for Go1.3, so it's not in the list of things we're supposed to be spending time on now. We're fixing bugs now and stabilizing things, not adding new code which might contain new bugs. We have to draw the line somewhere, and this is code that can live in external repos for now. It's not a bug in something you can't work around otherwise.
Sign in to reply to this message.
Totally agree. Thanks Brad. Sent from my iPhone > On Mar 17, 2014, at 11:39 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > > > > >> On Mon, Mar 17, 2014 at 11:35 AM, <kelsey.hightower@gmail.com> wrote: >>> On 2014/03/17 18:18:02, bradfitz wrote: >>> The length of this thread (which I haven't read yet), suggests that >> this is >>> too late for Go 1.3. >> >> I think this is a bit unfortunate, because most of this thread is not >> about the code under review. I chose to work on this issue because the >> related ticket was accepted which lead me to believe that the approach >> taken by this patch is the right one. But I do understand that this CL >> is really close or past the deadline for Go 1.3. > > But the accepted issue isn't marked for Go1.3, so it's not in the list of things we're supposed to be spending time on now. We're fixing bugs now and stabilizing things, not adding new code which might contain new bugs. > > We have to draw the line somewhere, and this is code that can live in external repos for now. It's not a bug in something you can't work around otherwise. >
Sign in to reply to this message.
R=close Please ping this thread again after Go 1.3.
Sign in to reply to this message.
On 2014/04/11 20:55:30, bradfitz wrote: > R=close > > Please ping this thread again after Go 1.3. Just pinging this thread on behalf of Kelsey since it's after Go 1.3 and I'd love to see this in time for 1.4 :)
Sign in to reply to this message.
On 2014/08/12 21:52:58, bgentry wrote: > On 2014/04/11 20:55:30, bradfitz wrote: > > R=close > > > > Please ping this thread again after Go 1.3. > > Just pinging this thread on behalf of Kelsey since it's after Go 1.3 and I'd > love to see this in time for 1.4 :) Ping.
Sign in to reply to this message.
Sorry for the review delay. FYI: the tree closes in 3 days, so you don't have much time. https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.go File src/pkg/net/http/request.go (right): https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:481: // (See RFC 2617, Section 2) Drop the parens and add a period at the end. https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:482: func (r *Request) BasicAuth() (username, password string, err error) { this opens questions as to what the errors might be. I don't think any are interesting. How about "ok bool" instead? https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:492: func ParseBasicAuth(auth string) (username, password string, err error) { Let's unexport this. You can keep it returning an error if you want that for testing. Your call. https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:493: s1 := strings.SplitN(auth, " ", 2) both these SplitNs are unnecessary and allocate. You can just do strings.HasPrefix(.. "Basic ") here and then slice it off. https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... src/pkg/net/http/request.go:504: s2 := strings.SplitN(string(c), ":", 2) you could just IndexByte the ':' and return slices and avoid making the slice in SplitN.
Sign in to reply to this message.
On 2014/08/28 15:36:28, bradfitz wrote: > Sorry for the review delay. FYI: the tree closes in 3 days, so you don't have > much time. > > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.go > File src/pkg/net/http/request.go (right): > > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:481: // (See RFC 2617, Section 2) > Drop the parens and add a period at the end. > > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:482: func (r *Request) BasicAuth() (username, > password string, err error) { > this opens questions as to what the errors might be. I don't think any are > interesting. How about "ok bool" instead? > > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:492: func ParseBasicAuth(auth string) (username, > password string, err error) { > Let's unexport this. > > You can keep it returning an error if you want that for testing. Your call. > > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:493: s1 := strings.SplitN(auth, " ", 2) > both these SplitNs are unnecessary and allocate. You can just do > strings.HasPrefix(.. "Basic ") here and then slice it off. > > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... > src/pkg/net/http/request.go:504: s2 := strings.SplitN(string(c), ":", 2) > you could just IndexByte the ':' and return slices and avoid making the slice in > SplitN. Thanks for the review, I'll be making these changes today.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, bradfitz@golang.org, alberto.garcia.hierro@gmail.com, blakesgentry@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
hightower - I think you missed one of Brad's requests from a previous version to unexport one of the funcs you added. https://codereview.appspot.com/76540043/diff/140001/src/pkg/net/http/request.go File src/pkg/net/http/request.go (right): https://codereview.appspot.com/76540043/diff/140001/src/pkg/net/http/request.... src/pkg/net/http/request.go:538: func ParseBasicAuth(auth string) (username, password string, ok bool) { I think you missed the request from Brad to unexport the func `ParseBasicAuth()`: https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g...
Sign in to reply to this message.
On 2014/08/29 20:44:33, bgentry wrote: > hightower - I think you missed one of Brad's requests from a previous version to > unexport one of the funcs you added. > > https://codereview.appspot.com/76540043/diff/140001/src/pkg/net/http/request.go > File src/pkg/net/http/request.go (right): > > https://codereview.appspot.com/76540043/diff/140001/src/pkg/net/http/request.... > src/pkg/net/http/request.go:538: func ParseBasicAuth(auth string) (username, > password string, ok bool) { > I think you missed the request from Brad to unexport the func > `ParseBasicAuth()`: > https://codereview.appspot.com/76540043/diff/80001/src/pkg/net/http/request.g... Good catch.
Sign in to reply to this message.
You going to fix it? On Fri, Aug 29, 2014 at 1:45 PM, <kelsey.hightower@gmail.com> wrote: > On 2014/08/29 20:44:33, bgentry wrote: > >> hightower - I think you missed one of Brad's requests from a previous >> > version to > >> unexport one of the funcs you added. >> > > > https://codereview.appspot.com/76540043/diff/140001/src/ > pkg/net/http/request.go > >> File src/pkg/net/http/request.go (right): >> > > > https://codereview.appspot.com/76540043/diff/140001/src/ > pkg/net/http/request.go#newcode538 > >> src/pkg/net/http/request.go:538: func ParseBasicAuth(auth string) >> > (username, > >> password string, ok bool) { >> I think you missed the request from Brad to unexport the func >> `ParseBasicAuth()`: >> > > https://codereview.appspot.com/76540043/diff/80001/src/ > pkg/net/http/request.go#newcode492 > > Good catch. > > https://codereview.appspot.com/76540043/ >
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, bradfitz@golang.org, alberto.garcia.hierro@gmail.com, blakesgentry@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks. On Fri, Aug 29, 2014 at 8:04 PM, <kelsey.hightower@gmail.com> wrote: > Hello golang-codereviews@googlegroups.com, josharian@gmail.com, > bradfitz@golang.org, alberto.garcia.hierro@gmail.com, > blakesgentry@gmail.com (cc: golang-codereviews@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.com/76540043/ >
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=5e03333d2dcf *** net/http: add BasicAuth method to *http.Request The net/http package supports setting the HTTP Authorization header using the Basic Authentication Scheme as defined in RFC 2617, but does not provide support for extracting the username and password from an authenticated request using the Basic Authentication Scheme. Add BasicAuth method to *http.Request that returns the username and password from authenticated requests using the Basic Authentication Scheme. Fixes issue 6779. LGTM=bradfitz R=golang-codereviews, josharian, bradfitz, alberto.garcia.hierro, blakesgentry CC=golang-codereviews https://codereview.appspot.com/76540043 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
|
