minidns icon indicating copy to clipboard operation
minidns copied to clipboard

Allow DNSServerLookupMechanism to specify ports for the servers

Open Ch4t4r opened this issue 6 years ago • 4 comments

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.

Ch4t4r avatar Apr 25 '18 01:04 Ch4t4r

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%
<!-- Total: 38 58
Files with Coverage Reduction New Missed Lines %
minidns-client/src/main/java/org/minidns/DnsClient.java 1 52.8%
<!-- Total: 1
Totals Coverage Status
Change from base Build 159: 0.08%
Covered Lines: 2897
Relevant Lines: 5464

💛 - Coveralls

coveralls avatar Apr 25 '18 01:04 coveralls

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 avatar Apr 26 '18 08:04 Ch4t4r

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

Ch4t4r avatar Oct 01 '18 09:10 Ch4t4r

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)

MikeSchroll avatar Oct 16 '18 13:10 MikeSchroll