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

resolve: Support for DNS-over-TLS #8849

Merged
merged 5 commits into from Jun 12, 2018

Conversation

@irtimmer
Contributor

irtimmer commented Apr 27, 2018

This adds support for DNS-over-TLS (RFC 7858), which can be enabled by setting DNSTLS to yes or allow_downgrade (although due the timeout of 10s downgrading can be slow) in resolved.conf, and long lived TCP connections.

Instead of reopening a new TCP connection for every DNS request using TCP, it will first check for already opened stream to the same server. A list of all transactions per stream is kept in a list beside a list containing all DNS packets requested to be written by the transactions. Also a reference to the stream is kept to keep it open until it's closed by the server or it's timed out. Maybe this could later be extended with edns-tcp-keepalive (RFC 7828), but I couldn't quickly find a server supporting this extension. Also connection errors are counted once per stream instead of per transaction.

To support different protocols over which DNS can be provide, the feature level is split into a feature level and protocol level. This makes support for EDNS, DNSSEC independent from the transport protocol (TLS, UDP, TCP).

For supporting TLS this uses GNUTLS which is already used by journald. Currently it won't validate certificates, connect only over port 583 and can only be configured global and not per link. To keep reconnections over TLS fast, it use TCP Fast Open (in a separate commit), TLS False Start and TLS Session Resumption. Although a quick test showed not everything is supported by the current public DNS-over-TLS severs.

For TCP Fast Open instead of using connect() the socket address is passed to the DnsStream, while sendmsg() is used to connect and send data at the same time.

Closes #5671 and #7617

@irtimmer irtimmer force-pushed the irtimmer:feature/dns-over-tls branch from 3188dc1 to 9764dac Apr 28, 2018

@poettering

Excellent work! Thank you for looking into this! This has been on our TODO list for a while, and it's greatly appreciated that you picked this up!

A few notes from a first (supeficial) review

@@ -81,6 +81,9 @@ struct DnsPacket {
/* For support of truncated packets */
DnsPacket *more;

/* For support of packets in queue */
LIST_FIELDS(DnsPacket, packets_in_queue);

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

hmm, I wonder if this is a safe thing to do. So far we considered packets more or less "immutable" after they have been created once, thus encouraging that consumers just add refs if they want to keep them around and don't have to be afraid that they change under their accesses too wildly. Coding using them can do with the packets whatever it wants really, in the current code and in future code and as long as they take a ref they know that the packet is gonna stay pinned and remain ina good state.

However, this property is somewhat affected by this embedded chain, as this means the packets can be used in a specific context only once: you can enqueue it in exactly one DnsStream queue exactly once. If any other code gets ahold of the DnsPacket, then it can't use it in that context really, and if it does by some accident weird things will happen…

Now, in the current code I can't see why one ever would want to enqueue the same DnsPacket in more than one stream, or more than once in the same stream, but I still would prefer if we'd not embedd the linked list structure here, just to keep the code more versatile and generic for future changes.

Hence, instead of embedding a linked list here, I think it would be nice to just use an OrderedSet object for maintaining the queue of packets to write. I figure the code is probably going to be shorter too that way, and a lot more generic, as enqueing the same DnsPacket objects into multiple streams or into the same stream more than once would be fully safe then.

on_stream_complete(s, 0);
return 0;
}
return 0;

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

you can dorp the inner return 0...

if (DNS_PACKET_ID(p) == t->id) {
ct = t;
break;
}

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

you can use hashmap_get(s->manager->dns_transactions, UINT_TO_PTR(DNS_PACKET_ID(p)) to get the transaction for the specified ID directly in O(1), no need to search linearly here

fd = -1;

if (t->server)
s->server = dns_server_ref(t->server);

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

dns_server_ref() returns NULL if you pass in NULL (most our _ref() calls do), hence you can drop the if check here, it's redundant

if (i->server == s)
return i;

return NULL;

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

i think it would make a ton of sense to embedd a ref to a DnsStream in the DnsServer structure (or even a Set object with multiple of them). Since we want to share connections now, it only makes sense if we maintain a per-server stream if we can as part of the server structure. I figure in the long run it might make sense to keep more than one stream per server open, hence maybe make this a Set* and allocate multiple streams per service.

I figure a simple algorithm of "when we want to send a new DnsPacket, look for suitable existing DnsStream objects to the DnsServer, and use the first one with an empty queue. If none has an empty queue allocate a new DnsStream object until a max of 5 streams per server is reached, in which case enqueue into the stream with the shortest queue" or so might be simple enough to implement for now and good for the beginning

If the ref to the DnsStream is embedded in the DnsServer we don't have to search the full list of streams here...

@@ -183,6 +183,36 @@ static int dns_stream_identify(DnsStream *s) {
return 0;
}

static ssize_t dns_stream_writev(DnsStream *s, struct iovec *iov, int iovcnt) {

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

iov can be const, no?

It's a bit weird that writev()'s iovcnt parameter is an "int" btw... it really should be a size_t, since it counts structures in memory. If we define a wrapper like this one we should really fix that, and hide the "int" thing internally

if (r < 0)
return dns_stream_complete(s, -r);
/* only identify after connecting */
if (!s->tfo_salen) {

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

as above, please use if (s->tfo_salen == 0) or so

@@ -341,6 +374,9 @@ DnsStream *dns_stream_unref(DnsStream *s) {
s->manager->n_dns_streams--;
}

if (s->tfo_address)
mfree(s->tfo_address);

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

no mfree() here, as you aren't interested in the return value. Moreover, the if check is redundant, both glibc free() and our mfree() wrapper are fine with a NULL parameter, and become NOPs in that case

@@ -213,14 +213,15 @@ void dns_server_move_back_and_unmark(DnsServer *s) {
}
}

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

hmm, can you elaborate on the reasoning of this commit? I mean, I'd assume that servers that can do DNS-over-TLS can also do EDNS0+DO+LARGE just fine, hence I always assumed we'd add DNS-over-TLS simply as another feature level on top of all the others?

Can you explain what made you split this up?

Note that the feature level the way it was currently implemented implemented a state machine of sorts, and not just a linear level. Specifically, we'd start out at LARGE, and then when encountering too many failures we'd go to DO, then to EDNS0, then to UDP, then to TCP, and then back to UDP and then TCP again and so on. Or in other words:

   LARGE → DO → EDNS0 → UDP → TCP
                         ↑     ↓     
                            ←

And I'd always have assumed it would now become:

   TLS → LARGE → DO → EDNS0 → UDP → TCP
                               ↑     ↓     
                                  ←

Where TLS is supposed to mean TLS-over-TCP, and LARGE, DO, EDN0 all mean extension of plain UDP.

meson.build Outdated
@@ -2801,6 +2814,7 @@ status = [
'symbolic gateway hostnames: @0@'.format(', '.join(gateway_hostnames)),

'default DNSSEC mode: @0@'.format(default_dnssec),
'default DNS-over-TLS mode: @0@'.format(default_dnstls),

This comment has been minimized.

@poettering

poettering Apr 30, 2018

Member

i figure using gnutls for this is fine for now. I have the suspicion now that everybody appears to settle on OpenSSL these days, and we should gradually move over to that.

This comment has been minimized.

@dkg

dkg May 3, 2018

Contributor

i think using gnutls is fine here, fwiw. knot-resolver (one of the popular dns resolvers that implements dns-over-tls) uses gnutls and has no problem with it.

@irtimmer irtimmer force-pushed the irtimmer:feature/dns-over-tls branch from 9764dac to 140aaf5 May 1, 2018

@irtimmer

This comment has been minimized.

Contributor

irtimmer commented May 1, 2018

Thanks for the review. I have tried to incorporate all suggested changes. As I was not completely satisfied with the changes to split up the feature level and it probably needs some further discussion I have excluded it for now from this pull request. Although I still think it's a good idea to split it up as I don't think expecting complete DNSSEC support when using DNS-over-TLS is a valid assumption. RFC 7858 even explicitly state that these two are independent. Also in some edge cases the feature level is currently downgraded (like SERVFAIL for Facebook subdomains) which without these change will a side effect also disable DNS-over-TLS. Some of the code for checking UDP is used also depends on the feature level which now get complicated as it can no longer be assumed that all higher feature levels are UDP.

For the reference to the DnsStream in DnsServer I choose not to use a Set. As TCP connections results in additional resource usage on the server, RFC 7766 state a client MUST take care to minimize the number of concurrent TCP connections and that there SHOULD be no more than one connection for regular queries. Also the connection is now kept open because of the reference in DnsServer.

@poettering

This comment has been minimized.

Member

poettering commented May 2, 2018

Although I still think it's a good idea to split it up as I don't think expecting complete DNSSEC support when using DNS-over-TLS is a valid assumption. RFC 7858 even explicitly state that these two are independent.

Hmm, I see, makes sense. But I'd really like to the feature level to be considered more a state machine, hence we probably should come up with a different state machine logic then for this. maybe:

TLS-DO → LARGE → DO → TLS-PLAIN → EDNS0 → UDP → TCP
                                           ↑     ↓     
                                              ←

Where TLS-DO would be the combination of TLS and DNSSEC, and TLS-PLAIN for would TLS without DNSSEC. If TLS and DNSSEC are on, we'd start with TLS-DO. If only DNSSEC is on, we'd start with LARGE, and if only TLS is on we'd start with TLS-PLAIN. If TLS is off we'd jump from DO directly to EDNS0 of course. Now, there's one major ambiguity: if both TLS and DNSSEC are in "allow-downgrade" mode, do we first give up on TLS or first give up on DNSSEC? With the above we'd first give up on TLS, and consider DNSSEC more important. This translates to preferring client-side authentication over privacy, which I think is a good thing.

if (x->size > y->size)
return 1;

r = memcmp(DNS_PACKET_DATA((DnsPacket*) x), DNS_PACKET_DATA((DnsPacket*) y), x->size);

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

you could shorten this to return memcmp(…) here…

s->n_written = 0;

dns_packet_ref(p);
ordered_set_put(s->write_queue, p);

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

adding an item to a set can fail. You need to check the ordered_set_put() return value hence, and only add a ref to the packet on success

return 0;
}

static int on_dns_stream_packet(DnsTransaction *t, DnsPacket *p) {

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

mhmm, i don't like that we have two functions now named so similar: on_stream_packet() and on_dns_stream_packet. Let's clean the naming up, i.e. properly prefix the two calls with the "class" they belong to: dns_transaction_on_stream_packet() and dns_stream_on_packet()

if (error != 0)
on_transaction_stream_error(t, error);
else if (DNS_PACKET_ID(s->read_packet) == t->id)
r = on_dns_stream_packet(t, s->read_packet);

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

hmm, we need to do something sensible with the return code here. Currently we propagate the first matching transaction, if there is one. If there can only be one, then we really should also have a break here, to clarify that this is the case. If there can be multiple, then we should handle possible failures on some of them in some smart way


/* Take ownership of packet to be able to receive new packets */
p = s->read_packet;
s->read_packet = NULL;

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

we have this nice TAKE_PTR() macro now, which you can use to reduce the two lines above into:

p = TAKE_PTR(s->read_packet);
@@ -182,6 +182,38 @@ static int dns_stream_identify(DnsStream *s) {
return 0;
}

static ssize_t dns_stream_writev(DnsStream *s, struct iovec *iov, size_t iovcnt) {

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

i understand that dns_stream_writev() is supposed to be modelled after writev(), but I'd prefer if we'd not copy it 1:1, but return negative errors as we normally do in our codebase, rather than propagate them via errno...

I figure returning negative error numbers here inside of the ssize_t would be fine, even though error numbers are typically "int"s, but let's keep this simple.

Also, the iov parameter really should be const, even if this means we need to cast the const away internally...

@@ -366,7 +401,7 @@ DnsStream *dns_stream_ref(DnsStream *s) {
return s;
}

int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd) {
int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd, union sockaddr_union *tfo_address) {

This comment has been minimized.

@poettering

poettering May 2, 2018

Member

the new parameter can be "const", right?

@dkg

This comment has been minimized.

Contributor

dkg commented May 3, 2018

this is great work, thanks @irtimmer !

I think it would be great to start by trying to merge the persistent TCP stream and the TCP fast-open patches so that the DNS-over-TLS patch can be evaluated independently.

some thoughts on the interface:

  • you're absolutely right to keep DNSSEC orthogonal to this configuration.
  • the option should be named PrivateDNS=, not DNSTLS= -- this more clearly expresses a straightforward user intent, and allows systemd to make sensible choices about what "private" means in the future. This option should apply both to resolved.conf and to any .network stanza that allows configuration of DNS for a given link. (it's conceivable that the admin has one "untrusted" interface that can't afford to leak DNS queries in the clear, but another interfaces where fallback to cleartext is OK)
  • the option's values should be none, opportunistic, or strict -- in opportunistic mode, before a connection is established, it can try port 853 (TLS) in the background to see whether a session can be established -- while that attempt is happening but isn't yet resolved, it should go ahead and send queries in the clear, to avoid the delay. if session establishment fails, it can continue to send queries in the clear. if a TLS session is successfully established when a query comes in, then it can send queries through the TLS connection.
  • if the initial implementation doesn't do certificate checking, it shouldn't offer strict yet. an implementation that just has opportunistic and none is a great improvement anyway :)
  • i can offer a series that expands the definition of the DNS server from just an IP address (192.0.2.4) to an IP address and hostname (e.g., 192.0.2.4|dns.example.com, though i'm happy to entertain alternate syntax), which would let us do certificate-hostname checking. With that change, then we could enable PrivateDNS=strict, which would not permit queries to go out over an interface in the clear.

RFC 8130 suggests that requiring TLS connections without requiring certificate verification isn't a good idea -- if we want opportunistic crypto, that's fine and definitely better than cleartext. but if we want to require TLS, we need to ensure we're talking to who we expect.

@irtimmer irtimmer force-pushed the irtimmer:feature/dns-over-tls branch 2 times, most recently from 9528e9a to 1000b14 May 4, 2018

@poettering

This comment has been minimized.

Member

poettering commented May 10, 2018

oh, please always add a comment when you force push a new version onto a PR. github sometimes doesn't notify us when this happens on its own, there's something weird there in github, so i didn't see the new version yet.

@irtimmer

This comment has been minimized.

Contributor

irtimmer commented May 10, 2018

In this case I deliberately didn't comment as I'm still have to fix some of the state logic and the error codes coming from GnuTLS

@irtimmer irtimmer force-pushed the irtimmer:feature/dns-over-tls branch from 1000b14 to b1bb9e8 May 14, 2018

@irtimmer

This comment has been minimized.

Contributor

irtimmer commented May 14, 2018

I've again tried to incorporate all requested changes. I've added the TLS-DO and TLS-PLAIN in the suggested order to the server level, although the state machine logic got a little more complicated and should now look like:

TLS-DO  →  TLS-PLAIN
  ↓            ↓
LARGE → DO → EDNS0 → UDP → TCP
                      ↑     ↓
                         ←

Depending on the kind of failure (TCP/TLS error or DNS response error) it is decided whether TLS-DO is downgraded to TLS-PLAIN or LARGE. This ensures we don't retry a TLS connection if the TLS connection fails or failback to plaintext queries in case the TLS server just doesn't support DNSSEC.

I've renamed the option DNSTLS to PrivateDNS and removed the strict option. Additionally I added a commit which add support for setting the PrivateDNS option per link.

@dkg One of the main reasons I haven't yet implemented certificate checking, is because I couldn't figure out a good way to specify the hostname or SPKI pins per server. So I'm really interested in the patches to expand the DNS server definitions.

@ott

This comment has been minimized.

Contributor

ott commented May 22, 2018

As mentioned in #7974, the state machine should not be derived from queries but from separate probe queries. Otherwise not possible to distinguish whether the query or the feature set caused the error in the response. Moreover, the feature set should be periodically reset to cope with load balancing, software upgrades, system load fluctuations and configuration changes but I think we can postpone this until this pull request has been merged.

@poettering

This comment has been minimized.

Member

poettering commented May 22, 2018

Moreover, the feature set should be periodically reset to cope with load balancing, software upgrades, system load fluctuations and configuration changes but I think we can postpone this until this pull request has been merged.

We already reset the feature set after a while, it's called the "grace period", and depending how stable the server is the grace period is picked to be 5min … 6h.

@shibumi

This comment has been minimized.

Contributor

shibumi commented Jun 10, 2018

Any update on this? Really can't wait to see this merged :)

@poettering

This looks really excellent. Just some minor nitpicks. Also, needs a rebase on current git. @irtimmer would be most excellent if you could do the final round quickly, then we should be able to merge it before the upcoming v239 release

And sorry for not reviewing this more timely!

@@ -306,10 +306,11 @@ static int dns_scope_socket(
int family,
const union in_addr_union *address,
DnsServer *server,
uint16_t port) {
uint16_t port,
union sockaddr_union *ret_socket_address) {

This comment has been minimized.

@poettering

poettering Jun 11, 2018

Member

a quick comment somewhere which explains this "delayed connect logic" a bit, i.e. that the caller needs to do the connect on its own later on (and why he would choose to do so) would be nice.

This comment has been minimized.

@irtimmer

irtimmer Jun 11, 2018

Contributor

a comment is added to the non-static method dns_scope_socket_tcp

@@ -386,6 +422,7 @@ int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd) {

s->n_ref = 1;
s->fd = -1;
s->tfo_salen = 0;

This comment has been minimized.

@poettering

poettering Jun 11, 2018

Member

no need to initialize this, as we use new0() above, which initializes this anyway. The only fields that need explicit initialization are the ones those different from zero

@@ -58,6 +58,7 @@ Network.Domains, config_parse_domains,
Network.DNS, config_parse_dns, 0, 0
Network.LLMNR, config_parse_resolve_support, 0, offsetof(Network, llmnr)
Network.MulticastDNS, config_parse_resolve_support, 0, offsetof(Network, mdns)
Network.PrivateDNS, config_parse_private_dns_mode, 0, offsetof(Network, private_dns_mode)

This comment has been minimized.

@poettering

poettering Jun 11, 2018

Member

this needs some brief mention in the man page

@@ -2337,6 +2409,7 @@ static void native_help(void) {
" domain [LINK [DOMAIN...]] Get/set per-interface search domain\n"
" llmnr [LINK [MODE]] Get/set per-interface LLMNR mode\n"
" mdns [LINK [MODE]] Get/set per-interface MulticastDNS mode\n"
" privatedns [LINK [MODE]] Get/set per-interface PrivateDNS mode\n"

This comment has been minimized.

@poettering

poettering Jun 11, 2018

Member

also needs some brief mention in the man page

@@ -23,5 +23,6 @@ Resolve.Domains, config_parse_search_domains, 0,
Resolve.LLMNR, config_parse_resolve_support, 0, offsetof(Manager, llmnr_support)
Resolve.MulticastDNS, config_parse_resolve_support, 0, offsetof(Manager, mdns_support)
Resolve.DNSSEC, config_parse_dnssec_mode, 0, offsetof(Manager, dnssec_mode)
Resolve.PrivateDNS, config_parse_private_dns_mode, 0, offsetof(Manager, private_dns_mode)

This comment has been minimized.

@poettering

poettering Jun 11, 2018

Member

needs a brief description in the man page

irtimmer added some commits Apr 22, 2018

resolved: longlived TCP connections
Keep DNS over TCP connection open until it's closed by the server or after a timeout.
resolved: TCP fast open connections
Add suport for TCP fast open connection to reduce latency for successive DNS request over TCP
resolved: support for DNS-over-TLS
Add support for DNS-over-TLS using GnuTLS. To reduce latency also TLS False Start and TLS session resumption is supported.
resolve: make PrivateDNS configurable per link
Like with DNSSec, make PrivateDNS configurable per link, so you can have trusted and untrusted links.

@irtimmer irtimmer force-pushed the irtimmer:feature/dns-over-tls branch from b1bb9e8 to 96a3ecd Jun 11, 2018

@irtimmer

This comment has been minimized.

Contributor

irtimmer commented Jun 11, 2018

I've fixed the small issues and made an attempt in updating the man pages. But suggestions to improve spelling or grammar are more than welcome

@@ -282,8 +283,8 @@
<listitem><para>Revert the per-interface DNS configuration. If the DNS configuration is reverted all
per-interface DNS setting are reset to their defaults, undoing all effects of <option>dns</option>,
<option>domain</option>, <option>llmnr</option>, <option>mdns</option>, <option>dnssec</option>,
<option>nta=</option>. Note that when a network interface disappears all configuration is lost automatically,
an explicit reverting is not necessary in that case.</para></listitem>
<option>privatends</option>, <option>nta=</option>. Note that when a network interface disappears all

This comment has been minimized.

@poettering

poettering Jun 12, 2018

Member

typo: privatends → privatedns

@poettering

This comment has been minimized.

Member

poettering commented Jun 12, 2018

CI all passed. let's merge.

@poettering poettering merged commit 401e860 into systemd:master Jun 12, 2018

4 checks passed

bionic-amd64 autopkgtest finished (success)
Details
bionic-i386 autopkgtest finished (success)
Details
bionic-s390x autopkgtest finished (success)
Details
semaphoreci The build passed on Semaphore.
Details
@poettering

This comment has been minimized.

Member

poettering commented Jun 12, 2018

See #9277 for a PR with a brief mention of this for the NEWS file for the upcoming v239 release.

@gdamjan

This comment has been minimized.

Contributor

gdamjan commented on man/resolved.conf.xml in 30e59c8 Jun 12, 2018

should this be "Defaults to false."?

This comment has been minimized.

Member

yuwata replied Jun 13, 2018

We accept 0, no, n, false, f, off as the boolean false. So, this may not consistent to the first sentence, but is not wrong.

[PRIVATE_DNS_NO] = "no",
[PRIVATE_DNS_OPPORTUNISTIC] = "opportunistic",
};
DEFINE_STRING_TABLE_LOOKUP_WITH_BOOLEAN(private_dns_mode, PrivateDnsMode, PRIVATE_DNS_OPPORTUNISTIC);

This comment has been minimized.

@yuwata

yuwata Jun 13, 2018

Member

So, setting 'yes' is equivalent to 'opportunistic'. Is it intended?

if (m->private_dns_mode != _PRIVATE_DNS_MODE_INVALID)
return m->private_dns_mode;

return _PRIVATE_DNS_MODE_INVALID;

This comment has been minimized.

@yuwata

yuwata Jun 13, 2018

Member

Should this PRIVATE_DNS_MODE_NO?

yuwata added a commit to yuwata/systemd that referenced this pull request Jun 13, 2018

@keszybz

This comment has been minimized.

Member

keszybz commented Jun 13, 2018

I'm late to the (review) party here. Nevertheless, I think "PrivateDNS=" is a bad name. "DNSTLS=" wasn't pretty, but at least it was informative. "private dns" has the problem that when another mechanism comes along (for example DNS encrypted with dtls or whatever), we'll have a naming problem. "private dns" does not work particularly well in google searches also. Expressing "intent" with low-level settings can be problematic. In particular I don't think this setting should enable e.g. dns-over-dtls if we implement it ever. Hence I think it should remain a low-level setting that describes what it does (DNSTLS= or DNSoverTLS=), and maybe we can add a high-level setting over that later.

keszybz added a commit that referenced this pull request Jun 13, 2018

@poettering

This comment has been minimized.

Member

poettering commented Jun 13, 2018

@keszybz I think I agree. The fact that this PR used the better known and better understood DNS-over-TLS name in the title tells a storry. So yupp DNSOverTLS= or DNSoverTLS= sounds great to me.

@HLFH

This comment has been minimized.

HLFH commented Jun 13, 2018

@poettering @keszybz I would also advocate for DNSoverTLS=

@keszybz

This comment has been minimized.

Member

keszybz commented Jun 13, 2018

I think we have agreement to rename this to DNSoverTLS= or DNSOverTLS. "O" is more consistent with other names we have, but "o" is more symmetrical. Not sure what will look better in the end.

@irtimmer Sorry for yanking you back and forth. Any chance you could prepare a patch to do the rename?

@irtimmer irtimmer deleted the irtimmer:feature/dns-over-tls branch Jun 13, 2018

@shibumi

This comment has been minimized.

Contributor

shibumi commented Jun 13, 2018

@keszybz DNSoverTLS

@mbiebl

This comment has been minimized.

Contributor

mbiebl commented Jun 15, 2018

Would it be possible to make this DNS-over-TLS feature compile time optional?
Atm, this pulls in a dependency on GnuTLS into the main systemd package in Debian

@yuwata

This comment has been minimized.

Member

yuwata commented Jun 15, 2018

I guess meson -Dgnutls=false disables DNS-over-TLS.

@mbiebl

This comment has been minimized.

Contributor

mbiebl commented Jun 15, 2018

We do have -Dgnutls=true as we want gnutls support in systemd-journal-remote (which is splitt off into a separate package in Debian).

@mbiebl

This comment has been minimized.

Contributor

mbiebl commented Jun 15, 2018

Even if I build with -Ddefault-private-dns=no, if gnutls has been detected, the DNS-over-TLS code becomes active.

@keszybz

This comment has been minimized.

Member

keszybz commented Jun 15, 2018

Please open a new issue and mark it for v239.

@evverx

This comment has been minimized.

Member

evverx commented Jul 5, 2018

@irtimmer is there any chance you could take a look at #9511?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment