Skip to content
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

Long / MultiString TXT records fail with deSEC #996

Closed
jnaskali opened this issue Dec 16, 2020 · 28 comments · Fixed by #1204
Closed

Long / MultiString TXT records fail with deSEC #996

jnaskali opened this issue Dec 16, 2020 · 28 comments · Fixed by #1204
Milestone

Comments

@jnaskali
Copy link

deSEC provider doesn't allow TXT records with multiple strings, but the service splits long records automatically.

After #947 validation changes, dnscontrol fails validation for long TXT records with ERROR: txt target >255 bytes and AUTOSPLIT not set. Adding AUTOSPLIT to deSEC records (or splitting manually) causes dnscontrol to fail with ERROR: TXT records with multiple strings not supported by desec

Before, long TXT records worked, though the automatically split record caused unnecessary modifications.

@tlimoncelli
Copy link
Contributor

I was afraid that might happen. I don't have a test account on dSEC, so I apologize for the problem this is causing you.

My suggestion is that the driver should re-assemble the individual strings before sending it to deSEC (perhaps only if AUTOSPLIT was set?). I don't think any systems that use TXT records care where the splitting happened.

At the same time, could someone reach out to deSEC and ask them to support multiple strings?

My concern is that there might be a system that requires long TXT records to be split a certain way... perhaps only splitting on spaces.

CC: @D3luxee

@xbra
Copy link

xbra commented Dec 18, 2020

I was afraid that might happen. I don't have a test account on dSEC, so I apologize for the problem this is causing you.

Hi @tlimoncelli, you are free to join https://desec.io since it’s completely free and run by a German non-profit organization. So at least, money is not a problem. ;-)

At the same time, could someone reach out to deSEC and ask them to support multiple strings?

I am not a DNS-pro, but to my mind, they are supporting multiple strings, see the documentation and value1 / value2:

TXT record

The contents of the TXT record must be enclosed in double quotes. Thus, when POSTing to the API, make sure to do proper escaping etc. as required by the client you are using. Here’s an example of how to create a TXT RRset:

    curl -X POST https://desec.io/api/v1/domains/{name}/rrsets/ \
        --header "Authorization: Token {token}" \
        --header "Content-Type: application/json" --data @- <<< \
        '{"type": "TXT", "records": ["\"test value1\"","\"value2\""], "ttl": 3600}'

My concern is that there might be a system that requires long TXT records to be split a certain way... perhaps only splitting on spaces.

I don’t think so. I am using both dnscontrol (btw: Thanks a lot, great tool) and deSEC and the string is split somewhere in the middle automatically, see:

$ dig txt mail._domainkey.secret.domain +short
"v=DKIM1; k=rsa; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAgMNryBTnQaCBAvdbfeyslPJ0wdDNTZNLUxQ5YaSCIz8U+75LATZWTiJQm5Pa/qnMHgbK14GnM3dOZTgrPsLZyEnKCoKZ4/jMTgJWsZo+0Q4aEwwjKTvWM2Q+DdMVtIFo4hbgwF3W31FvFDkDJJLx7vYh80zdXKju4bCVpNRYhECpS57ZfJLQQM2WbTZgUduug" "wkPRqT0qjnOZhsEQkIud5dbikrkOThsbKxPAA64WbLPnCzRnKYtmRklEPMixqXcnMkbrgb9FSezwRozZY06NMV8EzA+vgLFhXtm2xObaXXTBsK6AACuRvJ1DInb+/u4bGNXXqT7tajflRodfVT5xQIDAQAB"

The first string is exactly 255 chars, the rest is 155 chars long.
I hope, this helps. Thank you once again for your support.

Cheers,
Max

@tlimoncelli
Copy link
Contributor

Can someone with an account on deSEC please test this? #997

If that doesn't work, I included a comment about something else to try:

+                       if r.Type == "TXT" {
+                               // Try this first:
+                               zr.Records = r.TxtStrings
+                               // If that doesn't work, try:
+                               //zr.Records = []string{strings.Join(r.TxtStrings, "")}
+                       }

Hope that helps!

@xbra
Copy link

xbra commented Dec 18, 2020

