-
Notifications
You must be signed in to change notification settings - Fork 294
TLS Client authentication #172
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
base: master
Are you sure you want to change the base?
Conversation
ameshkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read all comments before making changes.
Briefly:
- Instead of
TLSClientConfigwe should passTLSClientCertificatesdown to the upstreams. Passing the whole config seems unnecessary. - There are some naming inconsistencies that should be resolved.
- There are no unit tests at all for this feature.
How to write a unit test. The tests that we have in the upstream right now are not ideal since they depend on remote servers.
For this feature, we'd better write a normal unit test that does not have any external dependencies.
Let's start with a DOT server. Here's what needs to be done:
- Generate a server cert-key pair (see
createServerTLSConfiginproxy_test.go) - Generate a client cert-key pair.
- Create a
net.Listenerthat listens to127.0.0.1:0(this way it will use an unoccupied port) - Grab the port from listener.Addr(), you'll use it to create an upstream
- Create a
tls.Listenerwith the tls.Config from steps 1 and 2. This tls.Config should have the following fields set:Certificates,ClientAuthandClientCAs-- so that it was verifying the client cert. See the example here: https://gist.github.com/xjdrew/97be3811966c8300b724deabc10e38e2#file-server-go-L29
- This tls.Listener should accept the incoming connection, read the DNS message, check that it's valid and return some response.
- Create an Upstream with
InsecureSkipVerifyset to true and TLS client auth enabled. - Send a DNS message through it and verify the response.
One thing to note. Instead of setting InsecureSkipVerify on step 6 it might be better to have an option to pass RootCAs to the the upstream's TLS config. Currently, it can be done via upstream.RootCAs property, but tbh this is a part of the old code that is not used anymore and can be removed. Consider extending upstream.Options and allowing passing RootCAs via it.
README.md
Outdated
| A simple DNS proxy server that supports all existing DNS protocols including `DNS-over-TLS`, `DNS-over-HTTPS`, `DNSCrypt`, and `DNS-over-QUIC`. Moreover, it can work as a `DNS-over-HTTPS`, `DNS-over-TLS` or `DNS-over-QUIC` server. | ||
|
|
||
| > Note that `DNS-over-QUIC` support is experimental, don't use it in production. | ||
| > Note that `DoH/DoT/DoQ` client authentication support is experimental, only DoH authentication has been tested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? We can test this rather easily with a unit-test for all three
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #172 +/- ##
==========================================
+ Coverage 68.67% 68.74% +0.06%
==========================================
Files 40 40
Lines 2369 2377 +8
==========================================
+ Hits 1627 1634 +7
- Misses 539 540 +1
Partials 203 203 ☔ View full report in Codecov by Sentry. |
Hello I added support for TLS Authentication. For now I tested it with my own DoH server and open ssl certificates. It should also work for DoT and DoQ but need further tested.
I tried my best to follow the comments from @ameshkov in my previous pull request (doauth).
BR
Ivan