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

NSAPI DNS query IP version check for non LWIP stacks. #11375

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@tymoteuszblochmobica
Copy link
Contributor

tymoteuszblochmobica commented Aug 29, 2019

Description

NetworkStack::gethostbyname(name, address, version) should return the same IP version as network interface.
There was solution #10794 for case when LWIP is in use.
This is fix for AT modems where DNS_API does the selection of server address.

Pull request type

[x ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AriParkkila
@kjbracey-arm
@SeppoTakalo
@kivaisan
@mikaleppanen

Release Notes

@ciarmcom ciarmcom requested review from AriParkkila, kivaisan, kjbracey-arm, mikaleppanen, SeppoTakalo and ARMmbed/mbed-os-maintainers Aug 29, 2019
@ciarmcom

This comment has been minimized.

Copy link
Contributor

AriParkkila left a comment

Accept also NSAPI_UNSPEC as cellular stack does not always get IP address from a modem.

If gethostbyname(,,version) was given it should probably be accepted in any case.

If we don't have IPv4 then should not accept A records, and if we don't have IPv6 then should not accept AAAA records.

If we have both IPv4 and IPv6 then get_ip_address prefers IPv6, but DNS should also query IPv4 servers as that might be the network's primary DNS server.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Sep 4, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 4, 2019

Please also fix astyle failures

@tymoteuszblochmobica

This comment has been minimized.

Copy link
Contributor Author

tymoteuszblochmobica commented Sep 4, 2019

Function gethostbyname remains unchanged. This fix refers only nsapi_dns_query_multiple ().
It uses the loop to find proper DNS server.

First nsapi_dns_get_server_addr(stack, &index, &total_attempts, &send_success, &dns_addr, interface_name)
tries to get it from stack calling:
stack->get_dns_server(*index, dns_addr, interface_name);

If it fails then dns_addr->set_addr(dns_servers[*index - DNS_STACK_SERVERS_NUM]);

Inside nsapi_dns.cpp there is secondary storage for DNS servers

static nsapi_addr_t dns_servers[DNS_SERVERS_SIZE] = {
{NSAPI_IPv4, {8, 8, 8, 8}}, // Google
{NSAPI_IPv6, {0x20,0x01, 0x48,0x60, 0x48,0x60, 0,0, // Google
0,0, 0,0, 0,0, 0x88,0x88}},
{NSAPI_IPv4, {209, 244, 0, 3}}, // Level 3
{NSAPI_IPv4, {84, 200, 69, 80}}, // DNS.WATCH
{NSAPI_IPv6, {0x20,0x01, 0x16,0x08, 0,0x10, 0,0x25, // DNS.WATCH
0,0, 0,0, 0x1c,0x04, 0xb1,0x2f}},
};

and it is used in this approach.

Previously it was a reason of improper behavoiur due to dns_addr->set_addr sets first adress from nsapi_dns dns_servers table (8.8.8.8) even if required version was IP6.
Therefore i added version checking

if(dns_addr.get_ip_version() != version){
retries = MBED_CONF_NSAPI_DNS_RETRIES;
index++;
continue;
}

to find next adresses from stack->get_dns_server or nsapi_dns dns_servers table.

I assume stack->get_dns_server is implemented only for LWIP so in case of cellular stack nsapi_dns dns_servers will be used.

Im confused with :
" Accept also NSAPI_UNSPEC as cellular stack does not always get IP address from a modem."

If nsapi_dns_query_multiple (,version==NSAPI_UNSPEC) and stack->get_dns_server is not implemented for cellular, how can we get proper DNS IP version?

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:nsapi_dns branch from e2265e2 to 7e9817e Sep 5, 2019
@AriParkkila

This comment has been minimized.

Copy link
Contributor

AriParkkila commented Sep 5, 2019

@tymoteuszblochmobica I think that version can be also NSAPI_UNSPEC?

@tymoteuszblochmobica

This comment has been minimized.

Copy link
Contributor Author

tymoteuszblochmobica commented Sep 5, 2019

Do you mean
if(version != NSAPI_UNSPEC && (dns_addr.get_ip_version() != version)){

@AriParkkila

This comment has been minimized.

Copy link
Contributor

AriParkkila commented Sep 6, 2019

@tymoteuszblochmobica If dual-stack is not required then A/AAAA version could be devised from the versions of the DNS servers' addresses?

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:nsapi_dns branch from 7e9817e to 33eae52 Sep 6, 2019
@tymoteuszblochmobica

This comment has been minimized.

Copy link
Contributor Author

tymoteuszblochmobica commented Sep 6, 2019

Current implementation:
// send the question
int len = dns_append_question(packet, 1, host, version);
err = socket.sendto(dns_addr, packet, len);

dns_append_question selects which record we want
// fill out question footer
if (version != NSAPI_IPv6) {
dns_append_word(p, RR_A); // qtype = ipv4
} else {
dns_append_word(p, RR_AAAA); // qtype = ipv6
}
dns_append_word(p, CLASS_IN);

@AriParkkila

This comment has been minimized.

Copy link
Contributor

AriParkkila commented Sep 6, 2019

@tymoteuszblochmobica Probably dns_addr.get_ip_version() could be given to dns_append_question if version is NSAPI_UNSPEC, assuming we do not request both IPv4 and IPv6?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 26, 2019

@tymoteuszblochmobica @AriParkkila How can we progress this PR? Is it still relevant?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

michalpasztamobica commented Sep 30, 2019

@AriParkkila . I talked to @tymoteuszblochmobica and it seems that the version should always be set before reaching the nsapi_dns_query() or nsapi_dns_query_async(). If it is unspecified then gethostbyname() will determine in based on stack's settings.

Do you agree?

@AriParkkila

This comment has been minimized.

Copy link
Contributor

AriParkkila commented Oct 1, 2019

the version should always be set before reaching the nsapi_dns_query()

It doesn't seem so with cellular modems/networks as we don't always get IP address at all.

@@ -1025,6 +1030,11 @@ static void nsapi_dns_query_async_send(void *ptr)
return;
}

if (dns_addr.get_ip_version() != query->version) {

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Oct 1, 2019

Contributor

Probably this should also have version != NSAPI_UNSPEC &&

@AriParkkila

This comment has been minimized.

Copy link
Contributor

AriParkkila commented Oct 1, 2019

It could be better to give dns_addr.get_ip_version() in dns_append_question()?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

michalpasztamobica commented Oct 1, 2019

@AriParkkila , please help me make sure I understand the background correctly... :)

I assume that our entry point to the DNS resolution is gethostbyname function. and that cellular modems, which inherit from NetworkStack have two options:

  1. rely on the gethostbyname provided by NetworkStack (implemented in nsapi_dns.cpp)
  2. implement their own gethostbyname, like RiotMicro does for example.

This PR will only affect the cellular modems which rely on our implementation of gethostbyname or it's async version. They both now try to guess what kind of IP connectivity is in place (4 or 6) based on the stack's IP address.

Now, your point is: what happens if DNS resolution takes place when the device doesn't have an IP address - do I understand correctly?
I wonder how would a DNS resolution work in this situation? Can the response be sent back to the device in any way?

@AriParkkila

This comment has been minimized.

Copy link
Contributor

AriParkkila commented Oct 1, 2019

DNS resolution takes place when the device doesn't have an IP address - do I understand correctly

Yes, the modem has IP address but it does not tell it to the device via AT commands. That is, we don't know the IP address type on the device, but since the modem has IP address DNS query is working fine.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

michalpasztamobica commented Oct 1, 2019

@AriParkkila , ok, we see your point now, thanks a lot for being patient with us 😃 But does this device actually use our nsapi_dns implementation for DNS resolution? Which device is it exactly? I wonder if it makes a difference if we provide it with IPv4 or IPv6 - we could just as well guess or assign a default value, as to my understanding there is no relation between DNS server's IP address and the type of addresses it will resolve?

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:nsapi_dns branch from 33eae52 to b8a18e2 Oct 8, 2019
@0xc0170 0xc0170 added needs: review and removed needs: work labels Oct 18, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

Reviewers please review

Copy link
Contributor

michalpasztamobica left a comment

I guess the most recent push from Tymoteusz includes @AriParkkila 's request: in case the ip version is unspecified the request will be send based upon whatever was specified in the dns_addr.
I think it's best to wait for Ari's approval, though.

@0xc0170 0xc0170 requested a review from AriParkkila Oct 23, 2019
@0xc0170 0xc0170 added needs: CI and removed needs: review labels Nov 4, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 4, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 4, 2019

I also restarted travis, internal fault

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 4, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 412b0ae into ARMmbed:master Nov 4, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8701 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.