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

Allow DNSServerLookupMechanism to specify ports for the servers #82

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Ch4t4r
Copy link

@Ch4t4r Ch4t4r commented Apr 25, 2018

As far as I can see there currently is no way to alter the port a query is sent to when using ResolverAPI or DnssecResolverApi. This would be possible if DNSServerLookupMechanism specified ports for the servers.

As this is a breaking change for projects using DNSServerLookupMechanism or AbstractDNSServerLookupMechanism it might be useful to keep the old getDnsServerAddresses() inside AbstractDNSServerLookupMechanism and mark it as deprecated, returning an empty list by default. A possible alteration could look like this:

/**
     * Port-aware replacement of {@link #getDnsServerAddresses()}
     *
     * @return A list of upstream DNS servers with their corresponding ports.
     *
     */
    @Override
    public List<IPPortPair> getDnsServerAddressesWithPorts(){
        List<String> servers = getDnsServerAddresses();
        // Throw some exception here if the list is empty?
        List<IPPortPair> pairs = new ArrayList<>(servers.size());
        for(String server: servers){
            pairs.add(new IPPortPair(server, IPPortPair.DEFAULT_PORT));
        }
        return pairs;
    }

    /**
     * @return A List containing non-null DNS servers represented as IP addresses
     *
     * @deprecated override {@link #getDnsServerAddressesWithPorts()} instead
     */
    public List<String> getDnsServerAddresses(){
        return Collections.emptyList();
    }

This however would break the concept of abstract classes as you would no longer be forced to implement getDnsServerAddressesWithPorts() in a subclass - that's why it isn't included in this commit.

Another way could be using REGEX to parse strings returned with getDnsServerAddresses(), extract possible ports from them (or fallback to 53) and return these for getDnsServerAddressesWithPorts by default.

@coveralls
Copy link

coveralls commented Apr 25, 2018

Pull Request Test Coverage Report for Build 160

  • 38 of 58 (65.52%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 53.02%

Changes Missing Coverage Covered Lines Changed/Added Lines %
minidns-android21/src/main/java/org/minidns/dnsserverlookup/android21/AndroidUsingLinkProperties.java 0 1 0.0%
minidns-client/src/main/java/org/minidns/dnsserverlookup/AndroidUsingExec.java 0 1 0.0%
minidns-client/src/main/java/org/minidns/dnsserverlookup/AndroidUsingReflection.java 0 1 0.0%
minidns-client/src/main/java/org/minidns/dnsserverlookup/IPPortPair.java 6 8 75.0%
minidns-client/src/main/java/org/minidns/dnsserverlookup/AbstractDnsServerLookupMechanism.java 5 11 45.45%
minidns-client/src/main/java/org/minidns/DnsClient.java 26 35 74.29%
Files with Coverage Reduction New Missed Lines %
minidns-client/src/main/java/org/minidns/DnsClient.java 1 52.8%
Totals Coverage Status
Change from base Build 159: 0.08%
Covered Lines: 2897
Relevant Lines: 5464

💛 - Coveralls

Copy link
Collaborator

@Flowdalic Flowdalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially MiniDNS's DNSServerLookupMechanism was designed with the information retrieved from typical system's DNS settings (think of e.g. /etc/resolv.conf). Those just return IP addresses, and nothing more.

I'm hesitantly to change this right now without a good reason or motivation. I'm not saying that we shouldn't do something like this in the future. But then we should go the full way through and hve DNSServerLokupMechanism return a DnsServer type (akin to your UpstreamDnsServer class), which includes not only the IP and port, but also the servers capabilities (if available). Capabilities could be

  • unknown
  • traditional DNS
  • DNS over TLS

* All redistributions of this software in source code must retain this copyright header
* All redistributions of this software in binary form must visibly inform users about usage of this software
* <p>
* [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this added by accident? If so, please remove it.

Copy link
Author

@Ch4t4r Ch4t4r Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was. I'm removing it.

@@ -29,6 +29,6 @@
*
* @return a List of Strings presenting hopefully IP addresses.
*/
public List<String> getDnsServerAddresses();
public List<IPPortPair> getDnsServerAddressesWithPorts();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to rename the method, just changing the return type is fine.

@Ch4t4r
Copy link
Author

Ch4t4r commented Apr 26, 2018

Using non-default ports is pretty rare. My initial thought was to add support for non-default ports without breaking too much existing stuff. If this doesn't get merged (and is done properly some time in the future) I'm totally fine with it as I can just use my fork.

I've changed the reviewed parts. The copyright header was auto-generated by Android studio and slipped through my sight.

@Ch4t4r
Copy link
Author

Ch4t4r commented Oct 1, 2018

I merged everything into this PR. If wanted/needed I can integrate the suggested changes regarding server capabilities as well, but that'd require more changes as DNS over TLS (or other capabilities as DNS over HTTPS) most of the time isn't wanted as it increases traffic by a lot - there'd need to be some sort of input from the developer then if those capabilities should be used

@MikeSchroll
Copy link

I'm in support of having alternative port support. This is something we use to get around wireless carriers who like to intercept dns and redirect to their own servers (transparent dns proxying at the carrier level)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants