Add DoT support for DNS updates #7678

thalman · 2024-11-01T14:27:50Z

DNS-over-TLS is a new standard for encrypting DNS traffic.

SSSD does not implement the DoT itself but relies on other components of the system. This modification allows as to set a DoT for dynamic DNS updates

:config: New set of options dyndns_dot, dyndns_dot_cacert,
dyndns_dot_cert and dyndns_dot_key allows to enable
DNS-over-TLS for DNS updates.

:relnote: The DoT for dynamic DNS updates is supported now.
It requires new version of nsupdate from BIND 9.20.3+

pbrezina · 2024-11-04T13:12:26Z

Hi, this looks pretty straight-forwards. Code looks good, I will test it.

I will also try to see if we can get dyndns tests into PR CI, but this one might be tricky.

alexey-tikhonov · 2024-11-04T13:20:53Z

It requires new version of nsupdate from BIND 9.20.3+

Is this tested in runtime?

abbra · 2024-11-04T13:39:15Z

src/man/sssd-ad.5.xml

+                </varlistentry>
+
+                <varlistentry>
+                    <term>dyndns_dot_cacert (string)</term>


Will it be possible to specify a directory? OpenSSL in CentOS Stream 10 and Fedora 41+ is switching to default not using a single file CA bundle due to performance issues, so we should expect people wanting to use a directory of certs as well for custom bundles.

No, I don't see any option in nsupdate. https://bind9.readthedocs.io/en/v9.20.1/manpages.html#id126 It all expects a file.

May be we should ask for that?

Could you open a ticket at https://gitlab.isc.org/isc-projects/bind9? You have more insights about it than I do.

pbrezina · 2024-11-07T14:43:19Z

@thalman Could you add a debug message that would show the nsupdate command line that is executed? Currently, it only shows the data we send.

pbrezina · 2024-11-07T15:26:13Z

This PR does not work as is at the moment, it is necessary to specify -H, server and port, i.e.:

nsupdate -g -S -H master.ipa.test

realm IPA.TEST
server master.ipa.test 853
...

Otherwise it tries to lookup the server using SOA record over port 53, which works. But then it continues and try to establish TLS on port 53 which timeouts... So right now, we need to explicitly set the hostname, server and port. However, documentation says that this should not be needed, so there's probably a bug in nsupdate. Opened: https://gitlab.isc.org/isc-projects/bind9/-/issues/5028

There is dyndns_server that we can use, but we should be able to parse it correctly:

master.ipa.test -> -H master.ipa.test, server master.ipa.test 853
master.ipa.test:555 -> -H master.ipa.test, server master.ipa.test 555
172.16.100.10:555#master.ipa.test -> -H master.ipa.test, server 172.16.100.10 555

pbrezina · 2024-11-07T15:33:22Z

To test this, you can install https://copr.fedorainfracloud.org/coprs/antorres/freeipa-unbound-dns/ on ipa container:

dnf copr enable antorres/freeipa-unbound-dns -y
dnf upgrade freeipa\* -y
ipa-dns-install --dns-over-tls --dot-forwared 1.1.1.1#one.one.one.one

# allow outside requests to 53 non encrypted
sed -i "s/listen-on { 127.0.0.1; };/listen-on { any; };/" /etc/named.conf
systemctl restart named

on client container, renamed it so we have a dns name managed by ipa server:

kinit [email protected]
ipa-join --hostname client.ipa.test

cat /etc/sssd.conf
[sssd]
config_file_version = 2
services = nss, pam
domains = ipa.test

[domain/ipa.test]
id_provider = ipa
access_provider = ipa
ipa_server = _srv_
ipa_domain = ipa.test
ipa_hostname = client.test
use_fully_qualified_names = true
debug_level = 0xfff0
dyndns_update = true
#dyndns_dot = true
ipa_hostname = client.ipa.test

thalman · 2024-11-18T13:06:50Z

The code has been changed according to our discussion. dyndns_dot has been removed and I check dyndns_server instead. The dyndns_server option can be an URI. Scheme dns+tls activates DoT support.

thalman · 2024-11-22T12:31:09Z

We should include #7712 first and rebase to fit the changes.

alexey-tikhonov · 2024-11-22T13:19:42Z

We should include #7712 first and rebase to fit the changes.

@thalman, a mistype? FWIW, #7712 was merged.

thalman · 2024-11-22T18:15:08Z

We should include #7712 first and rebase to fit the changes.

@thalman, a mistype? FWIW, #7712 was merged.

Sorry, my mistake - #7713 is the right one

pbrezina

Haven't tried it yet, but I did my first read through. See comments inside.

src/man/sssd-ad.5.xml

