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

Reset keepalive per new tcp session #562

Merged
merged 1 commit into from Dec 1, 2021

Conversation

wtoorop
Copy link
Member

@wtoorop wtoorop commented Nov 1, 2021

Before, if edns-tcp-keepalive was configured, it was not sent along in responses unless a client on a TCP (or TLS) session signaled support for it, but then the options was sent along in responses to all clients using TCP (or TLS), also to the clients that did not signal support by sending a initial query with the option if they would reuse the internal comm_point for the TCP session.
Although according to RFC7828 Section 3.3.2. a server does not need an initial query with keepalive option to sent along the option in responses, it is peculiar that a random client was needed to sent the option for unbound to start sending along the option on all responses (on TCP sessions that reused that comm_point). Clearly the intention was to sent along the option in responses only to those clients that signaled support by sending an initial query with the option. This PR makes it behave just like that.

Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Okay from me.

Copy link
Member

@gthess gthess left a comment

Choose a reason for hiding this comment

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

Nice find! Ok from me also.

@gthess gthess merged commit 806a758 into master Dec 1, 2021
@gthess gthess deleted the bugfix/reset-keepalive-per-tcp-session branch December 1, 2021 02:57
gthess added a commit that referenced this pull request Dec 1, 2021
    - Merge PR #562 from Willem: Reset keepalive per new tcp session.
jedisct1 added a commit to jedisct1/unbound that referenced this pull request Jan 13, 2022
* nlnet/master: (36 commits)
  - Add missing configure flags for optional features in the   documentation. - Fix Unbound capitalization in the documentation.
  - Fix to pick up other class local zone information before unlock.
  - Allow local-data for classes other than IN to inherit a configured   local-zone's type if possible, instead of defaulting to type   transparent as per the implicit rule.
  - Add code similar to fix for ldns for tab between strings, for   consistency, the test case was not broken.
  Continue with version 1.14.1
  - Fix validator debug output about DS support, print correct algorithm.
  Changelog note for NLnetLabs#581
  Fix -Wshadow
  Fix -Wmissing-prototypes by declaring functions static.
  - Fix compile warning for if_nametoindex on windows 64bit.
  - Fix doc/unbound.doxygen to remove obsolete tag warning.
  - configure is set to 1.14.0, and release branch.
  - Fix NLnetLabs#574: Review fixes for size allocation.
  - Fix NLnetLabs#454: listen_dnsport.c:825: error: ‘IPV6_TCLASS’ undeclared.
  Changelog note for NLnetLabs#530:     - Merge PR NLnetLabs#530 from Shchelk: Fix: dereferencing a null pointer.
  Changelog note for NLnetLabs#522:     - Merge PR NLnetLabs#522 from sibeream: memory management violations fixed.
  Changelog note for NLnetLabs#562:     - Merge PR NLnetLabs#562 from Willem: Reset keepalive per new tcp session.
  Changelog note for NLnetLabs#555:     - Merge PR NLnetLabs#555 from fobser: Allow interface names as scope-id in       IPv6 link-local addresses.
  Changelog note for NLnetLabs#493:     - Merge PR NLnetLabs#493 from Jaap: Fix generation of libunbound.pc.
  Changelog note for NLnetLabs#511:     - Merge PR NLnetLabs#511 from yan12125: Reduce unnecessary linking.
  ...
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