StackExchange / dnscontrol

Infrastructure as code for DNS!
https://dnscontrol.org/
MIT License
3.14k stars 399 forks source link

Multiple strings in RR value (from convertzone) cause parse failure #91

Closed philpennock closed 4 years ago

philpennock commented 7 years ago

Many RR-types which take strings, take N strings, for N >= 1. Eg, the TXT RR-type.

convertzone took a zonefile which had such RR values and made dnsconfig directives which had two sequential strings with no operator between them, which led to a JS parse failure.

In this case, the cause is DKIM keys in TXT records. The below is actual data for the spodhuis.org zone.

I read all the docs I could find before trying the tool and saw nothing describing how to manual multiple strings as part of one RR value. The output from convertzone is not accepted as input into dnscontrol.

% dnscontrol
2017/04/17 20:55:23 main.go:56: Error executing javasscript in (dnsconfig.js): (anonymous): Line 42:280 Unexpected string (and 3 more errors)

% sed -n 42p < dnsconfig.js
    TXT('d201611._domainkey', "v=DKIM1; k=rsa; p=MIG7MA0GCSqGSIb3DQEBAQUAA4GpADCBpQKBnQDJZk8JRPxvefSR/6CWRqgiGQvvtGuFmvIaUuOyVys2TYE61h/8rC6se00NKzN1hU+gJwfw8FKk8l+JhJ/znd9KdIJreu41KzIFIxeWsdYpcbwvTGYzfdbWifqNrgFa4l/D6Nicreh2/hT5aI121167qoZ6fgBRWFZ5vKXJoRHtGoqOX8qCLBBGUDz7ZaujVfI" "KQ1oU26fnLa2etlkCAwEAAQ==", TTL(7200)),
tlimoncelli commented 7 years ago

Hi Philip! Long time no see!

TXT records are worrisome for me. We implemented TXT() to only accept 1 string because we know all the DNS provider APIs support that. Some don't support multiple strings (those scoundrels, huh?). We figured that we'd cross the "multiple strings in TXT records" bridge when we get to it.

We generally try to have the DSL prevent users from doing dumb things. So, we often are more restrictive than the RFCs because we don't let people do things that are legal but rare enough that they are usually typos (see https://stackexchange.github.io/dnscontrol/why-the-dot). For example, you'll notice that TXT records are JavaScript quoted. Our idea is that you should put the string you want as it should appear, and any escaping/etc. should be done behind the scenes.

So, what do you recommend we do with TXT records? For example, should we permit one long string and spit it into 255-or-less segments (what splitting algorithm to use?). If a DNS provider's API only permits 1 string, and the TXT record has >1 string, should we error out, concatenate them, or ???

We're all ears.

philpennock commented 7 years ago

Hi Tom. :)

You can't split yourself, because the behavior of "how to join the strings" varies between application-layer specifications and has to be part of their specification of how they use DNS.

SPF and DKIM specs say to join the strings together with no join character. I forget which spec it is, but there's something which joins the strings together with a single ASCII 0x20 space between each one.

So you can't auto-split without knowing the purpose of the record. Might be a use-case for a library of accessor functions, such as SPF(...) or DKIM(...) which know how to validate the content and can make the call on splitting as a convenience for the user, and encourage folks to use those instead of the raw TXT record.

If the DNS provider's API only permits one string, and the TXT records has >1 string, error out. If you know that's a limitation and can do it without having to talk, during validation, even better. That way, the people using the tool discover "Oh, we can't deploy this zone on this provider, they can't handle it!" and they can figure out what to do. Which might be "explain to account manager why we're about to publicly switch away".

Sunlight has a tendency to get bugs fixed. :)

tlimoncelli commented 7 years ago

Agreed on all points. ie. Don't autosplit, error on N>1 not supported, provide SPF() and DKIM() functions to do validation and splitting. (Your suggestion of SPF()/DKIM() is brilliant!)

I had a hunch that that was the case with these issues but I didn't have the operational experience to back it up. Thanks for the confirmation!

philpennock commented 7 years ago

