Skip to content

Commit

Permalink
bypassing DNS queries of remote servers' address
Browse files Browse the repository at this point in the history
  • Loading branch information
zonyitoo committed May 7, 2021
1 parent d476ef3 commit b06391c
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 4 deletions.
37 changes: 33 additions & 4 deletions crates/shadowsocks-service/src/local/dns/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
//! This DNS server requires 2 upstream DNS servers, one for direct queries, and the other queries through shadowsocks proxy
use std::{
cmp::Ordering,
collections::HashSet,
io::{self, ErrorKind},
net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr},
str::FromStr,
sync::Arc,
time::Duration,
};
Expand Down Expand Up @@ -321,16 +323,36 @@ fn check_name_in_proxy_list(acl: &AccessControl, name: &Name) -> Option<bool> {
}

/// given the query, determine whether remote/local query should be used, or inconclusive
fn should_forward_by_query(acl: Option<&AccessControl>, query: &Query) -> Option<bool> {
if let Some(acl) = acl {
fn should_forward_by_query(context: &ServiceContext, balancer: &PingBalancer, query: &Query) -> Option<bool> {
// Check if we are trying to make queries for remote servers
//
// This happens normally because VPN or TUN device receives DNS queries from local servers' plugins
// https://github.com/shadowsocks/shadowsocks-android/issues/2722
for server in balancer.servers() {
let svr_cfg = server.server_config();
if let ServerAddr::DomainName(ref dn, ..) = svr_cfg.addr() {
// Convert domain name to `Name`
// Ignore it if error occurs
if let Ok(name) = Name::from_str(dn) {
// cmp will handle FQDN in case insensitive way
if let Ordering::Equal = query.name().cmp(&name) {
// It seems that query is for this server, just bypass it to local resolver
trace!("DNS querying name {} of server {:?}", query.name(), svr_cfg);
return Some(false);
}
}
}
}

This comment has been minimized.

Copy link
@Mygod

Mygod May 7, 2021

Contributor

Maybe only enable this behavior when a plugin is set?

This comment has been minimized.

Copy link
@zonyitoo

zonyitoo May 8, 2021

Author Collaborator

Maybe. But remote servers' domain can only be resolved by "local" resolver, right?

This comment has been minimized.

Copy link
@Mygod

Mygod May 8, 2021

Contributor

Not necessarily?

This comment has been minimized.

Copy link
@zonyitoo

zonyitoo May 8, 2021

Author Collaborator

Consider if remote servers' domain names are resolved by the "remote" resolver, sending requests to "remote" resolver requires to resolves the remote servers' domain names first, which will become an infinite loop.

This comment has been minimized.

Copy link
@Mygod

Mygod May 8, 2021

Contributor

Oh but the point is that this will only happen when plugin is enabled in VPN mode. As long as we bypass our own DNS requests correctly, this shouldn't be an issue? (We already bypassed these DNS requests when plugins are not enabled.)

This comment has been minimized.

Copy link
@zonyitoo

zonyitoo May 8, 2021

Author Collaborator

Indeed. So there won't be any difference for android in VPN mode. But keeping these lines right here will help to catch all exceptions.

This comment has been minimized.

Copy link
@Mygod

Mygod May 8, 2021

Contributor

Ok sounds good. We will keep this behavior until someone complains.


if let Some(acl) = context.acl() {
if query.query_class() != DNSClass::IN {
// unconditionally use default for all non-IN queries
Some(acl.is_default_in_proxy_list())
} else if query.query_type() == RecordType::PTR {
Some(should_forward_by_ptr_name(acl, query.name()))
} else {
let result = check_name_in_proxy_list(acl, query.name());
if result == None && acl.is_ip_empty() && acl.is_host_empty() {
if result.is_none() && acl.is_ip_empty() && acl.is_host_empty() {
Some(acl.is_default_in_proxy_list())
} else {
result
Expand Down Expand Up @@ -452,12 +474,19 @@ impl DnsClient {
message.set_recursion_desired(true);
message.set_recursion_available(true);
message.set_message_type(MessageType::Response);

if !request.recursion_desired() {
// RD is required by default. Otherwise it may not get valid respond from remote servers

message.set_recursion_desired(false);
message.set_response_code(ResponseCode::NotImp);
} else if request.op_code() != OpCode::Query || request.message_type() != MessageType::Query {
// Other ops are not supported

message.set_response_code(ResponseCode::NotImp);
} else if request.query_count() > 0 {
// Make queries according to ACL rules

let (r, forward) = self.acl_lookup(&request.queries()[0], local_addr, remote_addr).await;
if let Ok(result) = r {
for rec in result.answers() {
Expand Down Expand Up @@ -486,7 +515,7 @@ impl DnsClient {
// Start querying name servers
debug!("DNS lookup {:?} {}", query.query_type(), query.name());

match should_forward_by_query(self.context.acl(), query) {
match should_forward_by_query(&self.context, &self.balancer, query) {
Some(true) => {
let remote_response = self.lookup_remote(query, remote_addr).await;
trace!("pick remote response (query): {:?}", remote_response);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,11 @@ impl PingBalancer {
pub fn best_udp_server(&self) -> Arc<ServerIdent> {
self.inner.context.best_udp_server()
}

/// Get the server list
pub fn servers(&self) -> &[Arc<ServerIdent>] {
&self.inner.context.servers
}
}

impl Debug for PingBalancer {
Expand Down

0 comments on commit b06391c

Please sign in to comment.