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 CoreDNS DoH Server #1619

Merged
merged 13 commits into from May 21, 2018
Merged

make CoreDNS DoH Server #1619

merged 13 commits into from May 21, 2018

Conversation

@miekg
Copy link
Member

@miekg miekg commented Mar 18, 2018

1. What does this pull request do?

Add experimental DoH support.

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 18, 2018

Codecov Report

Merging #1619 into master will decrease coverage by 0.89%.
The diff coverage is 6.52%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1619     +/-   ##
=========================================
- Coverage   53.44%   52.55%   -0.9%     
=========================================
  Files         186      191      +5     
  Lines        9392     9590    +198     
=========================================
+ Hits         5020     5040     +20     
- Misses       3989     4164    +175     
- Partials      383      386      +3
Impacted Files Coverage Δ
plugin/normalize.go 76.27% <0%> (-2.68%) ⬇️
core/dnsserver/doh/responsewriter.go 0% <0%> (ø)
core/dnsserver/address.go 58.57% <0%> (-6.51%) ⬇️
core/dnsserver/register.go 23.42% <0%> (-1.11%) ⬇️
core/dnsserver/server_https.go 0% <0%> (ø)
core/dnsserver/doh/https.go 75% <75%> (ø)
core/dnsserver/server_grpc.go 8.1% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1822354...50d4655. Read the comment docs.

@miekg
Copy link
Member Author

@miekg miekg commented Mar 18, 2018

Also need a way to for http/2

and the forward and maybe proxy plugin should start speaking it.

Copy link
Contributor

@chantra chantra left a comment

Nice!

// MimeType is the DoH mimetype that should be used.
const MimeType = "application/dns-udpwireformat"

// RequestToMsg extra the dns message from the request body.

This comment has been minimized.

@chantra

chantra Mar 18, 2018
Contributor

typo: s/extra/extract/


buf, err := ioutil.ReadAll(req.Body)
if err != nil {
return nil, nil

This comment has been minimized.

@chantra

chantra Mar 18, 2018
Contributor

shouldn't this return the error?

}

w.Header().Set("Content-Type", doh.MimeType)
w.Header().Set("Cache-Control", "max-age=0")

This comment has been minimized.

@chantra

chantra Mar 18, 2018
Contributor

maybe a FIXME to implement cache-control later?

const MimeType = "application/dns-udpwireformat"

// RequestToMsg extra the dns message from the request body.
func RequestToMsg(req *http.Request) (*dns.Msg, error) {

This comment has been minimized.

@chantra

chantra Mar 18, 2018
Contributor

note that this only handle POST.
For GET use case, the content-type should be taken from the query parameter ct and the dns payload will be base64safeurl encoded (with trailing = removed) in the dns query parameter. Maybe worth a FIXME/TODO comment.

@miekg
Copy link
Member Author

@miekg miekg commented Mar 18, 2018

@chantra thanks for reviewing. yes error handling needs to be done - the draft is also light on this. I'll fix things after -04 is released.

@miekg miekg force-pushed the doh1 branch from 00ecfcb to f43a950 May 19, 2018
@miekg miekg changed the title WIP: make CoreDNS DoH Server make CoreDNS DoH Server May 19, 2018
@miekg
Copy link
Member Author

@miekg miekg commented May 19, 2018

@chantra PTAL, address all comments and implemented -08 of the draft.

@miekg
Copy link
Member Author

@miekg miekg commented May 19, 2018

Copy link
Member

@johnbelamaric johnbelamaric left a comment

minor issues, looks good

return nil, fmt.Errorf("no 'dns' query parameter found")
}
if len(b64) != 1 {
return nil, fmt.Errorf("multipe 'dns' query values found")

This comment has been minimized.

@johnbelamaric

johnbelamaric May 21, 2018
Member

nit: s/multipe/multiple/

This comment has been minimized.

@miekg

miekg May 21, 2018
Author Member

doine

buf, _ := dw.Msg.Pack()

w.Header().Set("Content-Type", mimeTypeDOH)
w.Header().Set("Cache-Control", "max-age=128") // Minttl as done in cache.

This comment has been minimized.

@johnbelamaric

johnbelamaric May 21, 2018
Member

No control over this?

This comment has been minimized.

@miekg

miekg May 21, 2018
Author Member

proper bug filed: #1823
The draft is still debating the exact requirements here, but it is clear what needs to be done. Left TODO with issue number.

@miekg miekg merged commit 18b92e1 into master May 21, 2018
2 of 4 checks passed
2 of 4 checks passed
coredns/ci Integration test failed 2/14 tests and 2/49 subtests.
Details
WIP work in progress – do not merge!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@miekg miekg deleted the doh1 branch May 21, 2018
@chantra
Copy link
Contributor

@chantra chantra commented May 23, 2018

Great stuff!

miekg added a commit that referenced this pull request May 23, 2018
The DoH work (#1619) made changes to pkg/nonwriter.Writer that in
hindsight were not backwards compatible; it added override for the
LocalAddr() and RemoteAddr(). Instead of rolling back that PR, this PR
reverts those changes and creates a DoHWriter for use in the
https-server.go side of things.

This was only caught in the integration test making this hard to catch,
so we add a upstream_file_test.go that tries (doesn't work yet) to test
this in the unit tests as well. Esp. helpful when 'git bisecting'.

Fixes #1826
miekg added a commit that referenced this pull request May 23, 2018
The DoH work (#1619) made changes to pkg/nonwriter.Writer that in
hindsight were not backwards compatible; it added override for the
LocalAddr() and RemoteAddr(). Instead of rolling back that PR, this PR
reverts those changes and creates a DoHWriter for use in the
https-server.go side of things.

This was only caught in the integration test making this hard to catch,
so we add a upstream_file_test.go that tries (doesn't work yet) to test
this in the unit tests as well. Esp. helpful when 'git bisecting'.

Fixes #1826
chrisohaver added a commit that referenced this pull request May 23, 2018
The DoH work (#1619) made changes to pkg/nonwriter.Writer that in
hindsight were not backwards compatible; it added override for the
LocalAddr() and RemoteAddr(). Instead of rolling back that PR, this PR
reverts those changes and creates a DoHWriter for use in the
https-server.go side of things.

This was only caught in the integration test making this hard to catch,
so we add a upstream_file_test.go that tries (doesn't work yet) to test
this in the unit tests as well. Esp. helpful when 'git bisecting'.

Fixes #1826
Jason-ZW added a commit to rancher/coredns that referenced this pull request Apr 17, 2019
* WIP: make CoreDNS DoH Server

* It works

* Fix tests

* Review from Tom - on diff. PR

* correct mime type

* Cleanups and use the pkg/nonwriter

* rename and updates

* implement get

* implement GET

* Code review comments

* correct context

* tweaks

* code review
Jason-ZW added a commit to rancher/coredns that referenced this pull request Apr 17, 2019
The DoH work (coredns#1619) made changes to pkg/nonwriter.Writer that in
hindsight were not backwards compatible; it added override for the
LocalAddr() and RemoteAddr(). Instead of rolling back that PR, this PR
reverts those changes and creates a DoHWriter for use in the
https-server.go side of things.

This was only caught in the integration test making this hard to catch,
so we add a upstream_file_test.go that tries (doesn't work yet) to test
this in the unit tests as well. Esp. helpful when 'git bisecting'.

Fixes coredns#1826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants