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

Paddings in CONNECT/200 headers compressed by HPACK #72

Closed
darhwa opened this issue May 22, 2020 · 7 comments
Closed

Paddings in CONNECT/200 headers compressed by HPACK #72

darhwa opened this issue May 22, 2020 · 7 comments

Comments

@darhwa
Copy link

darhwa commented May 22, 2020

  • The padding in

    never take effect, because neither should one use naked naiveproxy server nor can that Padding header pass through fronting servers. Naiveproxy server sends the 200 response to caddy, and caddy answers the clients with its own 200 response. Never tried haproxy, but should be the same (correct me if I'm wrong). Maybe we should post a feature request to caddyserver/forwardproxy? But first, is the reason strong enough?

  • std::string(base::RandInt(16, 32), '.'));
    If I understand it right, this Padding header is in order to hide the high frequency of short CONNECT request as you mentioned in Traffic analysis of HTTP/2 CONNECT tunnels #1 . But HPACK will quickly defeat this purpose. There are only 17 distinguished Padding headers, so it's very likely that most of them will be on the dynamic table of HPACK after you run naiveproxy for a while. That's indeed what I observed in wireshark. Majority of Padding fields occupy only 1 byte each in the compressed frames. rfc7541#section-6.2.3 said that a header field can be marked as never-indexed thus won't be compressed. But I can't find how to expose that API in chrome's h2 stack. An easy workaround is to generate a random string for each Padding header. For example, base::Base64Encode(base::RandBytesAsString(base::RandInt(12, 24)), &padding_string). What's your opinion?

  • The Proxy-Authenticate header is compressed to 1 byte even quicker, further reducing size of CONNECT request. Should that be concerned?

@klzgrad
Copy link
Owner

klzgrad commented May 22, 2020

padding in 200 ok

True, it is ineffectual. It meant two things: this padding was not high on the priority list, and my Wireshark had problems decrypting TLS 1.3 and I forgot to fix that and verify the headers.

Patching forwardproxy means maintaining a nonstandard build of Caddy. It's a little risky for me.

hpack

Good find. I didn't verify it hard enough.

Proxy-Authorization

This header is not a source of obfuscation, so no matter.

@klzgrad
Copy link
Owner

klzgrad commented May 22, 2020

I do remember Haproxy passes the extra headers through. I'm not familiar with what's going on with Caddy these days. It's a usability issue also, because I can only detect padding support automatically if the header is being passed through. But it seems not easy since forwardproxy is not even updated for Caddy 2.

That workaround keeps thrashing the hpack table space. Just change the indexing policy here

bool DefaultPolicy(quiche::QuicheStringPiece name,
quiche::QuicheStringPiece /* value */) {
if (name.empty()) {
return false;
}
// :authority is always present and rarely changes, and has moderate
// length, therefore it makes a lot of sense to index (insert in the
// dynamic table).
if (name[0] == kPseudoHeaderPrefix) {
return name == ":authority";
}
return true;
}

with something like return name != "padding". I do wonder why is only :authority indexed and not other pseudo-headers, especially with :method: CONNECT unindexed. And can I simply index it. // Most common pseudo-header fields are represented in the static table, // while uncommon ones are small, so do not index them.

@darhwa
Copy link
Author

darhwa commented May 23, 2020

caddy

I'm sure forwardproxy doesn't pass the extra headers, see:
https://github.com/caddyserver/forwardproxy/blob/247c0bafaabd39e17ecf82c2c957c46957c2efcc/forwardproxy.go#L365

detect padding support automatically if the header is being passed through

Does that work with your HTTP Fast Open? I doubt.

return name != "padding"

Agree. This is better.

Moreover, unindexed headers still go through a Huffman code compression (rfc7541#appendix-B), thus a '.' occupies 6 bits instead of 8. So I changed the placeholder to '*' which is still 8 bits. Other candidates are [& , ; X Z].

#73

@klzgrad
Copy link
Owner

klzgrad commented May 23, 2020

The point is how to patch it so it does pass headers.

HTTP Fast Open can be deferred after autoconf of padding, which is a one time thing.

I checked and found out the x/net/http2/server in Caddy will return a Date header no matter what, which changes once per second. So the concern about wasting hpack space is moot. And in bigger picture this overhead is tiny compared to user payload, so it's back to your first idea with RandBytesAsString.

@darhwa
Copy link
Author

darhwa commented May 24, 2020

Patching hpack_encoder is ad hoc, but very concise. I stand by it. You decide.

@klzgrad
Copy link
Owner

klzgrad commented May 24, 2020

Yeah, but can you patch Caddy's hpack encoder? Not so easy then.

@klzgrad klzgrad changed the title Some findings on padding Padding in CONNECT/200 headers compressed by HPACK May 24, 2020
@klzgrad klzgrad changed the title Padding in CONNECT/200 headers compressed by HPACK Paddings in CONNECT/200 headers compressed by HPACK May 24, 2020
@klzgrad
Copy link
Owner

klzgrad commented May 24, 2020

Fixed. There is a followup change in Caddy forwardproxy that allows the padding in 200 to take effect but it's beyond the scope of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants