Subject: [IPsec] Benjamin Kaduk's Yes on draft-ietf-ipsecme-split-dns-14: (with COMMENT)

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 15 Nov 2018 17:17:28 -0800

Benjamin Kaduk has entered the following ballot position for
draft-ietf-ipsecme-split-dns-14: Yes

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-ipsecme-split-dns/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I am balloting YES because I think this mechanism has significant value, but I do also
have some substantial comments that will likely result in changes to the draft.
(I also have a number of nit-level comments.)

Section 1

nit: the Abstract and Introduction are supposed to stand on their own, so
starting off with "Split DNS is a common configuration" before defining it
makes the reader work a bit harder than perhaps they need to.

Section 2

                                                       When Split DNS
   has been negotiated, the existing DNS server configuration attributes
   will be interpreted as internal DNS servers that can resolve
   hostnames within the internal domains.

nit: maybe add a word or two to emphasize that the existing attributes are
also IKEv2 Configuration Payload Attributes?

Section 2.1

                         If an INTERNAL_DNS_DOMAIN attribute is included
   in the CFG_REQUEST, the initiator MUST also include one or more
   INTERNAL_IP4_DNS and INTERNAL_IP6_DNS attributes in the CFG_REQUEST.

nit: I think I could parse this as saying that I need one or more IP4 and also
one or more IP6 attributes (i.e., precluding v4- or v6-only
configurations), which I assume is not the intent.  Maybe "or" or "and/or"
would be better than "and"?

   To indicate support for DNSSEC, an initiator includes one or more
   INTERNAL_DNSSEC_TA attributes as defined in Section 3 as part of the
   CFG_REQUEST payload. [...]

nit: There are perhaps philosophical arguments to have about whether this
indicates support for DNSSEC as a whole, or just for receiving trust
anchors for DNSSEC from the responder [for the split-DNS domain(s)].

Section 2.2

   supported by the server, unless the initiator has been configured
   with local polict to define a set of Split DNS domains to use by
   default.

typo: "policy"

We have to use DNS presentation format for the DS records and not wire
format?

Section 2.4

Please consider using IPv6 examples, per
https://www.iab.org/2016/11/07/iab-statement-on-ipv6/ .

Section 3.1

   o  Domain Name (0 or more octets) - A Fully Qualified Domain Name
      used for Split DNS rules, such as "example.com", in DNS
      presentation format and optionally using IDNA [RFC5890] for
      Internationalized Domain Names.  Implementors need to be careful
      that this value is not null-terminated.

W.r.t. IDNA, is this an A-label or a U-label?
W.r.t NUL-termination, do they need to not send the zero byte or not expect
to receive one?

Section 3.2

                                                    Any
   INTERNAL_DNSSEC_TA attribute that is not immediately preceded by an
   INTERNAL_DNS_DOMAIN or another INTERNAL_DNSSEC_TA attribute applying
   to the same domain name MUST be ignored and treated as a protocol
   error.

Isn't "ignored" incompatible with "treated as a protocol error"?

Section 4

   If a client is configured by local policy to only accept a limited
   number of INTERNAL_DNS_DOMAIN values, the client MUST ignore any
   other INTERNAL_DNS_DOMAIN values.

Is this a limited *number* or a limited *set*?  (If the former, exposition
about which ones are the "other" ones is probably in order.)

   private remote network using the IPsec connection.  If all traffic is
   routed over the IPsec connection, the existing global
   INTERNAL_IP4_DNS and INTERNAL_IP6_DNS can be used without creating
   specific DNS exemptions.

DNS or DNSSEC exemptions (i.e., both)?

Section 5

   DNS records can be used to publish specific records containing trust
   anchors for applications.  The most common record type is the TLSA
   record specified in [RFC6698].  This DNS record type publishes which
   CA certificate or EE certificate to expect for a certain host name.
   These records are protected by DNSSEC and thus can be trusted by the
   application.  [...]

I'm probably thinking too much about "trust" as a concept (due to the RATS
BoF), but especially given that this goes on to say that this is a local
policy decision, I'd suggest replacing "can be trusted" with something like
"has a trust path in" or "are trustable by".  Merely being in DNS with
valid DNSSEC signatures does not make the client trust it; that's still a
local policy decision.

                 It allows the remote IKE/IPsec server to modify DNS
   answers including its DNSSEC cryptographic signatures by overriding
   existing DNS information with trust anchor conveyed via IKE and
   (temporarilly) installed on the IKE client. [...]

nits: "including DNSSEC cryptographic signatures" (no "its"); "trust
anchor(s)"

   IKE clients willing to accept INTERNAL_DNSSEC_TA attributes MUST use
   a whitelist of one or more domains that can be updated out of band.
   IKE clients with an empty whitelist MUST NOT use any
   INTERNAL_DNSSEC_TA attributes received over IKE.  Such clients MAY
   interpret receiving an INTERNAL_DNSSEC_TA attribute for a non-
   whitelisted domain as an indication that their local configuration
   may need to be updated out of band.
