Content-Length and Transfer-Encoding are mutually exclusive HTTP headers

Fix proposed to branch: master
Review: https://review.openstack.org/6563

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/6564

Fix proposed to branch: stable/diablo
Review: https://review.openstack.org/6565

According to your quote from RFC2616, pound is violating the HTTP standard by not ignoring Content-Length in this case.
If it did, all would be working (because the chunked encoding itself specifies lengths)

Ah, the relevant section is this:
4.4. 3. If a Content-Length header field (section 14.13) is present, its decimal value in OCTETs represents both the entity-length and the transfer-length. The Content-Length header field MUST NOT be sent if these two lengths are different (i.e., if a Transfer-Encoding header field is present).

So the change is still needed to make glance-client standards-compliant.

Yeah, sorry, I should have included both the client-side and server-side directive when I quoted it.

It's also further complicated because Transfer-Encoding is a hop-by-hop header, so pound has to make an active effort to reuse it; since hop-by-hop headers are not implicitly passed through, "ignore" means "drop" in that case. The spec could be a little clearer there for proxies, but I think what it's doing is reasonable.

As I read the RFC, it says that, when there are both Transfer-Encoding and Content-Length headers, the latter (which means Content-Length, because it is listed last in this sentence) MUST be dropped. It is the only thing that makes sense in this situation.
So keeping Content-Length and dropping Transfer-Encoding is
a) the wrong thing to do and
b) violating this RFC.

Ah. Interesting. In the sentence "If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored." I interpreted "latter" as "latter header received", which pound also appears to have done, but I think your reading is correct.

I read the RFC to mean latter == Content-Length, since Content-Length was mentioned after Transfer-Encoding. And in the case of Pound with Glance, Pound is correctly ignoring the Content-Length header. Not sure I fully agree that "ignore" means "remove the header"... but c'est la vie.

Here's the problem: without a Content-Length header, the Glance API server is unable to correctly understand the total length of the bytestream to expect for an image, and therefore is unable to validate a number of things, including whether the image blob in the request is greater than the maximum image size Glance will accept.

I suppose we can live with this restriction -- but it will in effect be a security hole, as someone could send a gigantic stream of randomized data to Glance and there is no fail-fast short-circuit to prevent this type of runaway request.

There used to be issues in the Swift storage when an image's size could not previously be determined, but with Ewan Mellor's rewrite of the Swift chunking iterator, I believe that problem is since solved...

As an aside, this same issue exists in Swift (where some of the Glance client code comes from...). You will notice that the Swift put_object() call contains a content_length parameter that gets written into the HTTP headers -- even though the Transfer-Encoding: chunked header is written:

https://github.com/openstack/swift/blob/master/swift/common/client.py#L628

A different approach to solve this could be to keep the Content-Length header (if the size can be determined) and drop the Transfer-Encoding instead.

But, there still needs to be support for the streaming case, which is used for such calls
curl $URL | glance add ...

The downside is that this would make the code slightly more complex.
Another way would be to always use chunked encoding without Content-Length, but have only one big chunk for files of known size, so that the server could determine if the size is acceptable right after receiving the first bytes.

The content-length can only be considered a hint, you can never actually trust it. In the current code, using the filesystem store, you can send a content-length of 4 and then dump 100PB on glance and it'll accept it. The code that actually reads from the socket needs to stop when it hits IMAGE_SIZE_CAP; this is a minor security problem, but it requires auth, so it's not too bad.

In any case, I see the lack of input validation on content-length to be a separate bug; it will require some amount of rearchitecting because the store assumes the image_size/content-length can be trusted, but only the store knows how big the data actually is. Note also that chunked-encoding in glance is the fallback path; any performant implementation should be using the sendfile api anyway. I might even go so far as to say that glance should die immediately on startup when sendfile is not supported, unless the user passes a flag; sendfile has so much better performance that it's no contest.

Swift does not violate the HTTP spec that I can see, it only sends chunked if it doesn't send content-length: https://github.com/openstack/swift/blob/master/swift/common/client.py#L682 and 690. As far as I know, there's no max object size for swift, so there's no reasonable point at which the server can reject input ahead of time. Instead, it fallocates the content-length (https://github.com/openstack/swift/blob/master/swift/obj/server.py#L560) which would be rejected if it were larger than the filesystem could handle. It also validates that the upload_size matches the content-length (and refuses to add the object to the hash if violated: https://github.com/openstack/swift/blob/master/swift/obj/server.py#L576). I don't understand the intricacies of swift, but seeing that leads me to believe that someone specifically thought about it and handled it appropriately.

Does this set your fears to rest so you'll +1 the review?

Hi Mike,

One slightly related point we discussed at the summit was to require the presence of *some* header identifying the total image size.

This was intended to feed into some new API call-cost estimation logic to be used with smarter rate limiting.

Now this header doesn't necessarily have to be content-length, probably better to instead always set x-image-meta-size or a new header such as x-glance-image-size.

Cheers,
Eoghan

I agree with you, Mike.. was just trying to add some background information to the bug, that's all :)

As for Swift, Swift always uses chunked-transfer-encoding if there is a readable file-like object that is passed to put_object(). It will also include a Content-Length header if the content_length parameter is not None:

https://github.com/openstack/swift/blob/master/swift/common/client.py#L676

