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

Implement DNS over TLS for local DNS resolving #2015

Closed
wants to merge 4 commits into from

Conversation

MidoriInu1
Copy link

增加LocalDNS可在上层配置功能(原本地dns为代码硬编码)
响应Android 9.0号召,增加dns-over-tls,dns加密防劫持功能,上层可配置私人DNS
(是的,私人DNS就是google提倡的那个 private dns)

中间涉及到需要修改overtrue中部分代码,使底层支持dns-over-tls,所以拜托了我朋友(我老大),帮忙改写了对应的overture分支。

他那边应该已经提起PR了,请联合两个PR一起Review。
非常感谢。

对应的overtruePR:
shadowsocks/overture#3

@MidoriInu1
Copy link
Author

不同DNS配置项的图标我已经在制作中了,如果不介意的话,可以随意使用
image
@madeye

@madeye
Copy link
Contributor

madeye commented Dec 6, 2018

I think you can remove core/src/dnscrypt-proxy folder from the commit.

@madeye madeye requested a review from Mygod December 6, 2018 07:16
@madeye
Copy link
Contributor

madeye commented Dec 6, 2018

I suggest rename "Private DNS" to "Local TLS DNS" to avoid confusions with "Local DNS" option.

@MidoriInu1
Copy link
Author

Re:@madeye
非常抱歉。。因为之前和上游项目overtrue的作者沟通时,对方拒绝增加 tls 功能,当时想过用另一种方案实现,虽然最后在我老大的帮助下,成功使overtrue支持 dns-over-tls了,但是提交时我忘记删除我未写完的另一种方案了。。。非常抱歉,I'm sorry。。。。

p.s.图标制作中有点慢,毕竟我不是专业的UI,做好后我会再次提交一次PR,到时候烦请再次查看,非常感谢

@MidoriInu1
Copy link
Author

好的,稍等我修改下代码,因为android 9.0中官方将 dns-over-tls 功能在设置页面标识为private dns了,所以习惯性顺着走了_(:з」∠)_,稍等我马上再次提交修改完资源文件的PR

@madeye
Copy link
Contributor

madeye commented Dec 6, 2018

The overall quality of this PR is quite good.

I think it's fine to keep use private_dns in the source code. We just need to add some descriptions to that option in the UI.

@MidoriInu1
Copy link
Author

或许,中文可以翻译做本地加密dns?,我先按local tls dns改着。。。。

@MidoriInu1
Copy link
Author

我非常同意你的看法,代码中使用private_dns,对于其它代码贡献者而言是易读的,我现在正在修改UI上的显示(各String资源文件)

Copy link
Contributor

@Mygod Mygod left a comment

Choose a reason for hiding this comment

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

This proposal is set out to solve an issue that doesn't exist. DNS over TLS is aimed to preserve privacy instead of prevent DNS hijacking. Whilst a VPN is active, most (more on this later) DNS queries are already sent fully encrypted and hijacking is already impossible.

However, a subtlety is that in shadowsocks-android, DNS queries are also sent locally in overture so that we can selectively route DNS queries. I think a good way to do this is to read out system DNS using Network APIs so that users don't have to configure their DNS twice.

Your changes on overture's side looks good to me but this PR needs more work.

@MidoriInu1
Copy link
Author

@Mygod 这点我不是很认同,虽然我也许可以做到在android 9.0上读取用户当前网络的dns配置,但这对于目前大部分主流机型还是8.0的用户是不友好的。且源码中原本的local dns也是硬编码在kotlin代码中的,并没有去读取用户当前网络的dns配置。
而且我不认为使用 ss for Android APP的用户想要改变dns配置时,还需要去系统中设置,且依赖系统的支持。我认为这是不友好的,且用户应该有权力选择使用系统配置or自行单独配置。

另,国内现在已经有不少支持dns-over-tls的解析服务器了(v2ex上可以搜到不少),现在就让用户能够使用上不是更好么?

@Mygod
Copy link
Contributor

Mygod commented Dec 6, 2018

I'm fine if you want to add a global option for Android 8 and below only, but either way it's absolutely unnecessary to make it a per-profile option.

@madeye
Copy link
Contributor

madeye commented Dec 6, 2018

I agree that we can move the "private DNS" and "local DNS" option to global settings.

I think TLS DNS is also useful for preventing hijacking of the local DNS, e.g. ISP ADs. However, we disabled cert check in overture for TLS DNS, which means the hijacking of TLS DNS is still possible here.

@MidoriInu1
Copy link
Author

额。。我的意思是我在阅读原本master分支的代码时,发现local dns的配置是写死的,我只是希望作为一个用户我可以在上层选择配置不同的local dns,以及是否加密,毕竟ss for android是一个有VPN代理功能的应用。原本我的设想是在local dns选项旁边增加一个check box,提供用户可以选择是否开启tls加密。
但实际应用中我发现这个用户体验并不是很好,我一直使用的是朋友的支持dns-over-tls的解析服务器地址,但有时候他的机器并不是很稳定,每次这个时候我不得不切换回114的时候,不止是click一个 check box那么简单的,我还需要修改输入的dns地址。checkbox几乎是多余的,所以我在上层界面中将他们分为两个可输入地址项,也许我的界面设计过于激进了?但我真的觉得随着google对这个功能的重视以及普及,它在将来很可能会成为大众选项。但是dns-over-tls加密的代价是dns请求的时长会略长,所以对于比较重视解析速度的用户看来,他们可能并不愿意开启tls加密。所以多给用户一个选择的空间有什么不好呢?

