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

Rework domain verification a bit #67

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Rework domain verification a bit #67

merged 4 commits into from
Oct 22, 2024

Conversation

sfan5
Copy link
Contributor

@sfan5 sfan5 commented Oct 15, 2024

old domain verification:

  • if client IP matches the server domain: all good ✔️
    • e.g. 1.2.3.4 announces foo.com which resolves to 1.2.3.4 and 2001:db8::1
  • otherwise: failure ❌

problem: the domain verification has been disabled in prod for a long long time since it caused too many false-positive issues
A common problem is that the announce request comes over IPv6 (these days often configured automatically), but at the same time the domain does not have an IPv6 address.
(most people don't care or have reasons for avoiding it and you also need ipv6_server=true for it to work)

new domain verification:

  • if client IP matches domain: all good ✔️
  • otherwise, if client is IPv6 and domain only has IPv4 (or inverse): silently tolerated ✔️
    • the problematic situation described above
  • otherwise, if there is no domain name: failure ❌
    • e.g. 1.2.3.4 announces 4.5.6.7
  • otherwise: pass, but client is given a warning that this will become stricter in the future¹ ⚠️
    • e.g. 1.2.3.4 announces foo.com which points to 1.2.3.5

new domain verification (intended future change, next month):

  • if client IP matches domain: all good ✔️
  • otherwise, if client is IPv6 and domain only has IPv4 (or inverse): silently tolerated ✔️
  • otherwise: failure ❌

¹: there are a bunch of big servers that accidentally fail this check. hence the grace period.

@sfan5 sfan5 changed the title Rework domain verification Rework domain verification a bit Oct 15, 2024
@sfan5 sfan5 added the feature label Oct 19, 2024
Copy link
Contributor

@ShadowNinja ShadowNinja left a comment

Choose a reason for hiding this comment

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

Seems like a good compromise for now. Better than the status quo at least.

server.py Outdated Show resolved Hide resolved
@@ -420,6 +425,7 @@ def finishRequestAsync(server):

def asyncFinishThread(server):
checkAddress = server["ip"] != server["address"]
errorTracker.remove(getErrorPK(server))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if get() atomically got the error and cleared it instead of clearing it manually. Maybe name it take() instead.

Copy link

Choose a reason for hiding this comment

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

Seeing how its done throughout the file, (as well as how often its done), I don't think it's worth changing this. Any implicit behavior like that would also throw me off guard. Not worth it.

@swagtoy
Copy link

swagtoy commented Oct 20, 2024

I assume this is for all the spam that we were manually deboosting? If so, ship it.

Does this rely on the reported domain from the host? I forget the name of this but i.e. IRC servers simply set the vhost to this, but they usually ignore it. Some satellite ISP's also report different results. If this has nothing to do with that then we are good.

HOWEVER, a flag for servers to decide on a hard IP domain check might be a good idea, but i think minetest.conf handles that so this note is dumb

@sfan5
Copy link
Contributor Author

sfan5 commented Oct 20, 2024

I assume this is for all the spam that we were manually deboosting? If so, ship it.

While this fixes a very obvious vector for fake servers, that wasn't really the intent.

Does this rely on the reported domain from the host? I forget the name of this but i.e. IRC servers simply set the vhost to this, but they usually ignore it.

That's called reverse DNS and it's not used here.

@swagtoy
Copy link

swagtoy commented Oct 20, 2024

That's called reverse DNS and it's not used here.

Yeah thats what i was thinking of. Thank you.

An option to maybe enforce that per server... is this a bad idea? I can't think of a use of it though

Co-authored-by: ShadowNinja <[email protected]>
@sfan5 sfan5 merged commit 6edaa91 into master Oct 22, 2024
2 checks passed
@sfan5 sfan5 deleted the addrverify branch October 22, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants