Vladimír Čunát authored 3 years ago and Tomas Krizek committed 3 years ago

Their implementation was changed.
Fortunately I was able to find the message in Google's cache
and thus discover easily which one it is in the new archive.

For old doh we added this in commit a34aa1ee;
with the new implementation we somehow forgot.

The approach of the code was rather hacky, simulating some packets
arriving from upstream and making the module stack CONSUME that.
Instead we take a direct approach now: use the simplified validator API
and then insert into cache directly.

One effect is improved performance, and consequently roughly halving
the lag which happens when prefill module invokes this.
(With root zone the lag goes down to 0.1 s from over 0.2 s,
 on my relatively fast CPU.  Fortunately it's just once a day.)

The following actions will now be logged in debug level (or request
tracing): ANSWER, DENY, DENY_MSG, DROP, REFUSE, TC

This can be useful for RPZ and other policy debugging.

Purposefully ommitted actions:
PASS - since it's the same as normal processing
REROUTE - the action itself comes from renumber module
STUB,FORWARD,TLS_FORWARD - this could be more confusing than useful
  (e.g. when response comes from cache)

Version 2.9 isn't supported anymore anyway, but 3.0.2 is needed for
extended error constants.

Credit for code goes to Vladimír Čunát

Tomas Krizek authored 3 years ago and Vladimír Čunát committed 3 years ago

Answers to EDNS requests from certain lua policies that use the
answer_clear() function would lack OPT RR and thus violate the MUST
condition in RFC6891.6.1.1.

Josh Soref authored 3 years ago and Tomas Krizek committed 3 years ago

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

sandbox.lua:72: attempt to call global 'log_notice' (a nil value)
Broken by commit 39dd89db (MR !1208)

With broken IPv6 and no knowledge of IP addresses, we were quite often
chosing to resolve a NS's AAAA and then using it... which wasn't good.
Let's give preference to A here as well.

Tomas Krizek authored 3 years ago and Vladimír Čunát committed 3 years ago

By default, notice level is set. Thus, if users want to use log() in the
same way as pre-5.4, they'd have to increase the log level. This bumps
the log level of log() function to keep the same behavior.

The RFC says we MUST do it, though this implementation is lazy and
avoids a SHOULD in the RFC.

It's not a perfect solution and with the future policy engine it will
hopefully be better, but it's really trivial to add this already.
(should've done that years ago)

The nghttp2 on_stream_close callback is only called for streams that are
properly closed. If we need to tear down the HTTP connection due to any
reason (e.g. IO error in underlying layer), some streams may not be
propely closed.

Due to HTTP/2 flow control, we may also wait indefinitely for the data
to be written. This can also cause the stream to never be properly
closed.

To handle these cases, a reference of allocated data is kept and we
ensure everything is freed once we're closing the http session.

Vladimír Čunát authored 3 years ago and Tomas Krizek committed 3 years ago

Partly to document changes from recent changes,
partly to fix long-lasting issues in the descriptions.
Hopefully it will be easier to understand now.

The practical problem was also mitigated by libuv >= 1.32.0 (2ee2d46)

Vladimír Čunát authored 3 years ago and Štěpán Balážik committed 3 years ago

Perhaps this bug was now more pronounced since 5.3.0 changes.
Example problem was disabling minimization or 0x20 (globally or
for some problematic requests); without this change they would get
re-enabled during some fallback actions... which might be exactly
the wrong moment wrt. the motivation to setting these.
https://gitter.im/CZ-NIC/knot-resolver?at=60a221e86a950f3d46ed1cd9

Vladimír Čunát authored 3 years ago and Tomas Krizek committed 3 years ago

Discovered case: TCP write towards upstream fails due to ECONNRESET,
and on this place of code we "forget" the whole qr_task and thus its
corresponding kr_request, so it remains unanswered and using memory.

The purpose of this change is to make it harder to accidentally use the
legacy DoH implementation and free up the "doh" kind which may be used
as an alias to a modern implementation in the future.

Case: NSEC3 with too many iterations used for a positive wildcard proof.

To really fix the answers, this also needed fixing the `any_rank` part
which I somehow forgot in commit 7107faeb :-(

It's spread over multiple commits.  The other undefined-behavior fixes
probably don't cause any issue in practice, so they aren't mentioned.

Vladimír Čunát authored 3 years ago and Tomas Krizek committed 3 years ago

In particular, ignore ENOSPC from LMDB for a short time unless the
space-usage estimate is over 90%.  See code comments for details.

Example case: denying existence of ok.rdns.dev by
oj\255.rdns.dev. NSEC ok\000.rdns.dev.
This NSEC end was incorrectly ordered with the QNAME.
https://gitter.im/CZ-NIC/knot-resolver?at=606055b82beb1e1da3d73892

The code is Libor's :-)

In practice it can easily happen, as loading module and really
configuring it is often done separately.  Then we'd see two fstrm
threads, etc.