V3.16.0: dnscontrol check fail

[riku22]

Hello.

I have updated from dnscontrol v3.15.0.
And I updated creds.json and dnsconfig.js according to the instructions of dnscontrol preview, but I get the following error in dnscontrol check.

2022/05/21 12:02:18 printIR.go:88: 1 Validation errors:
2022/05/21 12:02:18 printIR.go:94: ERROR: Unknown DNS service provider type: "-"
exiting due to validation errors

When I specified the type in NewDnsProvider of dnscontrol.js as before V3.15.0, the check was successful.
Isn't dnscontrol check compatible with the new method?

Best regards.

[tlimoncelli]

Thanks for reporting that.

I fixed something similar a few weeks ago. This is definitely a bug.

Can you help me reproduce the issue locally? Is there a minimal dnsconfig.js (or redacted file) that you can provide?

[riku22]

Hi.

dnsconfig.js is as follows.

var registrar = NewRegistrar("none");
var gcloud = NewDnsProvider("gcloud");

D("example.com", registrar, DnsProvider(gcloud),
A("@", "127.0.0.2")
);

I changed var gcloud = NewDnsProvider("gcloud"); to var gcloud = NewDnsProvider("gcloud", "GCLOUD"); and the check succeeded

Best regards.

[adrian-hoasted]

same issue here.

var DNS_CLOUDFLARE = NewDnsProvider('cloudflare', {'manage_redirects': true});

listFiles: cd: ., user: ./domains/
2022/05/22 04:34:33 printIR.go:88: 4 Validation errors:
2022/05/22 04:34:33 printIR.go:94: ERROR: Unknown DNS service provider type: "-"
2022/05/22 04:34:33 printIR.go:94: ERROR: Unknown DNS service provider type: "-"
2022/05/22 04:34:33 printIR.go:94: ERROR: Unknown DNS service provider type: "-"
2022/05/22 04:34:33 printIR.go:94: ERROR: Unknown DNS service provider type: "-"
exiting due to validation errors

var DNS_CLOUDFLARE = NewDnsProvider('cloudflare', 'CLOUDFLAREAPI', {'manage_redirects': true});

listFiles: cd: ., user: ./domains/
No errors.
valid config
INFO: In dnsconfig.js NewDnsProvider("cloudflare", "CLOUDFLAREAPI") can be simplified to NewDnsProvider("cloudflare") (See https://stackexchange.github.io/dnscontrol/creds-json#cleanup)

[tlimoncelli]

I've fixed the problem with #1508 but I'm not entirely happy with it.

Cause of the bug

The "check" subcommand performs all validations that can be done without reading creds.json. The basic idea is that you have some users that aren't allowed to see creds.json (because it includes secrets) but are involved in submitting PRs. They should be able to do a basic syntax check without creds.json access. Testing that does require access to the creds.json file is done by dnscontrol preview, a command that is run by privileged users or via a CI/CD pipeline that does have access to the secrets in creds.json.

Prior to v3.16, dnscontrol check knew a provider's type id because it was stored in dnsconfig.js. Therefore it can run the provider's AuditRecords() method.

Starting in v3.16, if dnsconfig.js uses the new syntax for NewDnsProvider(), check can't run a provider's AuditRecords() method because it doesn't know what provider is in use. It doesn't have access to this information because it has moved to creds.json.

AuditRecords() currently does very little. It basically audits TXT records for incompatibility with the provider (i.e. some providers don't permit quotes, or very long strings, or non-printables).

I can think of a few ways to move forward:

Path 1: "check" doesn't do per-provider AuditRecords().

Simply skip tests that can't be done (as implemented in #1508)

• Pro: This keeps "check" consistent with its historical definition of "all the testing we can do without access to creds.json".
• Pro: Easy to implement (it's already implemented!)
• Con: "check" loses the TXT auditing functionality.
• Con: Validating TXT (and some day other) records is delayed to "preview".

Path 2: Opportunistically read creds.json

Change dnscontrol check to read creds.json if it exists. If the TYPE parameter exists for a provider, the AuditRecords() tests are run. If creds.json can't be read, silently do fewer tests.

• Pro: Restores the old behavior (performs more tests) for users that do have access to creds.json
• Con: check will work differently for different users. This inconsistency will be difficult to explain, and create confusion for users.

Path 3: Support the old syntax for users that want check to run AuditRecords()

The code still runs AutoRecords() when the old syntax is used (the 2-parameter NewDnsProvider("name", "TYPE")). We could simply retain this syntax for sites that want AuditRecords() to run on check. Currently this syntax results in an information message being printed encouraging the new syntax. We would disable the message.

FYI: The message looks like this:

INFO: In dnsconfig.js NewDnsProvider("name", "TYPE") can be simplified to NewDnsProvider("name") (See https://stackexchange.github.io/dnscontrol/creds-json#cleanup)

• Pro: Maximum flexibility
• Con: Flexibility is confusing. Having "magic" that happens based on whether you supply 1 vs. 2 parameters to NewDnsProvider() is the opposite of dnscontrol's goal of being easy to use with little "magic".

Path 4: Revert "plan A" and try Plan B or C

The original proposal (RFC: Include the provider type in creds.json, remove it from dnsconfig.js ) had 3 proposed ways to achieve this goal. Plan B and C would not have the problem described here.

In particular, provider names would embed their type, thus the code would always know the type.

For example, NewDnsProvider("cloudflare_tal", "CLOUDFLAREAPI") would change to NewDnsProvider("cloudflare_tal:CLOUDFLAREAPI") and the primary key in creds.json would be cloudflare_tal:CLOUDFLAREAPI.

• Pro: Solves this problem
• Con: Throws away a lot of code just for a bug that is more of a nuisance

Final thoughts

I'm probably over-thinking this. Having the TXT audit run on "preview" instead of "check" is no big loss. The check still runs, just shifted right a little. Therefore, Path 1 is probably good enough.

CC @philpennock since he had strong feelings about the RFC.

[philpennock]

I had a wide-margin preference for not embedding structure inside string keys in JSON, that's it.

Ultimately, with B or C you're saying "don't repeat information" and now also "but we should repeat information, just embed it inside one string" and then ... apparently repeat it in both files after all. My reading of #1457 was that the provider typing would only be in creds.json, as part of the structured names, not that the full name:PROVIDER would still be present in dnsconfig.js, which runs entirely counter to the title and theme of the RFC. I think this context has been lost when looking at the current issue with "what might make it work": yes, you could undo the duplication by changing how much needs to be repeated.

Cautiously: path 1 seems sane here.

[tlimoncelli]

Thanks for the feedback.

I think I'm over-thinking this (really? 4 paths forward?). I've implemented "path 1" and I think that's good enough.

Thanks!

[riku22]

Hello.

Thank you for the release of V3.16.1.
However, it seems that there are still problems.
Using records such as CAA and ALIAS will result in an error.

The error is as follows.

2022/05/28 14:16:39 printIR.go:88: 1 Validation errors:
2022/05/28 14:16:39 printIR.go:94: ERROR: domain example.com uses CAA records, but DNS provider type - does not support them
exiting due to validation errors

The contents of dnsconfig.js are as follows.

var registrar = NewRegistrar("none");
var gcloud = NewDnsProvider("gcloud");

D("example.com", registrar, DnsProvider(gcloud),
A("@", "127.0.0.2"),
CAA("@", "issue", "letsencrypt.org")
);

Best regards.

[tlimoncelli]

Thank you, @riku22 ! (and thanks for including a reproduction case!)

I was able to track down the problem. The fix is in #1514

[riku22]

Hello.

Thank you for the release of V3.16.2.
I confirmed that it was fixed.

Best regards.