dig was not working with a server with offloaded TLS;
this turned out to be because it was sending the URI as
"https://<address>/dns-query", not "https://<name>/dns-query".
using userarg instead of servname fixes the problem. (incidentally
also corrected logged output to use the hostname when available.)
calling recv_done() with ISC_R_CANCELED prevents dig from cleaning
up fully from the query, because when that result code is used
it means some resources are already gone. changing the result
to ISC_R_UNEXPECTED prevents the hang.
- a bug that was already fixed in tcpdns.c and tlsdns.c had been
left unfixed in tcp.c: we needed to check whether the socket() call
succeeds before attempting any asynchronous connection code.
this corrects a problem that occurred when uv__socket() failed
with too many open files.
- also corrected the behavior of nm_connectcb(). previously, it was
forcing synchronous execution when force_async was false. what we
want is to force asynchronous execution if force_async is true. if
force_async is false, we want it to decide based on whether we're in
the network thread.
isc_nm_httprequest() is retained for use by doh_test, but has
been renamed isc__nm_http_request() and is no longer part of
the main API.
eventually doh_test should be changed to use isc_nm_read() and
isc_nm_send(), and this function should be removed.
we don't want to call isc__nm_http_cleanup_data() for TLS sockets
or isc__nm_tls_cleanup_data() for HTTP sockets, they can step on
each other and cause a crash or deadlock.
this isn't working yet, but it's a step in the right direction:
we need to call the connect callback from isc_nm_httpconnect()
asynchronously, after the connect has already returned. this
will enable dig to unlock the lookup before tcp_connected()
locks it, as it does when using tcpdns or tlsdns sockets.
this commit also removes the http_connect_t structure and stores
the connection data in a newly created http socket instead. (since
the socket structure was going to be needed to call isc__nm_connectcb()
anyway, it seemed like a good idea to make use of it to simplify
the connection code.)
in tls_call_connect_cb(), we want to update the result for
the TLS socket. however, in some error conditions on connect,
the handle we're passing is for the TCP socket instead. this
was causing the wrong socket's result code to be updated, and
the thread that was waiting for the result to be updated never
woke up.
(note: while the fix in this commit causes doh_test to stop
deadlocking, I'm not sure whether I've actually fixed the bug.
in other places, we create a new 'tlshandle' to pass into
tls_call_connect_cb(). perhaps we should be doing that in
all cases?)
incidentally, I also renamed tls_connect_cb() to tcp_connected(),
because the handle passed in is for the TCP socket, and I kept
being confused by the name having "tls" in it.
the goal here is to have the interface to http sockets be
the same as other types of netmgr socket, so that (for
example) dig can open an http connection and then just work
with no further code change, the same as it does with tcpdns
or tlsdns.
but the supporting code for https seems to be written with
the assumption that there will be a tight connection between
a request and a response, so the send and read functions have been
compressed into a single 'httprequest' function call. in an effort
to make the normal send and read semantics work correctly, we're
trying to set it up so that read sets us up to listen for a
response, send sets up the query we're sending, and whichever one
comes second triggers the transaction.
currently this is working in doh_test, but dig is failing due to a
deadlock; isc_http_connect() needs to return so launch_next_query()
can unlock the lookup before tcp_connected() runs.
instead of setting up ALPN in isc_tlsctx_createclient(), we now
have a function isc_tlsctx_enable_http2client_alpn() that can be
run from isc_nm_httpconnect(). isc_tlsctx_enable_http2server_alpn()
can similarly be run from isc_nm_httpconnect().
- style, cleanup, and removal of unnecessary code
- combine isc_nm_http_add_endpoint() and isc_nm_http_add_doh_endpoint()
into one function, renamed isc_http_endpoint()
- move isc_nm_http_connect_send_request() into doh_test.c as a helper
function; remove it from the public API
- renamed isc_http2 and isc_nm_http2 entities to just isc_http and
isc_nm_http, for consistency with other existing names
- shortened a number of long names
- the caller is now responsible for determining the peer address
in isc_nm_httpconnect(); this eliminates the need to parse the URI
and the dependency on an external resolver. the caller is also now
responsible for creating the SSL client context, for consistency with
isc_nm_tlsdnsconnect().
Three small cleanups:
1. Remove an unused keystr/dst_key_format.
2. Initialize a dst_key_state_t state with NA.
3. Update false comment about local policy (local policy only adds
barrier on transitions to the RUMOURED state, not the UNRETENTIVE
state).
There was a bug in function 'keymgr_ds_hidden_or_chained()'.
The funcion 'keymgr_ds_hidden_or_chained()' implements (3e) of rule2
as defined in the "Flexible and Robust Key Rollover" paper. The rules
says: All DS records need to be in the HIDDEN state, or if it is not
there must be a key with its DNSKEY and KRRSIG in OMNIPRESENT, and
its DS in the same state as the key in question. In human langauge,
if all keys have their DS in HIDDEN state you can do what you want,
but if a DS record is available to some validators, there must be
a chain of trust for it.
Note that the barriers on transitions first check if the current
state is valid, and then if the next state is valid too. But
here we falsely updated the 'dnskey_omnipresent' (now 'dnskey_chained')
with the next state. The next state applies to 'key' not to the state
to be checked. Updating the state here leads to (true) always, because
the key that will move its state will match the falsely updated
expected state. This could lead to the assumption that Key 2 would be
a valid chain of trust for Key 1, while clearly the presence of any
DS is uncertain.
The fix here is to check if the DNSKEY and KRRSIG are in OMNIPRESENT
state for the key that does not have its DS in the HIDDEN state, and
only if that is not the case, ensure that there is a key with the same
algorithm, that provides a valid chain of trust, that is, has its
DNSKEY, KRRSIG, and DS in OMNIPRESENT state.
The changes in 'keymgr_dnskey_hidden_or_chained()' are only cosmetical,
renaming 'rrsig_omnipresent' to 'rrsig_chained' and removing the
redundant initialization of the DST_KEY_DNSKEY expected state to NA.
The previous commit changed the function definition of
'keymgr_key_is_successor()', this commit updates the code where
this function is called.
In 'keymgr_key_exists_with_state()' the logic is also updated slightly
to become more readable. First handle the easy cases:
- If the key does not match the state, continue with the next key.
- If we found a key with matching state, and there is no need to
check the successor relationship, return (true).
- Otherwise check the successor relationship.
In 'keymgr_key_has_successor()' it is enough to check if a key has
a direct successor, so instead of calling 'keymgr_key_is_successor()',
we can just check 'keymgr_direct_dep()'.
In 'dns_keymgr_run()', we want to make sure that there is no
dependency on the keys before retiring excess keys, so replace
'keymgr_key_is_successor()' with 'keymgr_dep()'.