src/providers/be_dyndns.c

src/util/util.c

pbrezina

The patch kind of works, but it still has issues.

First, I had to apply #7727 otherwise the arguments to nsupdate where added in reverse order nsupdate master.ipa.test -H ... which made nsupdate fail on the hostname.

Additionally, dns+tls should imply port 853 if other port is not set. Otherwise nsupdate will attempt to talk on port 53 which will fail. Until https://gitlab.isc.org/isc-projects/bind9/-/issues/5028 is fixed and backported, we need to workaround it.

And also see my previous comments.

I have prepared containers where you can test it:

services:
  client:
    image: ${REGISTRY}/dyndns-dot-client:f41
  ipa:
    image: quay.io/sssd/dyndns-dot-ipa:f41

You can add fake IP to the client:

sudo ip addr add 192.168.100.41 dev eth0

src/providers/be_dyndns.c

src/util/util.c

src/util/util.h

src/providers/be_dyndns.c

src/util/child_common.c

src/providers/be_dyndns.c

pbrezina · 2024-12-03T09:35:01Z

dyndns-tests fails / upd fixed already, github did not refresh page properly

pbrezina

Works as expected, please, see little change inside.

src/providers/be_dyndns.c

src/man/sssd-ad.5.xml

DNS-over-TLS is a new standard for encrypting DNS traffic. SSSD does not implement the DoT itself but relies on other components of the system. This modification allows as to set a DoT for dynamic DNS updates :config: the `dyndns_server` option is extended so it can be in form of URI (dns+tls://1.2.3.4:853#servername). New set of options `dyndns_dot_cacert`, `dyndns_dot_cert` and `dyndns_dot_key` allows to configure DNS-over-TLS communication. :relnote: The DoT for dynamic DNS updates is supported now. It requires new version of `nsupdate` from BIND 9.19+.

justin-stephenson

Ack, dynamic DNS update works using dyndns-dot-client and dyndns-dot-ipa containers with dyndns_server = dns+tls://172.16.100.10#master.ipa.test

Not necessary for this PR but IMO it would be nice to allow users to set dyndns_auth = DoT which will be detected as enabling DNS over DoT without the need to specify a URI (or I guess dyndns_auth = DoT, GSS-TSIG if both are used).

justin-stephenson · 2024-12-03T16:39:00Z

CI failure Static code analysis / python-system-tests (pull_request) Failing after 56s is unrelated to this PR.

pbrezina · 2024-12-03T17:13:45Z

Ack, dynamic DNS update works using dyndns-dot-client and dyndns-dot-ipa containers with dyndns_server = dns+tls://172.16.100.10#master.ipa.test

Not necessary for this PR but IMO it would be nice to allow users to set dyndns_auth = DoT which will be detected as enabling DNS over DoT without the need to specify a URI (or I guess dyndns_auth = DoT, GSS-TSIG if both are used).

This was the original idea, unfortunately, nsupdate is buggy at the moment. See: https://gitlab.isc.org/isc-projects/bind9/-/issues/5028

pbrezina · 2024-12-04T11:04:13Z

Pushed PR: #7678

master
- fe26a93 - Add DoT support for DNS updates
sssd-2-10
- c228b79 - Add DoT support for DNS updates
sssd-2-9
- 255e7a7 - Add DoT support for DNS updates

thalman requested a review from pbrezina November 1, 2024 14:28

pbrezina self-assigned this Nov 1, 2024

abbra reviewed Nov 4, 2024

View reviewed changes

thalman force-pushed the dot_nsupdate branch from 2010bd9 to c019ad0 Compare November 15, 2024 16:30

alexey-tikhonov added branch: sssd-2-9 branch: sssd-2-10 labels Nov 18, 2024

thalman force-pushed the dot_nsupdate branch from c019ad0 to 40c22e4 Compare November 22, 2024 11:10

thalman added the Blocked label Nov 22, 2024

thalman force-pushed the dot_nsupdate branch 2 times, most recently from 5090c4e to f292ec5 Compare November 27, 2024 13:00

pbrezina requested changes Nov 27, 2024

View reviewed changes

src/man/sssd-ad.5.xml Show resolved Hide resolved

src/providers/be_dyndns.c Outdated Show resolved Hide resolved

src/providers/be_dyndns.c Outdated Show resolved Hide resolved

src/util/util.c Outdated Show resolved Hide resolved

thalman force-pushed the dot_nsupdate branch 2 times, most recently from 8487350 to d9b40dc Compare November 28, 2024 10:24

pbrezina mentioned this pull request Nov 28, 2024

child: add extra args in correct order #7727

Draft

pbrezina requested changes Nov 28, 2024

View reviewed changes