v2.0.0-beta2

[mosajjal]

Full Changelog: v2.0.0-alpha3...v2.0.0-beta2

https://github.com/mosajjal/sniproxy/releases/tag/v2.0.0-beta2

---

This discussion was created from the release v2.0.0-beta2.

[holygrolli]

thx for the super fast release of beta2.
my previous issues were addressed now (DEBUG log level works and not proxying all domains) 👍

I will give these basic features a test today. and will also try out DoH the next days!

[holygrolli]

ok, after reading the code the 0 cidr(s) loaded is because of the general 0.0.0.0/0,deny rule. apparently it only logs the number of allow rules here.
After figuring that out I tried to verify if this works through following test:

#	acl # 1	acl # 2	dns	https443	expected
1	acl=domain ✅	acl=cidr 🚫	OriginIP 🚫	🚫	👎
2	acl=domain ✅	acl=cidr ✅	OriginIP 🚫	🚫	👎
3	acl=domain ✅		ProxyIP ✅	✅	👍
4	acl=domain 🚫		OriginIP 🚫	🚫	👍
5	acl=cidr 🚫	acl=domain ✅	OriginIP 🚫	🚫	❓
6	acl=cidr ✅	acl=domain ✅	OriginIP 🚫	🚫	👎

From my tests only rules 3 and 4 work as expected.
Rule 5 confuses me because the origin dns is returned, but I would expect no answer because my ip is not in the allow list. should sniproxy reply at all when the cidr acl denies it? from a point of view of unwanted use of my proxy I would expect nobody being able to acces it.
and regarding all other test cases: it seems whenever I use acl=cidr (regardless of the priority) the dns query always returns the origin address. this is unexpected.
Thanks for your support

[mosajjal]

thanks a lot for this test. I'm going to change the ACL to not have the inherit reject by default, and leave that to the user to put in. so in a scenario that you want to allow only a certain source IP cidr, it'll look like this:

10.0.0.0/24,allow
0.0.0.0/0,reject

also, rather than overriding the previous policy in case of an "accept", I restore the previous decision made by the other ACLs. that should prevent the conflicts.

let's test the latest commit and see if there's any improvements to that table. I haven't tested CIDR myself at all so appreciate your help on this :)

[holygrolli]

I provided #47 for testing the latest commit b20546a. seems to work
What I really would like to see in

sniproxy/dns.go

Lines 77 to 78 in 29a5a01

	// Otherwise do an upstream query and use that answer.
	resp, rtt, err := c.dnsClient.performExternalAQuery(q.Name, q.Qtype)

is a check if Decision == acl.Reject and do not forward the request at all. I don't want to be a public DNS

[holygrolli]

current testresults after b20546a

#	acl # 1	acl # 2	dns	https443	expected
1	acl=domain ✅	acl=cidr 🚫	OriginIP 🚫	🚫	❓ (domain acl has priority, probably expected)
2	acl=domain ✅	acl=cidr ✅	ProxyIP ✅	✅	👍
3	acl=domain ✅		ProxyIP ✅	✅	👍
4	acl=domain 🚫		OriginIP 🚫	🚫	👍
5	acl=cidr 🚫	acl=domain ✅	OriginIP 🚫	🚫	👎 (dns should be nil)
6	acl=cidr ✅	acl=domain ✅	ProxyIP ✅	✅	👍

OriginIP 🚫 = real IP is resolved/returned and sniproxy is NOT used
ProxyIP ✅ = sniproxy IP is resolved/returned and sniproxy IS used

[mosajjal]

merged your tests and added the outright reject for DNS. hopefully now everything should work as expected.

[mosajjal]

yep that's doable. can you please post the table with the latest commit's changes so we know the actual ACL logic is sound. I'll go take a look at the unit tests and merge them in as well.