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

Make enarx user info work #2087

Closed
bstrie opened this issue Jul 21, 2022 · 3 comments · Fixed by enarx/drawbridge#256 or #2126
Closed

Make enarx user info work #2087

bstrie opened this issue Jul 21, 2022 · 3 comments · Fixed by enarx/drawbridge#256 or #2126
Assignees
Labels
bug Something isn't working ux User experience

Comments

@bstrie
Copy link
Contributor

bstrie commented Jul 21, 2022

Currently attempting to invoke enarx user info fails with:

Error: Failed to get record for user: bstrie

Caused by:
    missing `content-length` header
@platten platten added the ux User experience label Jul 25, 2022
@bstrie bstrie removed their assignment Aug 8, 2022
@rvolosatovs
Copy link
Member

rvolosatovs commented Aug 8, 2022

The culprit of this issue is the following:

  1. oauth2/openidconnect crates depend on ureq with default features and, hence, enable "gzip" feature https://github.com/ramosbugs/oauth2-rs/blob/93452e57b52a9ecd374e7b4293836465d70caad6/Cargo.toml#L32
  2. (1.) results in accept-encoding=gzip header being sent by the client (Enarx CLI, exec-wasmtime does not depend on aforementioned crates and therefore do not run into this issue)
  3. Since the header is sent, Nginx strips Content-Length header and enables Transfer-Encoding: chunked
  4. Since Content-Length header is not present, Drawbridge client library throws an error https://github.com/profianinc/drawbridge/blob/f2965264059c2ca7c8e45831555bccd7dddc36a7/crates/client/src/entity.rs#L152, because it uses the Content-Length header to validate the size of received entity.

According to HTTP 1.1 spec, the Content-Length MUST NOT be present when Transfer-Encoding is used

When a message does not have a Transfer-Encoding header field, a Content-Length header field can provide the anticipated size, as a decimal number of octets, for a potential payload body. For messages that do include a payload body, the Content-Length field-value provides the framing information necessary for determining where the body (and message) ends. For messages that do not include a payload body, the Content-Length indicates the size of the selected representation (Section 3 of [RFC7231]).

Content-Length = 1*DIGIT

An example is

Content-Length: 3495

A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field.

https://httpwg.org/specs/rfc7230.html#rfc.section.3.3.2

In http2 spec, Transfer-Encoding is removed and therefore, it seems that Content-Length may always be present https://www.rfc-editor.org/rfc/rfc9113.html#name-malformed-messages

A few options here to proceed:

  1. Figure out a way to disable gzip ureq feature (I don't know of a nice way to do that without changing the third-party crates, since features are additive, @bstrie ?). This should be a viable option for a quick-fix, but it will be quite bad from performance perspective long-term, since we DO want to use streaming as opposed to fetching full workloads in one huge request, for example
  2. Use http2, since it seems that Content-Length header in that spec would not be removed even when streaming. Not sure it will work, however, because all connections start as http1.1 as far as I understand. @puiterwijk @npmccallum any insights?
  3. Introduce a custom header for sending the expected total size of the entity. I would prefer only doing this as last-resort if everything else fails.
  4. Do not validate the size of the entity against a header value. This means that for direct fetches of workloads there's no expected "size" to compare downloaded value with. We can, and will continue to, compare the size of the entity to the parent entry (e.g. the directory entry size or tag entry size). I do not think that this approach compromises security in any way, since the Content-Length, if sent, is part of the response containing the body anyway, but it does hurt performance, since size cannot be known ahead-of-time. Knowing the size is also useful for asserting integrity of data.

My preferred option is 2., if it works (I will try tomorrow) and is spec compliant. I would like to get some input meanwhile.

Note, also that setting this option to off https://nginx.org/en/docs/http/ngx_http_core_module.html#chunked_transfer_encoding in Nginx does remove the Transfer-Encoding header, but Content-Length is missing as well as a result. It may be that the option has to be applied twice to alleviate that.

@rvolosatovs rvolosatovs added the discussion This requires discussion before action can be taken label Aug 8, 2022
@bstrie
Copy link
Contributor Author

bstrie commented Aug 8, 2022

I don't want to disable gzip, so 1 is out. 2 seems fine to me if it's feasible. I'm also not opposed to 3, since it seems like what we're currently doing is a mild misuse of the Content-Length header, and even if we fix this particular issue who knows if we'll hit a similar one in the future.

@rvolosatovs rvolosatovs added bug Something isn't working and removed discussion This requires discussion before action can be taken labels Aug 9, 2022
@rvolosatovs
Copy link
Member

rvolosatovs commented Aug 9, 2022

Filed a PR in Drawbridge to explicitly disable the encoding enarx/drawbridge#256, which fixes the issue.

I don't want to disable gzip, so 1 is out. 2 seems fine to me if it's feasible. I'm also not opposed to 3, since it seems like what we're currently doing is a mild misuse of the Content-Length header, and even if we fix this particular issue who knows if we'll hit a similar one in the future.

As it stands today, no transfer-encoding (inluding gzip) is supported by Drawbridge client. Once we have support for it, we can enable it enarx/drawbridge#255, for now we just need to fix the issue and we can improve later.
Note, that it's more efficient to not use the transfer-encoding for this particular request. We can get a marginal performance improvement on workload fetching however, we'd need to profile that.

The only reason to use chunked transfer/streaming would be to improve server efficiency and throughput.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ux User experience
Projects
None yet
3 participants