Initial implementation of client side XoT.#149
Conversation
- Requires TLS 1.3 support - Uses a new `tls-auth` clause in the `request-xfr` pattern to specify the authentication domain name to use.
Co-authored-by: Wouter Wijngaards <wcawijngaards@users.noreply.github.com>
| // TODO: There is a nasty case where the far end is listening on TCP | ||
| // but not TLS. In that case the SSL_do_handshake function will loop, | ||
| // returning SSL_ERROR_WANT_READ for the tcp_timeout (120s). This can | ||
| // be handled various ways, not sure of the best one, need to discuss | ||
| // with Wouter... | ||
| int ret, err; | ||
| while (1) { | ||
| ERR_clear_error(); | ||
| if ((ret = SSL_do_handshake(tp->ssl)) == 1) { | ||
| DEBUG(DEBUG_XFRD, 1, (LOG_INFO, "xfrd: TLS handshake sucessful")); | ||
| break; | ||
| } | ||
| err = SSL_get_error(tp->ssl, ret); | ||
| if (err != SSL_ERROR_WANT_READ && err != SSL_ERROR_WANT_WRITE) { | ||
| log_msg(LOG_ERR, "xfrd: TLS handshake failed for %s to %s with value: %d", | ||
| zone->apex_str, zone->master->ip_address_spec, err); | ||
| close(fd); | ||
| xfrd_set_refresh_now(zone); | ||
| return 0; | ||
| } | ||
| /* else wants to be called again */ | ||
| } |
There was a problem hiding this comment.
So I cannot see a lot wrong with this. You could add if(ret == 0) { close(fd); xfrd_set_refresh_now(zone); return 0; } before the 'else wants to be called again' line. Then it would be fairly similar to other code that does not really have this issue.
There was a problem hiding this comment.
There was a problem hiding this comment.
That is the best fix! Makes the patch unproblematic for merge, in my opinion.
| log_msg(LOG_ERR, "xfrd: incomplete write of tls"); | ||
| break; | ||
| default: | ||
| log_msg(LOG_ERR, "xfrd: generic write problem with tls"); |
There was a problem hiding this comment.
It would be nice to print more details, either from errno, or perhaps the function log_crypto_err() for ssl details, depending on what failed. Perhaps also after other SSL_write and SSL_read and SSL_do_handshake failures.
Re-add close(fd) instead of shutdown
|
Hi @wcawijngaards - the git commit history on this PR became very messy with this so I begrudgingly had to cherry pick commits and force push. I believe it should address all your comments, let me know! |
|
You missed the item from xfrd-tcp.c line 677. The fix commit makes three log_msg calls of type LOG_INFO. That is less then the error level, but it still spams the logs with unused messages during normal operations. For debug and verbosity messages we have higher output levels defined. Please move the log at 1065 until after the EOF check below it. Please move the log at 1016 until after the EOF check below it, it is then fine to use LOG_ERR. At 749 and 827 use something like |
- move 2 log messages to after EOF - clean up some whitespace
…ect error line is reported and confcheck unit test now passes Add 1 zone to confcheck unit text to include XoT
… for a given zone
|
Hello We managed to do some tests of NSD as XoT client We didn't find in the configuration how to set a path for a custom CA, Additionally, we find also that it would be nice if, in the "tls-auth" |
|
@cesarkuroiwa thanks for the feedback! Agreed - those are two features that are missing and it would be good to add them both to support the mTLS mode of authentication specified in the draft for XoT. |
|
If there is interest, I've made a few changes to include some configuration variables and managed to successfully request a XFR from our implemention of a XoT server using mutual TLS authentication by passing a client certificate: |
|
@cesarkuroiwa What are you using for XoT server to authenticate the client? I think it would be nice to include mutual authentication once we have the server side for that too. I'll keep an eye on your fork :) The custom CA is nice, but I prefer to use the same name for the option as we use in unbound, which is |
|
@wtoorop We tested NSD acting as a XoT client against our in-house implementation of a DNS server, which currently act as a hidden master for our zones I believe the |
tls-authclause in therequest-xfrpattern to specify the authentication domain name to use.