Oh, some folks still get hung up on "should use the SPF RR-type" despite the IETF having formally deprecated it and saying to only use TXT records instead. Perhaps SPF_TXT and DKIM_TXT might make more sense, to reduce namespace collisions, but keep the distinguishing part of the name at the start, where it's more visible and more likely to be noticed if the plain TXT is used instead.

Also: the test.globnix.net zone is open-transfer from nlns.globnix.net. Feel free to take a look and scream. It's unreasonable for dnscontrol to support generating that zonefile (and in fact it should be working hard to make it impossible to do so) but there are some useful corner-cases to consider therein.

tlimoncelli commented 7 years ago

Yes, SPF_TXT() and DKIM_TXT() are better names.

Can you dump the test.globnix.net zone as a BIND-style zone file and attach it to this issue? I'd like to record it for posterity.

philpennock commented 7 years ago
dig +nomultiline -t axfr test.globnix.net @nlns.globnix.net | egrep -v '(NSEC|RRSIG)'
thorduri commented 7 years ago

Anyone looking into DKIM_TXT/SPF_TXT ?

The current behind the scenes quoting of TXT RRs makes essentially impossible to manage DKIM records with large keys.

Rather then manage per provider implementations of those functions, a literal modifier is possible ?

tlimoncelli commented 7 years ago

Can you give an example of the problem you are seeing?

We have some large DKIM records and handle them like this:

var HUBSPOT_STACKOVERFLOW = 'k=rsa; t=s; p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDPtW5iwpXVPiH5FzJ7Nrl8USzuY9zqqzjE0D1r04xDN6qwziDnmgcFNNfMewVKN2D1O+2J9N14hRprzByFwfQW76yojh54Xu3uSbQ3JP0A7k8o8GutRF8zbFUA8n0ZH2y0cIEjMliXY4W4LwPA7m4q0ObmvSjhd63O9d8z1XkUBwIDAQAB'

...
...

    TXT('smtpapi._domainkey', HUBSPOT_STACKOVERFLOW),
thorduri commented 7 years ago

@tlimoncelli

The length of HUBSPOT_STACKOVERFLOW is less then 255 characters (230 to be exact). The issues here arise when the TXT record crosses 255 characters.

A TXT RR for a 2048 bit RSA key used for DKIM, is around ~480 chars. If you attempt to create a record that long with e.g. the GCLOUD provider in a single addition it will fail, you need to split it, e.g.

"v=DKIM1\; k=rsa\; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnKZogtjOlHoeY8iZ5o5brlPOsj/a2Q9Bopu1kHxlxrdw7tZVL9FzUMngiIYGrl8dbP7Rvk7TLMoxHxVkRZPBtIpsKIab/gOUoPLQVYbrAmzyguHYBwAApi3H/pvjUsK8+XF0dKY17AR96lokAPqvfBaUb+DSx8zNw2hrYWYVqvCtnxHUGEUhT1bTlEZBptH3j" "JdQ/qm/HROSj3WQn55D8lyQMO8hHml0r15sNYSg8nInFmOhl2JmbsFKy+RoMTwbkk0/meRvcEFWLHkr4MSgbnie6OpQvM4Y51+kO6DUVr3rwjrdVO9wpFt+n/hdQ92TNif17RMJtE5AGaQ6BN3yJQIDAQAB\;"

This is currently (AFAICT) not possible, see here.

There is interaction between the JS quoted string and

The (somewhat complex) quoting rules of the DNS protocol will be done for you.

that makes advanced quoting/escaping impossible ;-)

tlimoncelli commented 6 years ago

I'm working on supporting multiple TXT strings. The PR is almost ready. Please take a look at https://github.com/StackExchange/dnscontrol/pull/293 and see if that works for you.

tlimoncelli commented 6 years ago

P.S. I'm curious if you've tried the SPF optimizer: https://stackexchange.github.io/dnscontrol/spf-optimizer

tlimoncelli commented 6 years ago

Ok, here's an update:

DKIM() does the right thing for short and long strings, doesn't get confused by TTL() and optional /meta parameters, etc.

Give it a try and tell me what you think.

igg commented 6 years ago

Using this syntax:

TXT("google._domainkey", DKIM("very long string"))

with GCLOUD results in:

