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

Fix edns keepalive #1317

Merged
merged 6 commits into from Dec 23, 2021
Merged

Fix edns keepalive #1317

merged 6 commits into from Dec 23, 2021

Conversation

ameshkov
Copy link
Contributor

@ameshkov ameshkov commented Dec 14, 2021

Added tests for EDNS0_TCP_KEEPALIVE pack/unpack as requested in the original PR.

Closes #1294

rs and others added 3 commits September 5, 2021 00:43
The current code is assuming the OPT code and length are part of the
data passed to the unpacker and that it should be appenedded during
packing while those fields are parsed by the caller.

This change fixes the parsing and make sure a request with a keepalive
won't fail.
@miekg
Copy link
Owner

miekg commented Dec 15, 2021

this succeeds #1294 ? and that can be closed?

@ameshkov
Copy link
Contributor Author

@miekg I think if you merge this one #1294 will be automatically merged as well since this PR contains the commit by @rs

edns.go Show resolved Hide resolved
edns.go Outdated Show resolved Hide resolved
edns.go Outdated Show resolved Hide resolved
@ameshkov
Copy link
Contributor Author

Fixed review comments

Copy link
Collaborator

@tmthrgd tmthrgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if the String change is correct or not, but the rest of it LGTM.

@miekg
Copy link
Owner

miekg commented Dec 22, 2021

hadn't spotted the String change, see no real reason do that (also), or is the RFC mandating something?

thanks @tmthrgd

@ameshkov
Copy link
Contributor Author

Restored String's behavior

Copy link
Collaborator

@tmthrgd tmthrgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@miekg miekg merged commit ba44371 into miekg:master Dec 23, 2021
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
* Fix un/packing of EDNS0 TCP keepalive extension

The current code is assuming the OPT code and length are part of the
data passed to the unpacker and that it should be appenedded during
packing while those fields are parsed by the caller.

This change fixes the parsing and make sure a request with a keepalive
won't fail.

* added tests for EDNS0_TCP_KEEPALIVE pack/unpack

* mark Length as deprecated

* removed named returns

* restored String

Co-authored-by: Olivier Poitrey <rs@rhapsodyk.net>
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

Successfully merging this pull request may close these issues.

None yet

4 participants