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

add nodeid and use hostname as cname #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

T0biii
Copy link

@T0biii T0biii commented Oct 27, 2024

closes #5

@herbetom
Copy link
Member

At a quick glance i don't think possible duplicates are handled suffiently well. What if a node has the name of another ones nodeid?
Sure, probably only happens if someone tries to test the limits of the system but i wouldn't rule out that it happens.

Also not sure wheter there has to be a check and handling for duplicate nodeid's.

Further i'll have to think about wheter i even like the idea of merging two diffrent namespaces. My first thought would be that it should probably be a subdomain. For example like so:

001122334455.nodeid IN AAAA  2001:db8::1
001122334455.id     IN AAAA  2001:db8::1 ; maybe shorter would be better?
test-knoten         IN CNAME 001122334455.id

But not convinced either way.

One a more genereal level i think having the nodeid's somewhere is helpfull. Thanks for taking a look at it! :)

@T0biii
Copy link
Author

T0biii commented Oct 27, 2024

thanks for the quick response.
I didn't think about the that but you might be right.
subdomain sound also very nice and should be a quick fix and sould also be added very easily

@herbetom
Copy link
Member

Also there probably should be a check regarding the integrity of the nodeid. That there is one, that it's the correct amount of characters ...

Not sure how well yanic does input/output validation. I've just recently seen a community's mehshviewer.json where the dot in the long- and latitude was missing. So something like 492143232 rather then 49.2143232 which is quite obviously not a valid value.

@T0biii
Copy link
Author

T0biii commented Oct 27, 2024

with .id this look something like this

	for node in nodes:
		for address in node.addresses:
			lines.append(LINE_TPL.format(name=f"{node.nodeid}.id", type="AAAA", data=address))

		if(node.hostname != node.nodeid):
			lines.append(LINE_TPL.format(name=node.hostname, type="CNAME", data=f"{node.nodeid}.id"))
			hostname_list.append(node.hostname)
		else:
			lines.append(LINE_TPL.format(name=node.nodeid, type="CNAME", data=f"{node.nodeid}.id"))
			hostname_list.append(node.nodeid)

		hostname_list.append(f"{node.nodeid}.id")

image

but sure more integrity check would also be very good as we also had recently some problems with e.g. withspaces
https://github.com/FreifunkBremen/yanic/pull/235

@herbetom
Copy link
Member

The "id" subdomain" should then also probably be appended to the "NOTALLOWED" config option. Even if it's not strictly neccesary ...


The gist is that I don't want to trust user input and want to prevent breakage and misuse as much as possible.

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.

Add also NodeID
2 participants