Make sure it puts quotes around SPF and TXT records

[andylobel]

I'm going to go by trying to import it into AWS Route 53 and other other examples i've seen, as I'm no expert on DNS Zones.

If there aren't quotes around these values then Route 53 fails to import them properly, and it comes out like the following, which is of course totally not going to work.

"v=spf1" "+a" "+mx" "include:_spf.google.com" "~all"

The same goes for DKIM TXT records.

[conatus]

I think the best approach here would be to detect a space in the TXT record, then on output add "around it.

It should also automatically split DKIM TXT records greater than 255 characters into chunks - I have personally run into this issue.

[elgs]

Can you please give me an example? I just had a quick test, and it seems the TXT records are enclosed by a pair of quotes.

[conatus]

@elgs

The issue for me is more that DKIM TXT records greater than 255 characters should be split into chunks.

I stumbled across this issue today from all the way back in 2019! And had a panic for a few moments when I used this and forgot about this.

To give an example:

"txt": [
    {
      "name": "soverin._domainkey",
      "txt": "v=DKIM1; k=rsa; p=<REALLY LONG STRING THAT IS AN RSA KEY>",
      "ttl": 3600
    }
  ],

soverin._domainkey	3600	IN	TXT	v=DKIM1; k=rsa; p=<REALLY LONG STRING THAT IS AN RSA KEY>

Expected:

soverin._domainkey	3600	IN	TXT	("v=DKIM1; k=rsa; <RSA CHUNK>"
 						"<RSA CHUNK>")

[elgs]

@conatus do you happen to know any specification, like RFC about the split of the long rsa key part? Or is it just an implementation's special behavior?

[conatus]

@elgs

I don't think this is a RFC thing, but is a relatively common use pattern as this use of DKIM keys to verify emails is very common.

https://serverfault.com/questions/255580/how-do-i-enter-a-strong-long-dkim-key-into-dns

Sufficiently common that there is a tool to do it. Google's Cloud DNS is one sevice that requires this.

https://www.mailhardener.com/tools/dns-record-splitter

[conatus]

@elgs

Did a bit more of a look and it is a component of RFC4408.

https://tools.ietf.org/html/rfc4408#section-3.1.3

As defined in [RFC1035] sections 3.3.14 and 3.3, a single text DNS
record (either TXT or SPF RR types) can be composed of more than one
string. If a published record contains multiple strings, then the
record MUST be treated as if those strings are concatenated together
without adding spaces.

[elgs]

@conatus thank you. I will look into it.

[conatus]

Thanks @elgs!

[elgs]

soverin._domainkey	3600	IN	TXT	v=DKIM1; k=rsa; p=<REALLY LONG STRING THAT IS AN RSA KEY>

Expected:

soverin._domainkey	3600	IN	TXT	("v=DKIM1; k=rsa; <RSA CHUNK>"
 						"<RSA CHUNK>")

@conatus, I want to confirm one thing. In the expected section, I am not seeing the p=. Should it be there? Or maybe the p= is part of the first <RSA CHUNK>.

[elgs]

I just released 0.0.27, and hopefully it fixed this issue.

Shame on me. It took almost 4 years to fix it.

[conatus]

Hey @elgs - this is my error in the example. p= should be in the expected example. As you say it is the part of the first <RSA CHUNK>.

The corrected expected is:

soverin._domainkey	3600	IN	TXT	("v=DKIM1; k=rsa; p=<RSA CHUNK>"
 						"<RSA CHUNK>")

No shame on anyone, really appreciate you fixing it. :)

Will check out the code then loop back to check this issue can be closed.

[conatus]

Sorry for the delay in testing this. This is still an outstanding issue.

Even in 0.2.8, the RSA key is not split into chunks. The result of this is that we have to manually remember this library causes this error and correct it, otherwise our email deliverability is effected.

If you could take another look, that would be wonderful.

[elgs]

I split the TXT or SPR in the parser, which is a mistake, I should have done them in the generator.

Now I have a new concern, splitting and adding quotes will make the parsing and generating processes not idempotent. This seems to be an issue, too. What do you think? @conatus

[elgs]

Somehow I want to remove the split and adding quote logic from this library and let the clients take care of these logic. So it will be idempotent. Rule of thumb: I am not touching your string.

Please let me know your thoughts.

[conatus]

@elgs As it is part of the standard, maybe we should add a some sort of switch to the JSON configuration file so this chante can be highlighted.

"txt": [
    {
      "name": "soverin._domainkey",
      "txt": "v=DKIM1; k=rsa; p=<REALLY LONG STRING THAT IS AN RSA KEY>",
      "ttl": 3600
      "splitRecord": true
    }
  ],

If you can point me to the point in the code where this all happens I'll probably be more able to take a view.

Alternatively, txt could take an array. As I said above this is a legitimate element of RFC 4408, so seems important to permit somehow.

[elgs]

After understanding the nature of this issue, I prefer to leave the entire txt string to the clients to manage, for 2 reasons:

1. Idempotency, this lib has two main apis, parse and generate, I want calling parse and generate to give back the same input to the parse;
2. The clients can handle the txt array like "\"string 0\" \"string 1\"";

I understand the boundary of the obligation to handle the txt string may not appear to be clear. The Idempotency is the main reason of my preference.

[conatus]

@elgs

Are you suggesting we split the key manually ourselves within the JSON file using escaping?

This does seem like an okay workaround that gets us moving. But the fact these strings can be split is in the RFC specification and right now this library does not support this. As this is an undocumented limitation of this library, it could lead quite a few people to trip over this problem, as we have done. The result for us was lots of emails going to Spam. Which is pretty large problem for us any anyone else who has to do DKIM1 keys - which is most people who want secure email.

As an alternative, how about this?

"txt": [
    {
      "name": "soverin._domainkey",
      "txt": [ "v=DKIM1; k=rsa;", "p=<REALLY LONG STRING THAT IS AN RSA KEY>", "<MORE OF THE STRING>", "<YET MORE OF THE STRING>"],
      "ttl": 3600
    }
  ],

This could work idempotently, if this is a design requirement.

cc @chrisdevereux - this is a workaround for the issue we are having with our processing zone files.

[elgs]

@conatus your suggestion looks good, I would adopt the txt array if this would be the start of the project. But there is one problem that is Google Cloud is depending on this project. Last time I broke their build because I renamed a field. Although I am not responsible for their projects, I am sure they would complain when they upgrade their dependencies if we made this change.

Maybe instead of making txt an array, we could keep the txt as is and add a new field called txt_array, and when generating, we will need to choose one and ignore the other if both exist in the json. What do you think?

[janbaykara]

@elgs I like this solution. An alternative would simply be to do an Array.isArray(textRecord) check if it happens to be an array.

Either way a fix for this would be really appreciated!