ERROR: TXT records with multiple strings (label google._domainkey domain: example.com) not supported by gcloud

This despite the fact that gcloud does support TXT records with multiple strings. This stackoverfow question seems to indicate that specifying multiple strings may not be easy.

This is probably a problem in the original #294 fix, which doesn't work with gcloud properly.

tlimoncelli commented 6 years ago

A little history: "convertzone" was just a hack I made to make it slightly easier to migrate zones to DNSControl. It is a first pass... that assumes people will do manual editing to complete the conversion.

We'd gladly accept PRs that improve dnscontrol in this area. (especially if it helps more people in the world use DKIM... I hate spam)

tlimoncelli commented 4 years ago

It might work better in this branch Give this branch a try... https://github.com/StackExchange/dnscontrol/pull/613

tlimoncelli commented 4 years ago

FYI: https://github.com/StackExchange/dnscontrol/pull/613 is merged to master.

tlimoncelli commented 4 years ago

@philpennock Does this issue still exist in the new "get-zones" subcommand?

philpennock commented 4 years ago

Using a dnscontrol binary built yesterday:

% cd zones
% ln -s ~/etc/services/DNS/zones.public/db.spodhuis.org spodhuis.org.zone
% cd ..
% dnscontrol get-zones bind BIND spodhuis.org                            
2020/03/02 16:24:49 dnsrr.go:93: rrToRecord: Unimplemented zone record type=DS (field.spodhuis.org. 7200    IN  DS  50664 7 1 8AA19AF49BFBAE7103E3450FB19E7C4B88FA556A)
{FAIL:1}% rm zones/spodhuis.org.zone
% pcregrep -v '\bDS\b' ~/etc/services/DNS/zones.public/db.spodhuis.org > zones/spodhuis.org.zone
% dnscontrol get-zones bind BIND spodhuis.org
  # Unimplemented zone record type=SMIMEA
  # remove, repeat dnscontrol, lather rinse repeat across:
  # Unimplemented zone record type=CERT
  # Unimplemented zone record type=OPENPGPKEY  (was in zonefile as TYPE61)

% dnscontrol get-zones bind BIND spodhuis.org > semi-original
% dnscontrol get-zones -format=dsl -out spodhuis.js bind BIND spodhuis.org
% vi spodhuis.js   # change REG_CHANGEME
% dnscontrol preview --config spodhuis.js

Generated DSL format file:

  1. missing comma at end of DnsProvider line
  2. no quoting or commas in the SOA() parameters
  3. no quoting of hostnames in generated NS lines (not NAMESERVER, but NS for sub-zones)
  4. the SOA field isn't actually accepted, so the DSL spits out an unparsed type
  5. SRV values are quoting all data fields as one string, instead of the weight/prio/port being separate parameters from the type
  6. Similarly for SSHFP
  7. Similarly for CAA but I can't massage into passing parameter validation (and the CAA record tag argument validation failed diagnostic is missing a line number so I don't know which one is causing issues)
  8. Similarlu to 5, 6 for TLSA
  9. 45 validation errors for not liking entries which start with an underscore ... I use CNAMEs for my TLSA records so that I don't need to repeat them, although with dnscontrol one might use a constant instead?

After which, diff -u semi-original tmp-zones/spodhuis.org.zone is pretty close; SOA was changed, CAA records were dropped (because I removed them since I couldn't get them to validate), $ORIGIN was dropped.

The TXT records do round-trip cleanly, so this particular issue does not still exist. :)

tlimoncelli commented 4 years ago

Ah! Great testing!

Would you be ok with that zone file being made into a test case (i.e. added to git)?

philpennock commented 4 years ago

Not that particular one, but I can put together something else if you want. Presumably you don't mean "pathological corner cases", such as the open transfer test.globnix.net of mine, but instead "real world examples using a bunch of different RR types"?

Beware, I'm the crazy person who actually uses RP records in a bunch of zones which don't receive email, pointing people in the right direction.

tlimoncelli commented 4 years ago

Yeah, not the pathological one, but some reasonable records :-)

tlimoncelli commented 4 years ago

Fixed by 4edf3608540ada5f993f04b59aa0227bcd5352c3