Hi @tlimoncelli, unfortunately not. With branch tlim_desectxt, I am building dnscontrol-git-3.4.2.r951.2703b2a3 (arch linux) and I still get:

$ dnscontrol preview
2020/12/18 15:00:28 printIR.go:88: 1 Validation errors:
2020/12/18 15:00:28 printIR.go:94: ERROR: TXT records with multiple strings not supported by desec (label="google._domainkey" domain=secret.domain)
exiting due to validation errors

Trying your other solution…

// Try this first:
//zr.Records = r.TxtStrings
// If that doesn't work, try:
zr.Records = []string{strings.Join(r.TxtStrings, "")}

... results in compilation error

$ GO111MODULE=on go run build/build.go -os=linux
2020/12/18 16:13:46 Building dnscontrol-Linux
# github.com/StackExchange/dnscontrol/v3/providers/desec
providers/desec/convert.go:68:27: undefined: strings
2020/12/18 16:13:47 exit status 2
exit status 1

I am absolutely not sure, if I did everything correctly. Sorry. Maybe I can provide a test domain and test config? Would that be helpful for you?

@tlimoncelli
Copy link
Contributor

You're very close!

That compilation error can be fixed by adding "strings" to the import statement at the top. If you have goimports installed, you can run goimports -w providers/desec/convert.go to do that automatically.

@xbra
Copy link

xbra commented Dec 18, 2020

Now it compiles, but I don’t know what config you prefer for DKIM in dnsconfig.js

TXT("google._domainkey", DKIM("v=DKIM1; k=rsa; p=MIIB...")),

results in

2020/12/18 16:36:23 printIR.go:88: 1 Validation errors:
2020/12/18 16:36:23 printIR.go:94: ERROR: TXT records with multiple strings not supported by desec (label="google._domainkey" domain=secret.domain)
exiting due to validation errors

AND

TXT("google._domainkey", "v=DKIM1; k=rsa; p=MIIB..."),

results in

2020/12/18 16:39:29 printIR.go:88: 1 Validation errors:
2020/12/18 16:39:29 printIR.go:94: ERROR: txt target >255 bytes and AUTOSPLIT not set: label="google._domainkey" index=0 len=410 string[:50]="v=DKIM1; k=rsa; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A..."
exiting due to validation errors

@tlimoncelli
Copy link
Contributor

Ah, I forgot to shut off those validation tests. I've updated #997. Give it a try now.

@xbra
Copy link

xbra commented Dec 18, 2020

I guess, we are getting closer. dnscontrol preview succeeds but every TXT wants to be modified now, e.g.

MODIFY TXT secret.domain: ("\"google-site-verification=9Mnh...\"" ttl=3600) -> ("google-site-verification=9Mnh..." ttl=3600)
MODIFY TXT google._domainkey.secret.domain: ("\"v=DKIM1; k=rsa; p=MIIB...7ji" "M\" \"RUx9...AQAB\"" ttl=3600) -> ("v=DKIM1; k=rsa; p=MIIB...7jiM" "RUx9...AQAB" ttl=3600)

To my mind, the provider is missing the correct string syntax with additional \" – see documentation from above and ["\"test value1\"","\"value2\""]

The full error of dnscontrol push is…

FAILURE! failed create rrset (deSEC): http status 400 400 Bad Request, the api does not provide more information

@tlimoncelli
Copy link
Contributor

@xbra I agree. It looks like convert.go needs to treat TXT specially.

@jamaltebi
Copy link
Contributor

jamaltebi commented Jan 31, 2021

Hi @xbra . I have the same issue as yours in ClouDNS with caracter \" create succeds with join String. but modify not

@xbra
Copy link

xbra commented Feb 3, 2021

Hi @tlimoncelli, how do we continue from here?

@peterthomassen
Copy link
Contributor

peterthomassen commented Mar 3, 2021

