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

Add bufsize plugin for preparing the DNS Flag Day and avoiding IP fragmentation #3401

Merged
merged 8 commits into from Nov 10, 2019

Conversation

@ykhr53
Copy link
Collaborator

ykhr53 commented Oct 26, 2019

1. Why is this pull request needed and what does it do?

In the next DNS Flag Day, message size should be considered to avoid IP fragmentation.

https://dnsflagday.net/2020/

Action: DNS Software Vendors
It is important for DNS software vendors to comply with DNS standards, and to use a default EDNS buffer size (1232 bytes) that will not cause fragmentation on typical network links.

Also we have to consider about fragment attack, bufsize can limit requester's UDP payload size.
This plugin can follow this draft too.

https://tools.ietf.org/html/draft-fujiwara-dnsop-fragment-attack-01#section-4.3

4.3. Limit requestor's UDP payload size to 512
Limiting EDNS0 requestor's UDP payload size [RFC6891] to 512 may be a
measure of path MTU attacks.

This plugin prevents IP fragmentation so that to be ready for the DNS Flag Day 2020 and to deal with DNS vulnerability.

2. Which issues (if any) are related?

#2972

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

None.

4. Does this introduce a backward incompatible change or deprecation?

None.

@corbot corbot bot requested a review from dilyevsky Oct 26, 2019
@corbot

This comment has been minimized.

Copy link

corbot bot commented Oct 26, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked dilyevsky (via /OWNERS) for a review.
Note this is not an exclusive request. Anyone is free to provide a review of this pull request.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
@ykhr53 ykhr53 force-pushed the ykhr53:add-bufsize branch from 1e5fae0 to fb8be48 Oct 26, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 26, 2019

Codecov Report

Merging #3401 into master will increase coverage by 0.14%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3401      +/-   ##
==========================================
+ Coverage   56.36%   56.51%   +0.14%     
==========================================
  Files         218      222       +4     
  Lines       10832    11005     +173     
==========================================
+ Hits         6105     6219     +114     
- Misses       4252     4302      +50     
- Partials      475      484       +9
Impacted Files Coverage Δ
plugin/bufsize/bufsize.go 100% <100%> (ø)
plugin/bufsize/setup.go 57.69% <57.69%> (ø)
plugin/file/reload.go 69.44% <0%> (-5.56%) ⬇️
plugin/transfer/transfer.go 79.74% <0%> (ø)
plugin/transfer/setup.go 50.81% <0%> (ø)

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 5d8bda5...ec38402. Read the comment docs.

@ykhr53 ykhr53 marked this pull request as ready for review Oct 26, 2019
Copy link
Collaborator

dilyevsky left a comment

Thanks for the change! The functionality itself appears to be 👌 I have only a few style comments. +r @miekg for new plugin approval.

plugin/bufsize/bufsize_test.go Outdated Show resolved Hide resolved
plugin/bufsize/setup.go Outdated Show resolved Hide resolved
plugin/bufsize/setup.go Outdated Show resolved Hide resolved
plugin/bufsize/bufsize.go Outdated Show resolved Hide resolved
plugin/bufsize/bufsize.go Show resolved Hide resolved
@dilyevsky dilyevsky requested a review from miekg Oct 31, 2019
Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
@ykhr53 ykhr53 force-pushed the ykhr53:add-bufsize branch from 3c75851 to 40e70a8 Oct 31, 2019
ykhr53 added 3 commits Oct 31, 2019
Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
@ykhr53 ykhr53 requested a review from dilyevsky Oct 31, 2019
Copy link
Member

miekg left a comment

The core of this plugin lgtm and I suspect it would be that small. A whole bunch of other comments and a big one on how to deal with the various network families.

plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/README.md Show resolved Hide resolved
plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/README.md Outdated Show resolved Hide resolved
plugin/bufsize/bufsize.go Show resolved Hide resolved
plugin/bufsize/setup.go Show resolved Hide resolved
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 8, 2019

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
@ykhr53 ykhr53 requested a review from miekg Nov 8, 2019
Copy link
Collaborator Author

ykhr53 left a comment

Thank you for reviewing! I fixed some comments on README, so please re-review it. thanks :)

plugin/bufsize/setup.go Outdated Show resolved Hide resolved
Copy link
Member

miekg left a comment

one nit, lgtm otherwise

Copy link
Collaborator Author

ykhr53 left a comment

you mean, need to change the function name to parse from bufsizeParse?

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 8, 2019

@ykhr53

This comment has been minimized.

Copy link
Collaborator Author

ykhr53 commented Nov 8, 2019

ok, thanks!
I've just followed some other plugins like nsidParse or cacheParse.
But I can see some plugins use parse, so I'll change to parse!

ykhr53 added 2 commits Nov 8, 2019
Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 8, 2019

@ykhr53 ykhr53 requested a review from miekg Nov 8, 2019
@miekg
miekg approved these changes Nov 10, 2019
@miekg miekg merged commit e23a34a into coredns:master Nov 10, 2019
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 56.51% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@ykhr53 ykhr53 deleted the ykhr53:add-bufsize branch Nov 10, 2019
@ykhr53 ykhr53 restored the ykhr53:add-bufsize branch Nov 11, 2019
gonzalop added a commit to gonzalop/coredns that referenced this pull request Nov 18, 2019
…gmentation (coredns#3401)

* add bufsize plugin

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* add docstring and comment

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* delete stdout messages when get an error

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* change to context.Background from TODO

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* define default bufsize as defaultBufSize constant

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* fix some comments

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* function name change: parse

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* function name change: parse

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
gonzalop added a commit to gonzalop/coredns that referenced this pull request Nov 18, 2019
…gmentation (coredns#3401)

* add bufsize plugin

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* add docstring and comment

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* delete stdout messages when get an error

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* change to context.Background from TODO

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* define default bufsize as defaultBufSize constant

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* fix some comments

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* function name change: parse

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>

* function name change: parse

Signed-off-by: ykhr53 <yukihira.lab@gmail.com>
Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.