View Issue Details

IDProjectCategoryView StatusLast Update
0004214unrealircdpublic2015-05-24 16:21
ReporternenolodAssigned Tosyzop 
PrioritynormalSeverityminorReproducibilityhave not tried
Status resolvedResolutionfixed 
Product Version 
Target Version3.4-alpha1Fixed in Version3.4-alpha3 
Summary0004214: bounce links that are too far out of sync
Descriptionnew text:
Bounce server links if their time is too much off, see last comment of Syzop. Original idea to kill of NTP code has been rejected.

original text by nenolod:
Title: remove NTP, TSCTL, etc - just bounce links that are too far out of sync
thinking about it a little, i think that this offset stuff can be manipulated to the advantage of splitriders.

i'd like to just kill it off. anyone who is not properly using NTP on their host deserves to be yelled at by the ircd.

one should especially look at this in the context of how the eventloop has to compensate for changes in the offset etc. it's just bad.
TagsNo tags attached.
3rd party modules

Relationships

related to 0004220 closedsyzop Have timesynch disabled by default 

Activities

falconkirtaran

2013-05-25 00:34

reporter   ~0017666

It adds a lot of complexity and attack surface for very little gain, and should be removed. In its place, warning and error thresholds (admin configurable) should be set up and enforced both at link connect and periodically through timestamp checking.

That said, the event loop will still have to deal with timestamp jumping and backwards clock advancement, because the server clock can change arbitrarily (eg. through the action of NTP on a fast system clock).

nenolod

2013-05-25 00:38

reporter   ~0017668

indeed -- but it's easier to do that if we don't have an offset jumping around all the time. :)

nenolod

2013-05-25 01:33

reporter   ~0017671

Syzop, if you don't object, I'd like to do this for alpha2.

syzop

2013-05-25 15:46

administrator   ~0017675

I see talk about attack vector and split riding etc.
Of course there's an issue if IRCd clocks differ. But builtin NTP / TSCTL itself don't cause that. In fact, without NTP built-in and TSCTL you will probably have MORE clocks out of synch and MORE vulnerable networks.

Then, the other argument about clock changes, that too can still happen, as you rightfully point out. In fact a lot of these events occur on the system and not by TSCTL. (and built-in NTP is ruled out as a cause as it happens before the event loop)

But, to get back to the issue, without all the other claims:
Should we remove NTP and TSCTL, and just reject links that have a too large offset? That's a good question.

I'd like some feedback from others on this. There are PRO's and CON's...

Even just a few seconds off does already create a 'security issue', like for nick lag colliding. If one servers clock is 2 seconds off it makes it much easier.
That's why a build-in NTP client can be so helpful if you don't run system NTP. The current implementation is not that accurate, but it can be improved.
I saw a discussion on IRC to reject an offset of 600, but 600 seconds are already a gigantic time difference.

Which is another question, are you sure you want to disallow linking if apparently NTPd isn't doing it's job? It adds another point of failure in the system.

Stealth

2013-05-25 16:55

reporter   ~0017677

I like the idea of removing the internal NTP, and the servers to just drop the link if the timestamp is too far off.

However, I don't like the idea of removing TSCTL's ability to set a time offset.

Other (related) thoughts: I liked InspIRCd's idea to designate one IRCd on a network as a time server to automatically set linking servers to the correct time. I think this should only be permitted if the server is allowed to link in the first place with a time difference of nothing serious (like less than 10 seconds).

falconkirtaran

2013-05-26 00:36

reporter   ~0017678

You really don't want ircd setting the system time. ntpd is better at it, and there can be only one (do you really want them fighting over your clock?).

nenolod

2013-05-26 01:30

reporter   ~0017679

I think keeping an offset biased against NTP is fine, but we should remove TSCTL.

We should also have a hard requirement of, "oh, you can't link if you're this far out."

It's the TSCTL command that is bugging me.

Stealth

2013-05-26 06:39

reporter   ~0017681

@falconkirtaran The IRCd will not be messing with the system time, just being the time authority on the IRC network using offsets to the system timestamp.

For example: you have 4 servers, you make the "hub" server the time server. When the other servers link it sends out the current timestamp, and the linking server adjusts its time accordingly. We may want to do this multiple times to ensure the servers have the correct time (like NTP over IRC or something). The result would be the IRCds having a timestamp independent from the system time, and everyone accurate within a couple seconds without the need for sysadmin intervention.

falconkirtaran

2013-05-26 06:45

reporter   ~0017682

Last edited: 2013-05-26 06:46

View 2 revisions

We already do pretty much that. I really dislike the added complexity. I'd also like to see the concept of a hub replaced with STP, as a slightly contradictory adjunct.

syzop

2013-05-26 10:36

administrator   ~0017683

What's so complex about it?
1) The NTP timesynch. code is completely contained and doesn't interfere with the rest.
2) As for time offsets (and TSCTL), we already use a macro, TStime(), for all time stuff.. nothing complex about it. Perhaps more an issue of getting used to.
3) And regarding time going forward or backwards, which is bad (I always said), as I and you already posted you'll keep that problem if system time is adjusted.. which is one of the main reasons for time shifts already. The code dealing with that too shouldn't just be removed because 'ahh ugly, complicated, del del del', no, it fixes a real problem.

Of course, in a perfect world, none of the three are necessary, but we don't live in a perfect world.

Speaking in general: There often was a good reason to add a feature. And for removal there should even be a BETTER reason, just a 'I don't like XYZ' is often insufficient.

Note that I haven't decided on any yet ;p. Hope some more people will jump in to discuss, and feel free to comment on my points of course.

katsklaw

2013-06-08 00:52

reporter   ~0017707

well my 2 cents is this:

regards to NTP;
1> server runs ntp and thus we don't need an internal one.
2> server does not run ntp and if it's enabled, services can desync even on 127.0.0.1 (bad thing).
3> server does not run ntp, but is linked to a network that does. If services isn't local then all clear.

In the even of #1 and #2, our internal ntp should be disabled because it's not needed or it's likely to cause bad juju because of services.

In the event of #3, having an internal ntp is a benefit if the admin of the machine refuses to install ntp and we don't have to keep track of clock difference, just drift. If server admin has root, they need to be advised to run ntp and thus condition #1.

It's my humble opinion that if we keep NTP, it needs to be disabled by default and the appropriate entries in the config file need to be added so admins can see how and when they should use the feature. Also #3 in my opinion is the really the only reason to keep NTP.

If that wasn't 2 cents worth .. Please keep the change! :)

syzop

2013-06-08 11:30

administrator   ~0017709

Thanks katsklaw, scenario 2) wasn't mentioned yet before, so is one to take into account.

IMHO time must be correct, always, even if you have two clocks that are both the same amount 'off', such as on localhost. While it doesn't directly cause a server synchronization issue it does still cause silly things to happen, such as times being incorrect in 'channel created on', 'topic last changed on', and 'online since' (in WHOIS) etc. etc.

Another suggestion to consider (again, not saying this is the way to proceed, just adding a possibility!):
What we can also do is keep NTP/TSCTL and *add* the bounce functionality.
If services would support this protocol change too, then scenario 2) is handled properly as well (link is rejected).

1) Works with system ntp enabled: unreal ntp can/should be disabled, keeping it enabled shouldn't hurt anything either though (TSdiff=0)
2) Works with system ntp disabled: unreal ntp enabled will address most issues
3) Works with both system & unreal ntp disabled/out-of-order (either this system or a remote server such as services), because we would reject links if our TS & their TS differs more than XYZ.

This way we encourage everyone to use correct time, while with the original idea (if you remove unreal NTP and only do the TSdiff server reject) you still permit incorrect time in the services scenario katsklaw mentioned.

Of course what Stealth suggested is also a possibility, but it requires active intervention for the admin to configure, and is probably not done on 2-3 server networks (though, perfectly possible). Plus, the same can be achieved and is already done with TSCTL / by some services. If you run a bit larger network you hopefully know about NTP and care enough to have the time on your servers to be correct.

What katsklaw mentioned in 0004220 is that he still often sees the 'incorrect time' problem with users. This is also a factor. I was hoping that by now system NTP would be so widespread that unreal NTP would be unneeded. But apparently that is not the case.

syzop

2015-05-24 16:20

administrator   ~0018339

Added in 3.4-alpha3:
https://github.com/unrealircd/unrealircd/commit/0a42cedf77aae8bbb6769e7477e485aee3ca1340
Bounce links that have their clock too far out of sync (0004214).
Currently set at 1 minute. TODO: make configurable. This only works with newer servers as it relies on PROTOCTL TS=xyz very early in the synch.

Issue History

Date Modified Username Field Change
2013-05-25 00:30 nenolod New Issue
2013-05-25 00:34 falconkirtaran Note Added: 0017666
2013-05-25 00:38 nenolod Note Added: 0017668
2013-05-25 01:33 nenolod Note Added: 0017671
2013-05-25 15:46 syzop Note Added: 0017675
2013-05-25 15:46 syzop Status new => feedback
2013-05-25 16:55 Stealth Note Added: 0017677
2013-05-26 00:36 falconkirtaran Note Added: 0017678
2013-05-26 01:30 nenolod Note Added: 0017679
2013-05-26 06:39 Stealth Note Added: 0017681
2013-05-26 06:45 falconkirtaran Note Added: 0017682
2013-05-26 06:46 falconkirtaran Note Edited: 0017682 View Revisions
2013-05-26 10:36 syzop Note Added: 0017683
2013-06-08 00:20 Stealth Relationship added related to 0004220
2013-06-08 00:52 katsklaw Note Added: 0017707
2013-06-08 11:30 syzop Note Added: 0017709
2014-03-14 01:14 peterkingalexander Issue cloned: 0004299
2014-05-31 22:14 syzop Summary remove NTP, TSCTL, etc - just bounce links that are too far out of sync => bounce links that are too far out of sync
2014-05-31 22:14 syzop Description Updated View Revisions
2015-05-24 16:20 syzop Note Added: 0018339
2015-05-24 16:20 syzop Status feedback => resolved
2015-05-24 16:20 syzop Fixed in Version => 3.4-alpha3
2015-05-24 16:20 syzop Resolution open => fixed
2015-05-24 16:20 syzop Assigned To => syzop