Tangentially, is Chef aware that their HTTP API is not standards-compliant? If not, I'd suggest filing an issue with them too.

@mdempsky 11.x is the last major release, so I don't suspect they will be upgrading the authentication mechanism. The latest version of the Chef server (12.x) has removed this header.

There is also a fork of the Chef Server written in Go that is currently only compliant with the 11.x API, so it only supports this authentication mechanism:

• https://github.com/ctdk/goiardi

So while I am personally only working with the API, that project will no longer function on Go 1.6+.

I really don't want to accept spaces, and I really don't want to add new API surface to whitelist broken things.

http2 is explicitly against this too. Would Chef 11.x ever use http2 in any sort of deployment?

What if we just explicitly whitelist Hashed Path? That's gross, but so is everything else.

Alternatively we do nothing and say if you don't want to upgrade from Chef 11 to Chef 12, don't upgrade from Go 1.5 to Go 1.6 either and keep your old software stack old. (kinda joking, but kinda not)

I really don't want to accept spaces, and I really don't want to add new API surface to whitelist broken things.

I agree. I guess the question I was posing here is: are we OK in saying we do not wish to support older specifications that do not implement this HTTP restriction? I'm thinking about cases where these specs were written when this wasn't explicitly forbidden. My example of the Chef server being one of them.

http2 is explicitly against this too. Would Chef 11.x ever use http2 in any sort of deployment?

Unfortunately, I can't speak on behalf of the Chef community. But to spitball, I don't believe they would, no.

What if we just explicitly whitelist Hashed Path? That's gross, but so is everything else.

We could whitelist it, I suppose. It feels absolutely gross, and I'd probably shy away from it. I would not want to litter the stdlib with things like that. If we did decide to go the whitelist route, that still leaves Go in a state that may be broken for other projects. If we're going to whitelist one non-compliant header, we should give the ability for others to do the same. But then there goes more in to the net/http package.

Alternatively we do nothing and say if you don't want to upgrade from Chef 11 to Chef 12, don't upgrade from Go 1.5 to Go 1.6 either and keep your old software stack old. (kinda joking, but kinda not)

Maybe. We use the Goiardi project listed above, and it works well for us in specific cases where we don't want to stand up a Chef server with all of its dependencies. We do want to upgrade to Chef 12, there are just a lot of yaks in the way. This may be bigger than just the Chef project, but it just so happened to be the case where I ran in to it.

are we OK in saying we do not wish to support older specifications that do not implement this HTTP restriction?

It's been there since at least 1999, if not earlier: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

That's not really new.

If we're going to whitelist one non-compliant header, we should give the ability for others to do the same.

You're talking about API surface again, though. I don't think this problem is actually very widespread. We'll see, though.

In RFC 2616 from 1999:

       message-header = field-name ":" [ field-value ]
       field-name     = token
...
       token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT

Note that SP (space) was always forbidden.

Yeah, seen. I was reading that specific section of the RFC, and didn't have the token definition in front of me. Using the full-form document made it clear. Unfortunately too clear.

Well then, I guess it would appear that I'm SOL. 😄

Well then, I guess it would appear that I'm SOL.

I'm still fine with putting a whitelist of shame with a single entry in the code.

I've been thinking about this since your initial response, and I still am in agreement that requests with headers like this should be rejected. But I wonder if it was necessary to make this change in a way that cannot be managed by the person consuming net/http.

In the document, Go 1 and the Future of Go Programs, there is the following statement made in the introduction:

It is intended that programs written to the Go 1 specification will continue to compile and run correctly, unchanged, over the lifetime of that specification. At some indefinite point, a Go 2 specification may arise, but until that time, Go programs that work today should continue to work even as future "point" releases of Go 1 arise (Go 1.1, Go 1.2, etc.).

Of course, it is intended that they do not change. In no way is it stating this is a binding statement. That's also confirmed based on a later section:

Although we expect that the vast majority of programs will maintain this compatibility over time, it is impossible to guarantee that no future change will break any program. This document is an attempt to set expectations for the compatibility of Go 1 software in the future. There are a number of ways in which a program that compiles and runs today may fail to do so after a future point release. They are all unlikely but worth recording.

...

• Specification errors. If it becomes necessary to address an inconsistency or incompleteness in the specification, resolving the issue could affect the meaning or legality of existing programs. We reserve the right to address such issues, including updating the implementations. Except for security issues, no incompatible changes to the specification would be made.

I think it was necessary to update net/http to reject requests with invalid headers. I'm not sure it was necessary to do so in a way that cannot be controlled by the consumer. I think it's possible to enhance this implementation in a way that achieves the goal of meeting spec, while having a clean workaround to achieve backwards-compatible behavior. Right now there isn't one.

Any favorite whiskies I could bribe you with to allow one small API addition to give consumers control of the whitelist? 😄

Honestly, the more I think about having the one item added to a whitelist the more gross it feels. I'd rather not go down that path. Is there a way it could be implemented that feels natural and doesn't add much weight? I'd be happy to offer my time to implement it and go through the CR process.