@madeye
Copy link
Contributor

madeye commented Dec 6, 2018

@MidoriInu1 as mentioned above by @Mygod, you can move these options to global settings instead, rather than put them in per-profile setting.

@Mygod
Copy link
Contributor

Mygod commented Dec 6, 2018

Based on the discussions, here's a list of things that need changing before this PR can be merged, in the order of urgency.

  • Remove InsecureSkipVerify;
  • Implement connection reuse in DNS-over-TLS, similar to how Android does this;
  • Move TLS over DNS option to global settings;
  • Obtain private DNS configurations from Android system;
  • Revert all changes to local DNS and translations. (feel free to open another PR regarding local DNS)

The reason that local DNS is hardcoded is that local DNS should in principle change with system network changes and the current solution is a hack. If you wish to remove that hack, you are welcome to do so, but perhaps in another PR.

@@ -5,6 +5,9 @@
<!-- misc -->
<string name="profile">"پروفایل"</string>
<string name="connection_test_testing">"در حال آزمایش…"</string>
<string name="remote_dns">"DNS از راه دور"</string>
<string name="local_dns">DNS محلی</string>
<string name="private_dns">"DNS خصوصی"</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not add translations here. See here for more details on contributing translations.

Copy link
Author

Choose a reason for hiding this comment

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

sorry。。。我并不知道i18n的资源需要单独提交至别处,抱歉。。。

Copy link
Author

Choose a reason for hiding this comment

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

唔。。现阶段我是否应该将这些资源还原为英文,并通过faq提交翻译?
或者如果你们希望revert掉这次PR,我可以不用去还原其它string文件,在我新的尝试修改(修改到全局?)的分支上将他们排除出去,作为之后的faq提交?

@MidoriInu1
Copy link
Author

唔,那我可能需要些时间去阅读全局设置项相关的代码,如果有需要的话可以revert掉我的提交。
我需要考虑下,怎么去作为全局设置提供这项服务,抱歉这方面没有好好思考过。
我只是看到 remote DNS 那一项在配置文件中,所以顺势觉得local dns也应在单独配置文件中,且我将源码中的local dns默认值,默认填写到了此项中,我觉得对用户来说也许并不需要费力,每次新建配置时它已经是填写好的,并且可以通过复制剪贴板及保存本地等方式备份或分享给另外的设备挺方便的。。。
我需要思考下怎么让其作为一种灵活的方式提供在全局设置中,可能需要一些时间,见谅。

@madeye
Copy link
Contributor

madeye commented Dec 6, 2018

No hurry, just follow the action items listed above and then we're happy to merge your PR. 😄

@MidoriInu1
Copy link
Author

so......what should I do ?

  • [ Remove InsecureSkipVerify ]

  • [ Implement connection reuse in DNS-over-TLS, similar to how Android does this]

  • [Move TLS over DNS option to global settings ]

但是local dns和private dns两者皆改为从系统设置抓取不可单独配置吗?还是?
如果自动抓取,那么 global settings 中 TLS over DNS 选项是否将不再需要?要自动根据抓取到的系统配置去分辨应该走tcp or udp?还是?仅在增加android 8以下的设备上增加这个全局选项?

抱歉,我今天的工作进度有些delay,我需要先处理下工作。。
唔。。麻烦确认一个最终方案,我最快应该在周末有时间可以按照最终方案去code。。
非常感谢。

@Mygod
Copy link
Contributor

Mygod commented Dec 6, 2018

Please revert all changes to local DNS. If you want to make changes to local DNS, please do that in a separate PR and we will discuss there instead.

You can do a global option and I will handle the obtaining configuration from Android 9+ automatically part when I got around to do that.

@MidoriInu1
Copy link
Author

so......what should I do ? now...
系统配置中dns的配置是跟随连接的wifi信号源不同而不同的,我要做到随时切换么?我不确定我是否能监听到wifi切换信号源的事件广播,我还需要去查一下相关资料。。。

另:非常抱歉,今天真的不能再跟进了,
明天我有个很重要的分享会,可是我还没有动笔写ppt和稿子....我要加班到没车回去了...
周末我们是否可以进一步讨论下,确定个合理的方案我来改写?
如果可以的话,麻烦周末前通过我的gmail联系我,告诉我通过什么社交软件,才能高效一点和项目的维护小组沟通?(我会通过gmail回复我的对应联系方式以及其他联系方式)
非常感谢。

@Mygod Mygod changed the title 扩展了一些自己常用的小配置项,求通过或选择性添加功能 Implement DNS over TLS for local DNS resolving Dec 6, 2018
@Mygod
Copy link
Contributor

Mygod commented Dec 6, 2018

I have updated the things to be done to get this merged. See #2015 (comment).

@qyb
Copy link

qyb commented Dec 6, 2018

  • Implement connection reuse in DNS-over-TLS, similar to how Android does this

Aha. It seems that TLS connection reuse in overture is my work.

@shadowsocks shadowsocks deleted a comment from qyb Dec 7, 2018
@Mygod
Copy link
Contributor

Mygod commented Feb 3, 2019

Superseded by #2096, in which overture was removed and DNS over TLS is supported on Android 9+.

@Mygod Mygod closed this in #2096 Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants