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

Implement RFC7830 padding for queries #39

Open
wants to merge 10 commits into
base: master
from

Conversation

@dmcardle
Copy link
Contributor

dmcardle commented Nov 20, 2019

No description provided.

tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/doh_test.go Outdated Show resolved Hide resolved
@dmcardle

This comment has been minimized.

Copy link
Contributor Author

dmcardle commented Nov 20, 2019

Argh, so one other thing to think about is that the incoming message is not guaranteed to be fully label-compressed.

(As an aside, is it okay to compress messages that were not already compressed?)

I really should unpack and pack the rawMsg before using its length.

tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
}

// Build the padding option that we will need. We can't use
// the length of |rawMsg|, since its labels may be compressed

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 20, 2019

Contributor

Sounds tricky. Can we add a test? You could take the same dnsmessage.Message, pack it compressed and uncompressed, verify that the sizes are different, and then verify that the sizes are the same after padding.

I do feel I should point out that compression is unlikely to activate in queries, since it only applies when there are multiple names with a shared suffix, and queries normally contain only a single name.

This comment has been minimized.

Copy link
@dmcardle

dmcardle Nov 21, 2019

Author Contributor

OK, so the library isn't flexible enough to allow me to choose compressed or not. I've sort of inverted your suggestion with two tests.

Test 1: Take bytes of a compressed query, unpack and repack with compression. Check that the original and repacked queries have the same length.

Test 2: Take bytes of an uncompressed query, unpack and repack with compression. Check that the original query is larger than the repacked query.

These tests are really just verifying that the dnsmessage library does what we expect. We should also test that an uncompressed query comes out padded to the right size.

This comment has been minimized.

Copy link
@bemasc

bemasc Dec 2, 2019

Contributor

OK, so the library isn't flexible enough to allow me to choose compressed or not.

It looks like there might be a way:
https://godoc.org/golang.org/x/net/dns/dnsmessage#Builder.EnableCompression

These tests are really just verifying that the dnsmessage library does what we expect.

That doesn't sound great. I think the real goal is to test that this does the right thing when the input is not compressed (or is compressed differently from how dnsmessage would do it).

This comment has been minimized.

Copy link
@dmcardle

dmcardle Dec 2, 2019

Author Contributor

You're right, it actually doesn't look too hard to use the Builder to control whether compression is enabled.

I think the real goal is to test that this does the right thing when the input is not compressed (or is compressed differently from how dnsmessage would do it).

Absolutely. To be clear, the "sanity check" tests on the behavior of the dnsmessage library are not the only tests.

I manually constructed the bytes of uncompressed and compressed queries, and wrote some tests using those. I also have tests for queries with/without OPT and with/without padding.

Let me know if there's some situation I'm missing, and/or if you think I should explicitly use Builder.

dmcardle added 2 commits Nov 21, 2019
@dmcardle

This comment has been minimized.

Copy link
Contributor Author

dmcardle commented Dec 2, 2019

@bemasc Don't forget me :)

tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
tunnel/intra/doh/padding.go Outdated Show resolved Hide resolved
}

// Build the padding option that we will need. We can't use
// the length of |rawMsg|, since its labels may be compressed

This comment has been minimized.

Copy link
@bemasc

bemasc Dec 2, 2019

Contributor

OK, so the library isn't flexible enough to allow me to choose compressed or not.

It looks like there might be a way:
https://godoc.org/golang.org/x/net/dns/dnsmessage#Builder.EnableCompression

These tests are really just verifying that the dnsmessage library does what we expect.

That doesn't sound great. I think the real goal is to test that this does the right thing when the input is not compressed (or is compressed differently from how dnsmessage would do it).

dmcardle added 2 commits Dec 2, 2019
msg.Additionals = append(msg.Additionals, dnsmessage.Resource{
Header: dnsmessage.ResourceHeader{
Name: dnsmessage.MustNewName("."),
Class: dnsmessage.ClassINET,

This comment has been minimized.

Copy link
@bemasc

bemasc Dec 2, 2019

Contributor

Unfortunately, OPT has some crazy ideas about what's supposed to go in these fields. I think the "right way" is to call SetEDNS0. To start, I would try udpPayloadLen = 65535, extRCode = 0, and dnssecOK = false.

This comment has been minimized.

Copy link
@dmcardle

dmcardle Dec 2, 2019

Author Contributor

Ah, thanks for that. Done!

I also improved the integration test so it sends a padded query to the server as well. FYI, it turns out that HonestDNS's responses include padding regardless of the request's padding.

@dmcardle

This comment has been minimized.

Copy link
Contributor Author

dmcardle commented Dec 8, 2019

Friendly ping :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.