Best,
-jay

Reviewed: https://review.openstack.org/6563
Committed: http://github.com/openstack/glance/commit/223fbee49a55691504623fa691bbb3e48048d5f3
Submitter: Jenkins
Branch: master

commit 223fbee49a55691504623fa691bbb3e48048d5f3
Author: Mike Lundy <email address hidden>
Date: Fri Apr 13 19:53:16 2012 -0700

    Omit Content-Length on chunked transfer

    Content-Length and Transfer-Encoding conflict according to the HTTP
    spec. This fixes bug 981332.

    This also adds the ability to test both the sendfile-present and
    sendfile-absent codepaths; the sendfile-present test will be skipped on
    sendfile-absent platforms.

    Change-Id: Ibb017b4a43d89ffdb6b868648296603cfec7780d

Jay, unless there's something I'm missing, swift does not set chunked when there is a content-length.
https://github.com/openstack/swift/blob/master/swift/common/client.py#L690

if hasattr(contents, 'read'):
    if content_length is None:
        conn.putheader('Transfer-Encoding', 'chunked')
        conn.endheaders()
    else:
        conn.endheaders()

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/6776

Mike, you're absolutely correct. Removed reference to Swift bug.

Reviewed: https://review.openstack.org/6776
Committed: http://github.com/openstack/glance/commit/7a9e3a7dc53a20971dbd653f2061337efec136a2
Submitter: Jenkins
Branch: stable/essex

commit 7a9e3a7dc53a20971dbd653f2061337efec136a2
Author: Mike Lundy <email address hidden>
Date: Fri Apr 13 19:53:16 2012 -0700

    Omit Content-Length on chunked transfer

    Content-Length and Transfer-Encoding conflict according to the HTTP
    spec. This fixes bug 981332.

    This also adds the ability to test both the sendfile-present and
    sendfile-absent codepaths; the sendfile-present test will be skipped on
    sendfile-absent platforms.

    [ This is backported from 223fbee49a55691504623fa691bbb3e48048d5f3 ]

    Change-Id: I20856eb51ff66fe4b7145f796a540a832c3aa4d8

Reviewed: https://review.openstack.org/6565
Committed: http://github.com/openstack/glance/commit/bad506d840aede9c7bb263393efc495f287affed
Submitter: Jenkins
Branch: stable/diablo

commit bad506d840aede9c7bb263393efc495f287affed
Author: Mike Lundy <email address hidden>
Date: Fri Apr 13 19:53:16 2012 -0700

    Omit Content-Length on chunked transfer

    Content-Length and Transfer-Encoding conflict according to the HTTP
    spec. This fixes bug 981332.

    [ This is backported from 223fbee49a55691504623fa691bbb3e48048d5f3 ]

    Change-Id: I62f2bcabd9712361dd8837e39a577c4add052d0f

** Impact **

If the glance api server is behind a conforming HTTP/1.1 reverse proxy (pound, in this example), all uploaded images will be corrupted. This is because glance.client sends both the "Content-Length" and "Transfer-Encoding: chunked" headers. The HTTP/1.1 spec (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4) says:

"If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored."

The glance client is sending Content-Length first, and pound sanitizes the request, so it rightfully strips out the Transfer-Encoding. This means that the chunk sizes in the body of the request are written as data to the image, resulting in a corrupted image.

This affects diablo, essex, and folsom, though it will only affect non-sendfile platforms on essex and folsom (since that codepath is not affected).

** Development Fix **

This is fixed in the development trunk at: https://review.openstack.org/6563 and in quantal

** Stable Fix **

This is fixed in the stable/essex branch at: https://review.openstack.org/6776

** Test Case **

Run the glance unit tests

** Regression Potental **

Minimal, this code path is not used in Ubuntu by default.

Hello Mike, or anyone else affected,

Accepted glance into precise-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Please find the attached Jenkins job results from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Glance has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

For records of upstream test coverage of this update, please see the Jenkins links in the comments of the relevant upstream code-review(s):

Trunk review: https://review.openstack.org/6563
Stable review: https://review.openstack.org/6776

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

Test coverage log.

This looks ready to move to -updates. What is the status of this in Quantal? We need to be sure that it'll be addressed there so that upgraders don't suffer a regression.

This bug was fixed in the package glance - 2012.1+stable~20120608-5462295-0ubuntu2.2

---------------
glance (2012.1+stable~20120608-5462295-0ubuntu2.2) precise-proposed; urgency=low

  * New usptream snapshot. (LP: #1010473)
  * Resynchronize with stable/essex:
   - 5462295 Fix i18n in glance.notifier.notify_kombu. (LP: #983829)
   - 7a9e3a7 Omit Content-Length on chunked transfer. (LP: #981332)
   - 5838b63 Fix content type for qpid notifier. (LP: #980872)
   - 98913da search for logger in PATH. Fixes (LP: #978907)
   - f136e7e Ensure swift auth URL includes trailing slash. Fixes (LP: #979745)
  * debian/rules: Add ability to build tarballs from a git snapshot.
  * debian/patches/fix-pep8-ubuntu.patch: Fix pep8 errors.
 -- Chuck Short <email address hidden> Tue, 05 Jun 2012 10:43:12 -0400

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.