User story #2791
closed
When a node is accepted, an error or a warning should be displayed if its hostname or ip is already present
Added by Nicolas PERRON over 11 years ago. Updated over 11 years ago.
Description
In Rudder, it is possible to accept any node, even if they have the same hostname or ip than another. This should not happen or at least should not be unnoticed.
Besides, in CFEngine, the ACL used to prevent a node to access another node folder is defined by the hostname which result in the last node with a hostname already used by an access denied without explicit message.
Files
| prevent_node_with_used_name.png (43.7 KB) prevent_node_with_used_name.png | Jean VILVER, 2012-09-21 16:24 |
Updated by Jonathan CLARKE over 11 years ago
- Status changed from New to Discussion
- Target version changed from 24 to 2.4.0~beta4
This looks like a pretty serious problem to me - potentially a security issue?
I think this should be adressed in 2.4 if possible (or 2.3?). Any ideas, opinions?
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Discussion to 2
- Assignee changed from François ARMAND to Jean VILVER
- Target version changed from 2.4.0~beta4 to 2.4.0~beta5
Updated by Jean VILVER over 11 years ago
- Status changed from 2 to Discussion
- Assignee changed from Jean VILVER to Jonathan CLARKE
I'm thinking to put the duplicate node (with a hostname or a IP already present) in the Accept new node table, and to refuse when the user try to add it to rudder.
Is that a good solution ? Or do you prefer a different behavior ?
Updated by Jonathan CLARKE over 11 years ago
- Target version changed from 2.4.0~beta5 to 2.4.0~rc1
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Discussion to 2
- Assignee changed from Jonathan CLARKE to Jean VILVER
Jean VILVER wrote:
I'm thinking to put the duplicate node (with a hostname or a IP already present) in the Accept new node table, and to refuse when the user try to add it to rudder.
Is that a good solution ? Or do you prefer a different behavior ?
This could be a quick simple solution, and is sufficiently non intrusive to be a good approach for this beta version. Please make sure to include a clear warning message to the user, something like "The node XXX could not be accepted because another node in Rudder already has the same hostname or IP address: node YYY", replacing XXX and YYY with the real node names (XXX for the one to not accept, and YYY for the one that already exists and conflicts).
For a future version (2.5?), a nicer approach would be to have a new tab in the "Accept new nodes" screen for these "abnormal" cases, like the "Unusual changes" tab discussed in #623.
Updated by Jonathan CLARKE over 11 years ago
- Target version changed from 2.4.0~rc1 to 2.4.0~rc2
Updated by Nicolas PERRON over 11 years ago
- Target version changed from 2.4.0~rc2 to 2.4.0~rc1
Updated by Jean VILVER over 11 years ago
- Status changed from 2 to Pending technical review
- % Done changed from 0 to 100
Applied in changeset 43414f2ca0892325c6f55d3d1e4b67528ff319e6.
Updated by François ARMAND over 11 years ago
There is a lot of things not OK with that commit:
- case processed in added lines 316/318 are already handled in the next lines
- in the main logic, the exact same research ("get all existing nodes", i.e: nodeInfoService.getAllIds/nodeInfoService.getNodeInfo(id) ) is done two times two lines apparts
- there is an assigned val "m" nether used
- line 749/751, there is a return in place of a "for comprehension" building
- around the same line, there is ".get" used, that should never either being used
So, I propose to revert that commit and rewrite it.
Updated by François ARMAND over 11 years ago
Moreover, I don't know what is a duplicated IP. Do we really want to refuse a node because some node is already saved with IP "127.0.0.1" ?
We should probably filter out it, but is it the only case where it does not make sense to refuse ?
What about 192.168.* ? That seems less a problem, as Rudder only serve on one interface, IP should be unique from there.
What about IPv6 loopback ?
Updated by Jean VILVER over 11 years ago
- Status changed from Pending technical review to In progress
François ARMAND wrote:
[...]
So, I propose to revert that commit and rewrite it.
Ok, I'm working on it.
Updated by François ARMAND over 11 years ago
- Status changed from In progress to Pending technical review
Applied in changeset 5c5ec1c02782591295302fe30b15a7d5b783f1c1.
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Pending technical review to In progress
This was closed in error due to a merge containing the "Fixes #xxxx" commit. It still needs doing.
Updated by François ARMAND over 11 years ago
- Status changed from In progress to Pending technical review
Applied in changeset 14bb15ed1264f65a121a23a3bc63dba8e148e8cb.
Updated by Jonathan CLARKE over 11 years ago
- Assignee changed from Jean VILVER to François ARMAND
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Pending technical review to Discussion
François, I just looked at this commit, but I didn't see anything that ignores duplicate IPs if they are in the 127.0.0.0/8 range. Is this check indeed implemented? It is very important that it is, otherwise we'll only be able to accept one node.
Updated by Nicolas CHARLES over 11 years ago
- Status changed from Discussion to Pending technical review
Updated by Nicolas CHARLES over 11 years ago
Jonathan CLARKE wrote:
François, I just looked at this commit, but I didn't see anything that ignores duplicate IPs if they are in the 127.0.0.0/8 range. Is this check indeed implemented? It is very important that it is, otherwise we'll only be able to accept one node.
In the preAccept, there is a filter on LocalAddress, LoopbackAddress and Multicast, so this works as expected
Updated by Nicolas CHARLES over 11 years ago
- Status changed from Pending technical review to 10
This is valid, thank you Francois
Updated by Jonathan CLARKE over 11 years ago
- Status changed from 10 to Released
Nicolas CHARLES wrote:
Jonathan CLARKE wrote:
François, I just looked at this commit, but I didn't see anything that ignores duplicate IPs if they are in the 127.0.0.0/8 range. Is this check indeed implemented? It is very important that it is, otherwise we'll only be able to accept one node.
In the preAccept, there is a filter on LocalAddress, LoopbackAddress and Multicast, so this works as expected
Right, missed that, thanks for the pointer.
I checked these methods, and I agree that they do what we want, after having to read the source code (see http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/Inet4Address.java). However, I'd like to note that the method "isLocalAddress" is extremely poorly named, as that method doesn't answer the question "is this a local address", but rather the question "is this the special wildcard address 0.0.0.0?". This is fine, but had me worried, because we don't want to ignore IPs that are local to the Rudder server - on the contrary.
Updated by Jonathan CLARKE over 11 years ago
- Target version changed from 2.4.0~rc1 to 2.4.0~beta5