Are you sure this is the problem. We had a long chat about this on slack and it appears that the Hashed Path: header is only required inside the contents of the body (not the http body) to be hashed. It never appears on the wire according to the documentation at

https://docs.chef.io/release/oec_11-2/api_chef_server.html
https://docs.chef.io/api_chef_server.html

I might be reading this wrong. If so, please point me to the correct documentation for the version of the server you have to talk to.

This is how I interpret it as well. v11.0 of the Chef Server docs: https://docs.chef.io/release/oec_11-0/auth.html#header-format (similar to what Dave has linked).

• You generate a string (signature) in the format of base64(sha1(header-format) where header-format is the below:

Method:HTTP_METHOD
Hashed Path:HASHED_PATH
X-Ops-Content-Hash:HASHED_BODY
X-Ops-Timestamp:TIME
X-Ops-UserId:USERID

• You slice that up into N 60-character long values and add a X-Ops-Authorization-N header as appropriate.
• Note that no Hashed Path HTTP header is shown in the example: https://docs.chef.io/release/oec_11-0/auth.html#example

Thanks, @davecheney and @elithrar. Yeah, reading those docs, it sounds like whatever problem @theckman is having is not about spaces.

What do you actually seeing happening on the wire?

Yeah. I found my issue late last night I believe. I plan on looking at the workaround today. I just didn't get a chance to comment here.

It is very possible there are now zero examples of this issue, so we may end up closing it. Let me look right now.

Confirmed. This ended up being a bug in the go-chef project. The Chef server didn't end up needing the header itself, as was confirmed above.

The implementation appears to be accidentally including the header with the request. Normally, there isn't any harm in doing so as I'm not sure of any other servers that outright reject requests if there is a space in the header.

So we're down to zero cases. I think that may be the sign that this issue isn't important. The Chef issue was just my example. I still think that this failure should not be abstracted away from consumers, as they should have the final say on the request (as well as the error page that gets returned by the server). This just stems from two desires:

• I'd like to be able to provide custom error pages, when possible, that is in a format expected by the consumer (e.g., people consuming JSON that check for an "error" key in the response). In its current implementation they would get text/plain I believe.
• I still don't like the idea of the HTTP client, that all other clients will seemingly be built on, enforcing this behavior without consumers being able to control any aspect of it. Then again, in that case I guess you just vendor net/http and update it on every Go release.

Seeing as I was the only reporter, and my issue was a problem that was actually an incidental bug, maybe we should just ignore this pending any further reports.

If you want to change errors you can implement your own ResponseWriter and
intercept them before calling Write().
On Fri, Feb 19, 2016 at 8:11 AM Tim Heckman notifications@github.com
wrote:

Confirmed. This ended up being a bug in the go-chef project. The Chef
server didn't end up needing the header itself, as was confirmed above.

The implementation appears to be accidentally including the header with
the request. Normally, there isn't any harm in doing so as I'm not sure of
any other servers that outright reject requests if there is a space in the
header.

So we're down to zero cases. I think that may be the sign that this issue
isn't important. The Chef issue was just my example. I still think that
this failure should not be abstracted away from consumers, as they should
have the final say on the request (as well as the error page that gets
returned by the server). This just stems from two desires:

• I'd like to be able to provide custom error pages, when possible,
  that is in a format expected by the consumer (e.g., people consuming JSON
  that check for an "error" key in the response). In its current
  implementation they would get text/plain I believe.
• I still don't like the idea of the HTTP client, that all other
  clients will seemingly be built on, enforcing this behavior without
  consumers being able to control any aspect of it. Then again, in that case
  I guess you just vendor net/http and update it on every Go release.

Seeing as I was the only reporter, and my issue was a problem that was
actually an incidental bug, maybe we should just ignore this pending any
further reports.

—
Reply to this email directly or view it on GitHub
#14392 (comment).

@elithrar I've been using net/http for awhile and don't remember the ability to set your own ResponseWriter for the *http.Server. Do you have an example?

@theckman Glad to hear this ended up working out.

I like that these validations are now in place. It prevents people from building APIs that use invalid headers. At least if they are using Go. 😄

@nathany So based on that should *http.Client.Do() return an error if a header name has a space in it?

I haven't verified it, but I believe it does, and personally I think it should. This is based on a 17-year old RFC and you've already found that the whitespace in your case is related to signing a request and not actual HTTP headers. We have no evidence that any server anywhere expects whitespace in header keys (and if any do, they are clearly broken).

Happy to discuss further on Slack, but I'd like to be respectful of Brad's time. He has 56 other open issues to deal with.

@nathany agreed; taken there.

@bradfitz is there a way the error could be improved so at least one side knows which header was the one that caused the failure?

is there a way the error could be improved so at least one side knows which header was the one that caused the failure?

Which error? Was any error mentioned in this bug?

Too many topics have been discussed here. Let's close this one and issue separate focused bugs from this. Please include details in the new bugs though with code and output.