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

Add support for DNS over TLS #392

Merged
merged 3 commits into from
Oct 23, 2019
Merged

Add support for DNS over TLS #392

merged 3 commits into from
Oct 23, 2019

Conversation

bwelling
Copy link
Collaborator

This adds a dns.query.tls() method, which sends DoT queries.

This is similar to tcp(), but adds ssl_context and server_hostname parameters.

@nicki-krizek
Copy link
Collaborator

I didn't do a thorough review, but the changes seem to work as expected. I wonder what's would be a proper way of integrating these changes with dns.resolver.Resolver (#358), but it might be out of scope of this PR.

@bwelling
Copy link
Collaborator Author

bwelling commented Oct 1, 2019

Not my call, but I think that integrating into dns.resolver.Resolver is a different issue than adding the protocol support, unless there's something needed in the tls() function to support the Resolver integration.

@filips123
Copy link
Contributor

There is one thing I found that needs to be considered. The problem is that ssl module is always imported, even if it is not used. This works normally for most Python installations (which have OpenSSL enabled), but it won't work in some cases where OpenSSL is not installed. In this case, the import will fail so the whole module will not be usable.

The solution would be to create _ssl_compat module which tries to import ssl and then handle import errors. For more details see how websocket-client handles this.

@bwelling
Copy link
Collaborator Author

bwelling commented Oct 7, 2019

I believe that replacing the import with something like this would be a lot simpler than adding a new module, if this is a real issue.

try:
    import ssl
except ImportError:
    class ssl(object):
        class WantReadException(Exception):
            pass
        class WantWriteException(Exception):
            pass
        class SSLSocket(object):
            pass
        def create_default_context():
            raise Exception('no ssl support')

@filips123
Copy link
Contributor

@bwelling Yes, this is a better version. In websocket-client, it was done with an additional module because SSL needs to be used across multiple files. But here, SSL is only required in one file, so additional module is not needed.

And yes, I think that forcing SSL could be an issue. SSL can not be available in Python, especially if you are building Python from source or using different and more limited runtime environment (maybe some microcontrollers, WebAssembly, some operating systems...).

@cjsoftuk
Copy link

Looking forward to this feature getting merged (as we're building Icinga 2 tests on top of dnspython for DNS checks and a customer has just deployed DoT and DoH). Is there any indication of when this might be available in master?

@rthalley rthalley merged commit 1ede966 into rthalley:master Oct 23, 2019
@filips123
Copy link
Contributor

@rthalley Can you also merge my PR for DNS over HTTPS?

@filips123
Copy link
Contributor

Also, forcing to use ssl is not good solution. You should fix it as in #392 (comment).

@rthalley
Copy link
Owner

I was committing the ssl fix already :)

@cjsoftuk
Copy link

@rthalley - Thank you for such prompt action on this!

nrhall pushed a commit to nrhall/dnspython that referenced this pull request Jun 23, 2020
nrhall pushed a commit to nrhall/dnspython that referenced this pull request Jun 23, 2020
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 this pull request may close these issues.

5 participants