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.

Sign up for GitHub

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

Jump to bottom

Only match lowercase and ascii domains #630

Merged
merged 9 commits into from
Sep 19, 2021
Merged

Only match lowercase and ascii domains #630

merged 9 commits into from
Sep 19, 2021

Conversation

Y0ba
Copy link
Contributor

@Y0ba Y0ba commented Sep 18, 2021 •
edited

No description provided.

@Y0ba Y0ba marked this pull request as ready for review September 18, 2021 18:53
@zonyitoo

This comment has been minimized.

Sign in to view
@Y0ba
Copy link
Contributor Author

Y0ba commented Sep 18, 2021

I also noticed check_outbound_blocked uses acl rules. Does it need conversion too?

zonyitoo
zonyitoo requested changes Sep 18, 2021
View reviewed changes
crates/shadowsocks-service/src/acl/mod.rs Outdated Show resolved Hide resolved
crates/shadowsocks-service/src/local/dns/server.rs Show resolved Hide resolved
crates/shadowsocks-service/src/acl/mod.rs Outdated Show resolved Hide resolved
@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 18, 2021 •
edited

I also noticed check_outbound_blocked uses acl rules. Does it need conversion too?

Of course.

@Y0ba
Copy link
Contributor Author

Y0ba commented Sep 18, 2021

I factored out conversion logic into separate function convert_to_ascii so now it's trivial to use. I still don't know what the best place for conversion.

  • check_host_matched - sometimes will lead to double conversion.
  • check_host_in_proxy_list (via bool flag or separate function) - will need to add conversion separately in check_outbound_blocked anyway because it calls check_host_matched directly.
  • check_outbound_blocked & check_target_bypassed - that's where I put them. And documented both check_host_in_proxy_list and check_host_matched that they accept now only ASCII encoded domains.

What do you think?

@zonyitoo
Copy link
Collaborator

I am Ok with your current solution, just one problem left.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 19, 2021 •
edited

We may also need to handle FQDN.

Since all the ACL file we can find on the internet does not contain the last ., we should try to remove the last . in check_host_matched and also remove the last . when parsing ACL files.

@Y0ba
Copy link
Contributor Author

Y0ba commented Sep 19, 2021 •
edited

Also as a convenient feature it's also possible to trim whitespace for all rules since they're invisible and make rules invalid.

@zonyitoo
Copy link
Collaborator

There should be nothing left. Do you have others? @Y0ba

@Y0ba
Copy link
Contributor Author

Y0ba commented Sep 19, 2021

Do you have others? @Y0ba

Everything seems fine.

@zonyitoo zonyitoo merged commit b2167b6 into shadowsocks:master Sep 19, 2021
@Y0ba Y0ba deleted the lowercase-all-things branch September 19, 2021 05:12
atkdef pushed a commit to atkdef/shadowsocks-rust that referenced this pull request Sep 24, 2021
@Y0ba @zonyitoo
Only match lowercase and ascii domains (shadowsocks#630)
58d7b07 
* Only match lowercase and ascii domains

* Fix clippy warnings

* Make conversion in check_outbound_blocked to

* unified ACL public APIs to convert hosts to ASCII

* trim FQDN last dot

* optimize Rules Debug message length

* DNS server check ACL FQDN directly

* trim whitespaces in rules

* fix comment

Co-authored-by: zonyitoo <zonyitoo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zonyitoo zonyitoo zonyitoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Y0ba @zonyitoo