windowsdnsserver driver implementation

[bilalekremharmansa]

What(What have you changed?)

windowsdnsserver driver implementation uses PowerShell DNSServer module for Let's Encrypt dns challenge.

details about DNSServer module: https://docs.microsoft.com/en-us/powershell/module/dnsserver/?view=win10-ps

The driver uses a wrapper python library, which is called windowsdnsserver-py, to interact with PowerShell DnsServer module. Basically, the driver performs process calls to DNSServer over python subprocess module. Since commands are made on local machine (remote session is not supported), this driver has to be used on windows server where dns server is located, either using sewer as a cli and as a library.

Why(Why did you change it?)

new dns driver to sewer

[codecov]

Codecov Report

Merging #215 (0aeb342) into master (d9238ac) will decrease coverage by 0.66%.
The diff coverage is 70.00%.

❗ Current head 0aeb342 differs from pull request most recent head 366ffd2. Consider uploading reports for the commit 366ffd2 to get more accurate results

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
- Coverage   88.02%   87.36%   -0.67%     
==========================================
  Files          21       22       +1     
  Lines        1303     1353      +50     
==========================================
+ Hits         1147     1182      +35     
- Misses        156      171      +15     

Impacted Files	Coverage Δ
sewer/dns_providers/windowsdnsserver.py	70.00% <70.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 056ac64...366ffd2. Read the comment docs.

[mmaney]

Sorry, @bilalekremharmansa, it's been crazy here - more a hardware upgrade that went awry than any other single thing. For now, mostly code non-specific questions for you.

Love that you've noticed dns-01 and added the new driver! I'll have to revise that, since you didn't stuff it into the old interface, but that's a problem I enjoy. :-) A couple questions that relate to the feature columns there.

• Do you know that it won't work with the extended wildcard, or has that just not been tried? That would be like --domain test.invalid --alt_domains *.test.invalid in terms of the cli options. What sometimes does cause an issue is that this use case requires setting two different TXT records on the same DNS name.
• Alias support is easy if you want to add it. Just wanted to add that point.
• Propagation... I think you implemented the interface and, from a quick look a week ago, it looks okay. Perversely, what I feel I have to question is whether there's any point in having that check in a driver that only works with a local DNS server. Or is it known that there can be a delay after accepting the record before it will answer for it?

Moving along to zone, have you seen my out-loud musing about something very much like that in #214? I'm not sure that it would make sense to try to use the same name, anyway, since it's previously been seen as an optional simplification, or an override when the sensible grovelling of the available DNS data would lead to the wrong result. Frankly, it does feel [more] right to have the driver own the parameter when it's mandantory for operation. Thoughts?

[bilalekremharmansa]

Hi, @mmaney

First of all, I did not understand what you mean by "since you didn't stuff it into the old interface".

• I've not tried the wildcard certificate yet. I will try it soon, and I will let you know about the result. By the way, in that case, how will I add two different TXT records ? Are there going to be two 'challenges' in DNSProviderBase#setup ?

• I've checked aliasing and I am planning to support it.

• You probably right about propagation. While I was implementing the propagation, I've not had enough knowledge about local DNS Server, and I thought we need a delay for LE to ask for the record. Now, I could remove it since it has no effect remarkable.

About the zone, I've glanced at to existing dns providers and some of them use zone information to operate and some of them has not need at all. In that case, I've agreed with you, optional zone name could be useful in some cases, but I think each driver should has its own parameter if require's zone information.

[mmaney]

@bilalekremharmansa, let me rephrase the "stuffing" remark: Thank you for using the new interface, even though all the existing examples of working drivers are still implemented using the legacy interface!

wildcard: yes, a requested name *.domain.tld has to map to a challenge based on the valid DNS name domain.tld, so AcmeLib (what I'm calling all the inside stuff as I rework it) passes the challenge to the driver as a TXT record for _acme-validation.domain.tld. When the request wants to include the naked domain.tld, it has to include that name as well, and that produces another challenge for the same _acme-validation.domain.tld. Hopefully the server will Just Work and accept both resource records. Many of the services seem to have an issue - usually in their APIs, I suspect - with doing this, and need service-specific coaxing to work. So far they've all been able to be convinced to work.

As you've observed, some of the services require the driver to identify the zone in which to publish the records. I don't happen to use any such service myself, but from what I've seen, there's some sort of list the service keeps that is connected to the user account, and the search is for the one that matches the requested ...domain.tld. As the bug RFC I posted says, there have been occasional discussions about letting the user explicitly say what zone to use, which makes sense to me. Even with that the driver might still need to check the list; perhaps the service needs the zone's internal ID rather than the DNS name, but that would still be simpler than trying to match a varying part of the requested name.

[bilalekremharmansa]

@mmaney, okay I understand now and I thought that being first one to implementing the new dns driver would be nice 😄. If you have any reservations I would like to hear.

I've also made some tests about wildcard issue, and here my observations;

• Windows DNSServer module is able to handle multiple TXT records by default, so I've not modified existing implementation.
• I've tried with arguments that you mention in previous comment, like: --domain example.tld --alt_domains **.example.tld. During the process of dns-challenging, I observerd the DNS service and two different TXT records are added, so certificate is created succesfully. Then I check certicate details, it was issued to "example.tld" and there were two SANs, one was the wildcard one "*.example.tld", and the other one was "example.tld".
• Yet the following arguments produce errors "--domain .example.tld" which is not related to acme. Windows does not allow * (asterisk) character in a file name. Since sewer try to create account.key file name as '.example.tld.account.key', OS produce the following error;

File "C:\virtualenv\ssl\lib\site-packages\sewer\cli.py", line 410, in main
account.write_pem(account_key_file_path)
File "C:\virtualenv\ssl\lib\site-packages\sewer\crypto.py", line 251, in write_pem
with open(filename, "wb") as f:
OSError: [Errno 22] Invalid argument: 'C:\tmp\*.example.tld.account.key'

• In that case, adding bundle_name solves the problem, like "--bundle_name=test". With this way account key name would be 'test.account.key'.

And the zone issue, what is your mind about the zone issue ? I've come up with two ideas;

1 - Deprecate domain paramater, and propose two new parameters -> "zone:str" and "subdomain:str"

For instance for challenge to "test.example.tld" -> zone: example.tld, subdomain: test

This one would be used from all providers and that would make more clear, however that breaks backward compatibility for sure, and I assume you would not prefer that.

2- New parameter for both zone and cli, like zone_id:str

Some providers will not use this new parameter. Still, should driver implement this behaviour by theirselves ? or sewer client should implement it ? Currently, I think adding to sewer client semantically is not completely wrong, yet some providers will not use it and that yields a provider(s) spesific argument. For example, in that case people who use windowsdnsserver driver need to know zone_id is a REQUIRED parameter, on the other hand acmedns driver users will not use zone_id at all.

---

Lastly, I have a question, here is a code snippet in sewer DNSProviderBase

class DNSProviderBase(ProviderBase):
  ...

  def target_domain(self, chal: Dict[str, str]) -> str:
  "returns fqdn where challenge TXT should be placed"

  d = chal["domain"]
  return "_acme-challenge." + d if not self.alias else d + "." + self.alias

In master code, there is no field named 'domain' in challenge. I am not planning to use this function, but I notice that and wanted let you know in case you are not aware of it.

[bilalekremharmansa]

circleci failed because I did not push windowsdnsserver-py package's new version to pypi when circleci is triggered. Now, I've published the new version to pypi. Is there a way to rerun without committing ?

[mmaney]

@bilalekremharmansa Yes, there's a rerun button somewhere in the mess of CircleCI's web stuff. Don't know if you can get to it, and I just kind of poke around looking for it myself... Not so fond of CCI, its reports seem to obfuscate as much as they reveal. But for now, well, it's already setup and more or less works.

I was somewhat surprised to find that sewer created a file named *.domain.tld on Linux, and have thought that ought to be handled better. With Windows not at all in mind my first thought was WILDCARD.domain.tld. ... I never really thought about it before, but I wonder what sewer does if you give it DOMAIN.TLD? I guess I need to make a note to look at this corner of the code, too.

re: zone - I don't see this as having anything, really, to do with the current domain parameter. It's a hint to the driver, so it belongs in the driver's __init__ arguments. Since it's one that could be of use to more than one driver I'm inclined to have it pass back up through kwargs, even if it then gets passed back through the challenges' dicts. A driver which requires the zone could report its absence in kwargs, while the ones who just enjoy the hint can find it when they need it, in the challenges data.

(I can't now recall how far I've gone with cleaning up that challenge dict interface. Early work ran into obscure issues in testing - mocks that were far too attached to details, plus that was back when a lot of the old tests were mysterious magic to me . So adding your zone before 0.9 might be... interesting.)

I was going to dispute the absence of a "domain" key, but after a little digging I see you're right. Look at unbound_ssh in dns_providers for the only known (and working!) use of target_domain. I think this may have been a case where those old issues with changing the contents of the challenge dicts used inside Client have delayed the implementation I had planned for that. :-(

Thanks!

[bilalekremharmansa]

@mmaney, yes I found the rerun button, and I am not allowed to click it. I see you handled it.

I was somewhat surprised to find that sewer created a file named *.domain.tld on Linux

While i was telling you to Windows does not allow asterisk character, I had not deep thought how it behaves on Linux. I am not sure whether sewer could manage to create a file name like "*.example.tld" or not on Linux.

Now, I've made a quick research, and you can create a file name that contains asterisk by escaping it on Linux, for instance;

$ touch \*.example.tld

That creates an empty file with named "*.example.tld". But I am not sure sewer use this trick (or the python module that sewer uses).

WILDCARD.domain.tld looks fine to me (which I prefer this one), or a second option domain.tld could be used as well. As I say said, it is not a blocking issue and can be solved by using bundle name.

---

zone -- that will probably work, and it would be easy to implement. It will be a generic parameter for dns providers who has interest in. By the way I might realise an edge case about zone. Please, correct me if I'm wrong, imagine a challenge with the following parameters:

--domain example.tld
--zone example.tld (or --p_opts zone=example.tld)
--alt_domains "blabla.example.tld", "myother.domain.tld"

In that case driver has to deal two different zones which will not have ability to handle "myother.domain.tld". I know I am pushing the limits, but it came to mind and thought its worth to mention.

---

FYI: I noticed something in Aliasing.md. In first paragraph you mention that aliasing could be use with "p_opts alias=.." which how it is working right know. However, in bottom of the document, a cli option is mentioned "--alias_domain" which is not available right know.

[mmaney]

Just patched the ["domain"] and aliasing doc issues you mentioned (for the 0.8.5 branch). These are both, in their own way, due to some parts of the code & docs having changed and other parts not having noticed. :-/

[mmaney]

@bilalekremharmansa let's get this one in for 0.8.5!

The problem with the aliasing support is fixed, so if you could rebase onto the current master branch, change that, and test it, that would be one down.

I don't like the use of asserts in some of the ways you have, such as testing and "reporting" the problem. Maybe it's just because I've been fighting with some old tests (mostly in test_Client I think) where the [over]use of generic exceptions has the tests parsing the exception's text, which is of course easily broken by just improving the description. But I'm moving in the direction of using our own exceptions, though I don't think there are many (any?) in 0.8.4. There's also an issue with those being errors that certainly ought to be logged, if sewer had proper (read: persistent) logging setup. Which is also on my list...

Also trying to make sure interfaces are checked more carefully where they are intended to interact with users' code. So that first one might be (for now, with logging left to the future)

if not isinstance(zone, str) or not zone:
    raise ValueError("windowsdnsserver requires a string value for the zone argument")

...I need to refresh my memory of just what is in 0.8.4 on this score.

Semi-trivial: the name is unwieldy. "server" is entirely unnecessary (in a sewer context). "windns" is arguably clear enough, I think.

Pointless, trivial functions like _get_challenge_name() drive me crazy. Also, I let someone else slip a bunch of Java(?) like stuff similar to that through in another driver "to be agile", and of course it's still sitting there indigestibly.

Oh yeah, speaking of names and such, this should be in sewer/providers - dns_providers is for the old fashioned drivers. I still have hopes that those will gradually get updated...

One more oh yeah - in 0.8.4, or maybe even earlier, the CI switched to using pytest as the test runner (configured in pyproject.toml). So if you were only using unittest because that's what you saw in the repo, feel free to switch. All my new tests (in the 0.9 branch) have been pytest, and they're so much clearer I've been converting some of the old ones as well.

Thanks

[bilalekremharmansa]

Thanks for the review @mmaney!

I've been refactoring the existing code, and mostly it's done. Yet, I could not decide how to mock objects with using pytest. What library (or approach) sewer use to mock test objects ?

Just after refactoring, I will rebase whole commit history of "dev/windowsdnsserver" with squashing commits if you don't mind ?

[mmaney]

@bilalekremharmansa last things first: if you want to squash your changes that's fine. sewer is configured (on github) to do squashing commits, and I don't usually care myself other than I hate to see what GH's own advice about updating a clone repo to track its parent appears to favor, with repeated merges from master.

As a reviewer, I don't want to be distracted by stuff from outside the work at hand as much as possible. ;-)

I haven't really needed to do mocks in the new (pytest) test cases yet. Nor have I looked at your tests hardly at all, since as with so much of sewer the real test has to have access to the service. The driver tests mostly mock out the most important interface, the interaction with the outside service. Not that I can see what else they could do...

In your case, the mock might [need to] be a dummy power shell runner or something like that? It's for sure the CI testing can't touch a real power shell!

I'm still more than a little distracted by the half (?) done 0.9 rework...

[bilalekremharmansa]

@mmaney,

Yes, I need to mock power shell in some cases, and also mocking sys.platform to 'windows' is required since CI testing is not running on Windows (second one comes from windowsdnsserver-py, it throws an error when DNS service is not running on Windows platform)

[bilalekremharmansa]

Hello @mmaney,

It's been a while since you've not heard anything from me :) At first, I was waiting a comment from you to my previous comment, which was about how to mock powershell with pytest. Then, unfortunately, I forget existing of this PR :/

Anyways, I've refactored the code regarding your comments, i think it is ready to go now. I, kindly, ask you to review once again, but before that I should mention about tests. I've not migrated unittest to pytest in this work (test of windns still uses unittest module), the reason is mocking. I've still not clear what to do about it.

By the way, I've run some tests to staging environment with the following parameters, and I've not encounter with any problem.

python -m sewer --endpoint staging --domain test.example.com --provider=windns --p_opts zone=example.com
python -m sewer --endpoint staging --domain example.com --alt_domains test.example.com --provider=windns --p_opts zone=example.com
python -m sewer --endpoint staging --domain test.anothersite.com --provider=windns --p_opts "zone=example.com" "alias=example.com"