[...]
   IKE clients MAY interpret an INTERNAL_DNSSEC_TA for domain that was
   not preconfigured as an indication that it needs to update its IKE
   configuration (out of band).  The client MUST NOT use such a
   INTERNAL_DNSSEC_TA to reconfigure its local DNS settings.

These two paragraphs seem essentially redundant (the first seems better to
me).

   IKE clients MUST ignore any received INTERNAL_DNSSEC_TA requests for
   a FDQN for which it did not receive and accept an INTERNAL_DNS_DOMAIN
   Configuration Payload.

nit: are these really "requests"?

Section 6

It's probably appropriate to reiterate here that "As discussed in Section
5, the INTERNAL_DNSSEC_TA mechanism allows the credential used to
authenticate an IKEv2 association to be leveraged into authenticating
credentials for TLS and other connections.  This reflects something of a
privilege escalation, and initiators should ensure that they have
sufficient trust in the communications peer to responsibly use (or not use)
that elevated privilege."

   If the initiator is using DNSSEC validation for a domain in its
   public DNS view, and it requests and receives an INTERNAL_DNS_DOMAIN
   attribute without an INTERNAL_DNSSEC_TA, it will need to reconfigure
   its DNS resolver to allow for an insecure delegation.  It SHOULD NOT
   accept insecure delegations for domains that are DNSSEC signed in the
   public DNS view, for which it has not explicitely requested such
   deletation by specifying the domain specifically using a
   INTERNAL_DNS_DOMAIN(domain) request.

It seems like heeding this SHOULD NOT would potentially require the
initiator to make extra DNS requests from the external view that it would
not otherwise need to make.  I agree that this is the behavior we want to
encourage with respect to not downgrading away from DNSSEC, but it's less
clear to me that we want to encourage the initiator to go out of its way to
check, as opposed to just using information it may happen to have already.

---

From: "Waltermire, David A. (Fed)" <[email-masked]>
Date: Fri, 16 Nov 2018 21:28:54 +0000

Ben,

> We have to use DNS presentation format for the DS records and not wire

The group was "split" on this question. We did a hum, with most responding in the room that they either did not care or had a slight preference for presentation format. This is why it is this way.

Regards,
Dave

---

From: Paul Wouters <[email-masked]>
Date: Sat, 17 Nov 2018 01:15:01 -0500 (EST)

> The group was "split" on this question. We did a hum, with most responding in the room that they either did not care or had a slight preference for presentation format. This is why it is this way.

Note that we did not use humming as a vote. The humming gave an idea of
the room that the disadvantage of IKE software needing to understand and
convert wire to/from presentation format only to prevent "string
overflow errors" was not worth the trouble of forcing a wire format,
especially since IKE just relays this information to another
application, and this API almost certainly uses presentation format
(for example, unbound-control)

Paul

---

From: Tero Kivinen <[email-masked]>
Date: Sun, 18 Nov 2018 13:14:40 +0200

Paul Wouters writes:
> > The group was "split" on this question. We did a hum, with most
> > responding in the room that they either did not care or had a
> > slight preference for presentation format. This is why it is this
> > way. 
> 

Actually the text in section 2.2 is confusing. It is claiming that

   This attribute lists the corresponding internal DNSSEC
   trust anchor in the DNS presentation format of a DS record as
   specified in [RFC4034].

The format we use for DS records is not presentation format, nor is it
wire format.

In section 3.2 we see that we are using Wire format for DNSKEY Key
Tag, DNSKEY Alg and Digest Type, but presentation format for Digest
Data.

I.e., we do split the DNSKEY Key Tag, DNSKEY Alg, and Digest type
fields out from the presentation format and encode them as 16- and
8-bit integers in wire format, but then for the Digest Data, which in
wire format would be just binary blob, we use Presentation format of
the DS, i.e., hexadecimal with all kind of parenthesis, comments,
escaping mechanisms and newlines.

I myself would have been much more happy with wire format also for
Digest Data, but authors wanted to allow things like:

     INTERNAL_DNSSEC_TA(1270,8,1,"( 506AD3A656  ; comments
		D14D9246558 ; in file
		77628FFD6A98D7A847E ) ; more")

Which is then encoded as:

                       1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-----------------------------+-------------------------------+
   |R|   Attribute Type 0x19       |      Length 0x48              |
   +-+-----------------------------+---------------+---------------+
   | DNSKEY Key Tag 0x04  0xf6     |DNSKEY Alg 0x8 |Digest Type 0x1|
   +-------------------------------+---------------+---------------+
   |  0x28 "("        0x20 " "        0x35 "5"        0x30 "0"     |
   |  0x36 "6"        0x41 "A"        0x44 "D"        0x33 "3"     |
   |  0x41 "A"        0x36 "6"        0x35 "5"        0x36 "6"     |
   |  0x20 " "        0x20 " "        0x2B ";"        0x20 " "     |
   |  0x63 "c"        0x6f "o"        0x6d "m"        0x6d "m"     |
   |  0x65 "e"        0x6e "n"        0x74 "t"        0x73 "s"     |
   |  0x0a "\n"       0x09 "\t"       0x09 "\t"       0x44 "D"     |
   |  0x31 "1"        0x34 "4"        0x44 "D"        0x39 "9"     |
   |              ...                                              |
   +---------------------------------------------------------------+

and so on, instead of:

     INTERNAL_DNSSEC_TA(1270,8,1,0x506AD3A656D14D924655877628FFD6A98D7A847E)

with DS wire format encoding for Digest Data would result following
attribute:

                       1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-----------------------------+-------------------------------+
   |R|   Attribute Type 0x19       |      Length 0x1c              |
   +-+-----------------------------+---------------+---------------+
   | DNSKEY Key Tag 0x04  0xf6     |DNSKEY Alg 0x8 |Digest Type 0x1|
   +-------------------------------+---------------+---------------+
   |  0x50            0x6a            0xd3            0xa6         |
   |  0x56            0xd1            0x4d            0x92         |
   |  0x46            0x55            0x87            0x76         |
   |  0x28            0xff            0xd6            0xa9         |
   |  0x8d            0x7a            0x84            0x7e         |
   +---------------------------------------------------------------+
-- 
[email-masked]

---

From: Paul Wouters <[email-masked]>
Date: Mon, 19 Nov 2018 11:59:08 +0700

> The format we use for DS records is not presentation format, nor is it

That's correct. We used some strange compromise text :(

> Digest Data, but authors wanted to allow things like:

The authors would have been happy with full presentation format for both :)

You were the one that raised an issue that strings would be passed 
carelessly and could be abused by peer's to try and get a shell :P

> 		77628FFD6A98D7A847E ) ; more")

I don't remember that I wanted to have comments. If I did, I certainly 
don't care now and I would be happy to move everything back to DNS 
presentation format.

I would not want to use wireformat anywhere, because as I argued in the 
past, the IKE daemon is just passing on this information and for it to 
need to convert its DNS configuration, which is guaranteed to be in DNS 
presentation format, it would need to copy some DNS conversion code or 
link against some DNS library to convert it to wire format. And a 
security check could simply reject all characters outside [A-Za-z0-9\-_\.]*

On the other end, passing the DNS information from IKE daemon to the 
system is most likely happening over a DNS presentation API as well, as 
all API's supporting commandline configurations cannot take DNS wire 
format but take DNS presentation format.

So I think we have two options:

1) Clarify the text that the DS RRtype data is not in either wire or 
presentation format.

2) Change the DS RRtype data to also be fully DNS presentation format.

I have a preference for 2) but I guess the working group in the past 
reached consensus on 1)

Paul

---

From: Tero Kivinen <[email-masked]>
Date: Tue, 20 Nov 2018 12:46:26 +0200

> carelessly and could be abused by peer's to try and get a shell :P

Yes, as I have seen so many times that people pass strings for
configuration directly from the remote end to the configuration file,
either because of it is easy, or by accident (shellshock, all kind of
CGI issues etc).

Using wireformat and forcing IKE library to do one line of code to
convert wireformat to restricted presentation format removes that kind
of issues:

printf("DS %d %d %d %s\n", unpack("nCCH*", $cfg_payload));

On the other hand generating the wire format from the values coming
from configuration is similarly very easy (provided it comes from
normal config file not from dns configuration file):

$cfg_payload = pack("nCCH*", $dnskey_key_tag, $dnskey_alg,
	            $digest_type, $digest_data_in_hex);

On the other hand parsing presentation format is much harder issue... 

> don't care now and I would be happy to move everything back to DNS 

Presentation format includes things like comments, newlines,
whitespace etc.

That is the reason I do not like to have presentation format inside
the binary wire formats like IKE. We do not need to do string parsing,
or regex processing in IKE library, but to properly parse presentation
format you need to do that. 

> link against some DNS library to convert it to wire format.

I do not see any reason why the incoming configuration from IKE is
going to be in the presentation format. I think most people do use
configuration formats like:

modecfgdns1=1.2.3.4
modecfgdns2=8.8.8.8
modecfgdomain1=example.com
modecfgtadnsseckeytag1=1270
modecfgtadnssecalg1=8
modecfgtadigesttype1=1
modecfgtadigestdata1=506AD3A656...

or pseudo presentation format (which really isn't presentation format
as it does not allow comments or newlines inside () etc).

modecfgta1=1270 8 1 506AD3A656...
    
> And a security check could simply reject all characters outside
> [A-Za-z0-9\-_\.]*

For presentation format that is not enough (for example it does not
git rid of comments or newlines and paranthesis), and even then it is
wrong is it does not allow any whitespace. For pseudo simplified
presentation format it is too much, as it allows dash, underline, and
period which are not needed...

> format but take DNS presentation format.

As I said converting wire format to simplified presentation format
(which is still valid presentation format) is trivial. 

> 1) Clarify the text that the DS RRtype data is not in either wire or 

Yes. This is needed. I would actually also would like to have text
explaining can you have comments whitespace, newlines etc inside the
Digest Data or not too, while we are at it. I would expect that most
of the implementations would not necessarely like those and sending
those over the wire is just wasting bytes. 

-- 
[email-masked]

---

From: Michael Richardson <[email-masked]>
Date: Tue, 20 Nov 2018 09:33:27 -0500

> format you need to do that.

+1
For all the reasons you cited.

(Mind, I think split-horizon dns is perpetuation of a bad idea from 1989.
Unrouted IPv6 for internal use has no split-dns needs, so I don't really
care if gateway's will find a way to attack client systems.)

--
Michael Richardson <[email-masked]>, Sandelman Software Works
 -= IPv6 IoT consulting =-

---

From: Paul Wouters <[email-masked]>
Date: Tue, 20 Nov 2018 09:46:08 -0500 (EST)

> For all the reasons you cited.

Parsing wouldn't happen the way Tero envisions this.

IKE software like have something like an option that reads RRTYPEs, so
that a DNS/VPN administrator can do:

dig ds internal.example.com > /etc/ipsec.d/internal.example.com.ds
dig ns internal.example.com > /etc/ipsec.d/internal.example.com.ns

And put that in a cron job in case they do internal DNSSEC with key
rollovers.

with a config line in the IKE config that loads these entries.

conn example.com
 	modecfgdns=example.com
 	modecfgdns-ns=/etc/ipsec.d/internal.example.com.ns
 	modecfgdns-ds=/etc/ipsec.d/internal.example.com.ds
 	[...]

Paul

---

From: Paul Wouters <[email-masked]>
Date: Tue, 20 Nov 2018 10:06:00 -0500 (EST)

> either because of it is easy, or by accident (shellshock, all kind of

That's why we have the Security Considerations:

    The content of INTERNAL_DNS_DOMAIN and INTERNAL_DNSSEC_TA may be
    passed to another (DNS) program for processing.  As with any network
    input, the content SHOULD be considered untrusted and handled
    accordingly.

> convert wireformat to restricted presentation format removes that kind

Sure, but it would add two conversions that shouldn't be needed at all.

> normal config file not from dns configuration file):

Most likely, when automating this, it would be coming from live DNS
output, eg the dig command. And not some new config file syntax for
this. If I wouldnt use files as in my previous email's example, I
would still want to be able to configure it like:

 	modecfgdns-ds="example.com. IN DS 12345 8 1 BLOB"

But pointing to a file with just DNS RRTYPEs is much easier, especially
if you have 20 internal domains, as most universities seem to have :P
Then you don't have to have duplicate keywords and/or ordening issues,
and you wouldn't have large blobs in your config file. It is just much
easier to create and maintain this in dig command output, so DNS
presentation format.

> format you need to do that.

I think you can do this without regexp Tero :)

> modecfgtadigestdata1=506AD3A656...

This would be terrible to generate from live DNS output. Much harder
than "dig ds example.com @internalDNS > dns.txt"

> those over the wire is just wasting bytes.

I'm confused what you are suggesting here. You say it should support the
crazy stuff but then suggest implementations will just not support the
crazy stuff?

>>>       INTERNAL_DNSSEC_TA(1270,8,1,0x506AD3A656D14D924655877628FFD6A98D7A847E)

It would be:

        INTERNAL_DNSSEC_TA("1270 8 1 0x506AD3A656D14D924655877628FFD6A98D7A847E")

Note that here the domain name is not even part of the data. If you were
sending presentation format, it would be nicer:

        INTERNAL_DNSSEC_TA("example.com. IN DS 1270 8 1 0x506AD3A656D14D924655877628FFD6A98D7A847E")

And there would be no ordering constraints for INTERNAL_DNSSEC_TA()
needing to immediately follow INTERNAL_DOMAIN()

Paul

---

From: Michael Richardson <[email-masked]>
Date: Tue, 20 Nov 2018 10:50:40 -0500

> care if gateway's will find a way to attack client systems.)

To be clear: I acknowledge that I'm in the rough.  My comments should not be
intended to be seen as blocking.

--
]               Never tell me the odds!                 | ipv6 mesh networks [
]   Michael Richardson, Sandelman Software Works        | network architect  [
]     [email-masked]  http://www.sandelman.ca/        |   ruby on rails    [