I'm a maintainer at deSEC and hope to give some clarification, as this came up on our forum.

  • deSEC does support multiple TXT records in one RRset (just like multiple A records in one RRset).

  • deSEC also supports multiple tokens within on TXT record, for example "first-token" "second-token".

  • If the above is combined, you can send the following as the records value in the JSON payload:

    ["\"first-token\" \"second-token\"", "\"some other TXT record\""]
    
  • Each token must be surrounded by quotes ".

    RationaleIn the DNS spec, this is optional if the tokens do not contain spaces. However, it leads to an ugly ambiguity, especially when JSON quoting is added. Consider the payload `["foo bar"]` which corresponds to the TXT record `foo bar`. According to the specs, these are two tokens (`"foo" "bar"`), which might be unexpected. We thus enforce that TXT tokens in the payload are quoted (so you need to say `"foo bar"` or `"foo" "bar"`). As this ends up in a JSON string, it requires JSON quotes around it, so the token quotes need to be escaped.
  • When queried, this results in:

    $ dig +short ...
    "first-token" "second-token"
    "some other TXT record"
  • The deSEC API normalizes all input (canonical representation) before storing. There are various reasons for that, one being that the DNS spec prescribes that record values in an RRset must be unique (and canonicalization simplifies uniqueness checks). For example, if you create AAAA with 0:0::1, it will be stored as ::1.

  • The DNS spec limits the length of TXT tokens to 255 bytes. (The DNS transport protocol cannot transfer longer values, as the token length is represented by one byte (range 0..255), see also here.) Therefore, tokens which are longer than 255 bytes are split into multiple tokens every 255 bytes. This is part of the above normalization procedure, i.e. we store it this way, and do not keep the non-split version. (From a DNS perspective, the fact that the string was not originally split is no real information, as you cannot query it via DNS. We don't allow storing non-DNS information in our API.)

  • As a result of that, if you store a string longer than 255 characters, and later check if it is the same, you will find that not to be the case (similarly with AAAA normalization etc.).

Does that help?

@nils-wisiol

@tlimoncelli
Copy link
Contributor

Wow! So much good information! Thank you for assembling that all in one place.

My first suggestion is to work off the tlim_target_and_txt branch. It has all the pieces needed to make fixing this a lot easier. The related PR is here: #1063 (The goal of that PR is to simplify this exact situation!)

My next suggestion is to add code right after models.PostProcessRecords(existing) (near desecProvider.go line 73) call a function that loops through dc.Records looking for TXT records. Each one needs to be rewritten so that they are 255 octets or shorter. You can simply call txtutil.SplitSingleLongTxt(dc.Records) or write your own function if you prefer. (txtutil.SplitSingleLongTxt only splits up the first token).

After that, the integration tests probably work. If not the tips in the PR mentioned above should help.

Tom

@peterthomassen
Copy link
Contributor

peterthomassen commented Mar 3, 2021

Just to make sure there is no misunderstanding, TXT records at deSEC may be longer than 255 characters; it is the tokens in the records that may not exceed this length, see RFC 1035 Sec. 3.3.14 (the tokens are called "character-string"s there). dnscontrol may have another terminology, though.

Note that each token must be surrounded by quotes ". Rationale: In the DNS spec, this is optional if the tokens do not contain spaces. However, it leads to an ugly ambiguity, especially when JSON quoting is added. Consider the payload ["foo bar"] which corresponds to the TXT record foo bar. According to the specs, these are two tokens ("foo" "bar"), which might be unexpected. We thus enforce that TXT tokens in the payload are quoted (so you need to say "foo bar" or "foo" "bar"). As this ends up in a JSON string, it requires JSON quotes around it, so the token quotes need to be escaped.

@tlimoncelli tlimoncelli added this to the v3.8.0 milestone Mar 7, 2021
@tlimoncelli
Copy link
Contributor

Please try running an integration test and see if it works as expected.

@xbra
Copy link

xbra commented Mar 9, 2021

Hi @tlimoncelli, unfortunately the escaping of strings is done wrong and pushing TXT records fails. In preview mode it tries to
MODIFY TXT _dmarc.***: ("\"v=DMARC1; […] fo=1\"" ttl=3600) -> ("v=DMARC1; […] fo=1" ttl=3600)
Which later fails with
FAILURE! failed create rrset (deSEC): http status 400 400 Bad Request, the api does not provide more information

@peterthomassen
Copy link
Contributor

FAILURE! failed create rrset (deSEC): http status 400 400 Bad Request, the api does not provide more information

Just as an aside: When we send a 400 response, we include more information in the response body about what exactly went wrong. (So the claim "the api does not provide more information" is not right. :-) )

@tlimoncelli
Copy link
Contributor

tlimoncelli commented Mar 14, 2021

Does something like #1095 help? (I don't have an account on deSEC so this is just a guess)

@xbra
Copy link

xbra commented Mar 15, 2021

@tlimoncelli – sorry, it’s not working. I never wanted to dive that deep into Go and dnscontrol, but in order to read the error (that is according to @peterthomassen always included in the response body) I cleaned protocol.go:

diff --git a/providers/desec/protocol.go b/providers/desec/protocol.go
index 48440144..cb20b376 100644
--- a/providers/desec/protocol.go
+++ b/providers/desec/protocol.go
@@ -208,12 +208,7 @@ retry:
                        time.Sleep(500 * time.Millisecond)
                        goto retry
                }
-               var errResp errorResponse
-               err = json.Unmarshal(bodyString, &errResp)
-               if err == nil {
-                       return bodyString, fmt.Errorf("http status %d %s details: %s", resp.StatusCode, resp.Status, errResp.Detail)
-               }
-               return bodyString, fmt.Errorf("http status %d %s, the api does not provide more information", resp.StatusCode, resp.Status)
+               return bodyString, fmt.Errorf("http status: (%d) %s\nDetails: %s", resp.StatusCode, resp.Status, bodyString)
        }
        //time.Sleep(334 * time.Millisecond)
        return bodyString, nil

This gives the error message: [{"non_field_errors":["Data for TXT records must be given using quotation marks."]},{"non_field_errors":["Data for TXT records must be given using quotation marks."]}]

So, again, TXT records need to be enclosed in ".

diff --git a/providers/desec/convert.go b/providers/desec/convert.go
index 45859a0a..c42703ca 100644
--- a/providers/desec/convert.go
+++ b/providers/desec/convert.go
@@ -63,7 +63,7 @@ func recordsToNative(rcs []*models.RecordConfig, origin string) []resourceRecord
                                Records: []string{r.GetTargetCombined()},
                        }
                        if r.Type == "TXT" {
-                               zr.Records = []string{r.GetTargetField()}
+                               zr.Records = []string{`"` + r.GetTargetField()+ `"`}
                        }
                        zrs = append(zrs, zr)
                        //keys[key] = &zr   // This didn't work.

And this, finally, succeeds in my case for DKIM records written in that way in dnsconfig.js

TXT("google._domainkey", "v=DKIM1; k=rsa; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAgMNryBTnQaCBAvdbfeyslPJ0wdDNTZNLUxQ5YaSCIz8U+75LATZWTiJQm5Pa/qnMHgbK14GnM3dOZTgrPsLZyEnKCoKZ4/jMTgJWsZo+0Q4aEwwjKTvWM2Q+DdMVtIFo4hbgwF3W31FvFDkDJJLx7vYh80zdXKju4bCVpNRYhECpS57ZfJLQQM2WbTZgUduugwkPRqT0qjnOZhsEQkIud5dbikrkOThsbKxPAA64WbLPnCzRnKYtmRklEPMixqXcnMkbrgb9FSezwRozZY06NMV8EzA+vgLFhXtm2xObaXXTBsK6AACuRvJ1DInb+/u4bGNXXqT7tajflRodfVT5xQIDAQAB")

I guess it’s not well coded, but it shows us the direction.

@tlimoncelli
Copy link
Contributor

I've put your changes into a PR: #1109

If someone would like to improve this, I suggest you start by copying the code from providers/hexonet/records.go (encodeTxt and decodeTxt) as it seems to be similar to what is needed.

@tlimoncelli tlimoncelli modified the milestones: 3.8.0, 3.9.0 Mar 29, 2021
@tmkis2
Copy link

tmkis2 commented Apr 5, 2021

in providers/desec/convert.go remove the unused "strings" from the import in line 7 then this will work exceptionally fine with desec as provider.

@tmkis2
Copy link

tmkis2 commented Apr 8, 2021

I guess, we are getting closer. dnscontrol preview succeeds but every TXT wants to be modified now, e.g.

MODIFY TXT secret.domain: ("\"google-site-verification=9Mnh...\"" ttl=3600) -> ("google-site-verification=9Mnh..." ttl=3600)
MODIFY TXT google._domainkey.secret.domain: ("\"v=DKIM1; k=rsa; p=MIIB...7ji" "M\" \"RUx9...AQAB\"" ttl=3600) -> ("v=DKIM1; k=rsa; p=MIIB...7jiM" "RUx9...AQAB" ttl=3600)

This is the last thing that is bugging me with the desec provider.

The full error of dnscontrol push is…
FAILURE! failed create rrset (deSEC): http status 400 400 Bad Request, the api does not provide more information

This was fixed easily and is already mentioned below in a PR

Any idea how to fix the "" problem with TXT record checks?

@tlimoncelli
Copy link
Contributor

It looks like the first and last quote aren't handled right. In it might be providers/desec/protocol.go's upsertRR function not encoding things right, or providers/desec/convert.go near line 64 where it converts the record.

netgate-git-updates pushed a commit to pfsense/FreeBSD-ports that referenced this issue Apr 23, 2021
Releases since 3.7.0 have a bug that affects many deSEC users with long
or multistring TXT records:
StackExchange/dnscontrol#996
@IzumiSenaSora
Copy link

IzumiSenaSora commented Jun 20, 2021

Aha! So it was DNSControl Issue! I though its was deSEC API issue! BTW waiting for fix! Thank You!

@PartTimeDataScientist
Copy link

Any chance that someone looks into this issue in the forseeable future? This seems to be very close to a fix but like with NS1 the listed "maintainer" @D3luxee does not seem to maintain the implementation any longer. At least he didn't participate in the discussion on this issue which is now >6 months old. (And this is not to be understood as accusation but more an observation!)

Not being able to manage TXT records with deSEC renders this highly interesting DNS provider close to unsusable unfortunately...

@tlimoncelli
Copy link
Contributor

Sadly the community-supported providers need the support of the community to survive. As the maintainer of DNSControl, I can't take on the responsibility of maintaining them. However this doesn't mean the owner of a provider should fix every bug. Everyone can step up. The code is very easy to read and we have a test suite that makes it easy to be sure you haven't broken anything. We have a number of major features that were written by first-time Go users.

I'm willing to answer questions and coach anyone interested.

@D3luxee
Copy link
Contributor

D3luxee commented Jul 3, 2021

Any chance that someone looks into this issue in the forseeable future? This seems to be very close to a fix but like with NS1 the listed "maintainer" @D3luxee does not seem to maintain the implementation any longer. At least he didn't participate in the discussion on this issue which is now >6 months old. (And this is not to be understood as accusation but more an observation!)

Not being able to manage TXT records with deSEC renders this highly interesting DNS provider close to unsusable unfortunately...

I will take a look into this in the next days, i was very busy over the last months and had no time left. Now i got vaccation and can afford to spend some time on this.

D3luxee added a commit to D3luxee/dnscontrol that referenced this issue Jul 7, 2021
relates to StackExchange#996 where we had insufficient error output due to unknown api error format
D3luxee added a commit to D3luxee/dnscontrol that referenced this issue Jul 7, 2021
@D3luxee
Copy link
Contributor

D3luxee commented Jul 7, 2021

I think its working now the provider can create, update and delete long / multistring txt records now.
Please build and test the long_txt branch here: https://github.com/D3luxee/dnscontrol/tree/long_txt

tlimoncelli added a commit that referenced this issue Jul 8, 2021
* use /auth/account endpoint for token validation
this implements the token validation using the /auth/account api endpoint as suggested in #1177 instead of fetching the domain list

* deSEC: add support for long txt records #996

* deSEC: add support for a different api error response
relates to #996 where we had insufficient error output due to unknown api error format

* deSEC: remove unused fetchDomainList function

* deSEC: improve error handling

* deSEC: support for long / multistring txt records
the previous commit was broken this is now working (CRUD)

* deSEC: document what desecProvider.domainIndex is used for

* deSEC: handle the rate limiting correctly
we try to use the Retry-After header to determine how long we should sleep until retry

* deSEC: further improvement of rate limit handling
we cut off if the Retry-After header exceeds 3 minutes because this might be the daily limit.

Co-authored-by: Tom Limoncelli <tlimoncelli@stackoverflow.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants