Project

General

Profile

Actions
Copy link

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 12 years ago. Updated about 12 years ago.

Status:
Released
Priority:
3
Assignee:
François ARMAND
Category:
Web - Nodes & inventories
Target version:
2.4.0~beta5
Pull Request:
UX impact:
Suggestion strength:
User visibility:
Effort required:
Name check:
Fix check:
Regression:

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
Actions
Copy link
 #1

Updated by Jonathan CLARKE over 12 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?

Actions
Copy link
 #2

Updated by Jonathan CLARKE over 12 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
Actions
Copy link
 #3

Updated by Jean VILVER over 12 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 ?

Actions
Copy link
 #4

Updated by Jonathan CLARKE over 12 years ago

  • Target version changed from 2.4.0~beta5 to 2.4.0~rc1
Actions
Copy link
 #5

Updated by Jonathan CLARKE over 12 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.

Actions
Copy link
 #6

Updated by Jonathan CLARKE over 12 years ago

  • Target version changed from 2.4.0~rc1 to 2.4.0~rc2
Actions
Copy link
 #7

Updated by Nicolas PERRON over 12 years ago

  • Target version changed from 2.4.0~rc2 to 2.4.0~rc1
Actions
Copy link
 #8

Updated by Jean VILVER over 12 years ago

  • Status changed from 2 to Pending technical review
  • % Done changed from 0 to 100
Actions
Copy link
 #9

Updated by Jean VILVER over 12 years ago

The result in the image attached.

Actions
Copy link
 #10

Updated by François ARMAND over 12 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.

Actions
Copy link
 #11

Updated by François ARMAND over 12 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 ?

Actions
Copy link
 #12

Updated by Jean VILVER over 12 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.

Actions
Copy link
 #13

Updated by François ARMAND about 12 years ago

  • Status changed from In progress to Pending technical review
Actions
Copy link
 #14

Updated by Jonathan CLARKE about 12 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.

Actions
Copy link
 #15

Updated by François ARMAND about 12 years ago

  • Status changed from In progress to Pending technical review
Actions
Copy link
 #16

Updated by Jonathan CLARKE about 12 years ago

  • Assignee changed from Jean VILVER to François ARMAND
Actions
Copy link
 #17

Updated by Jonathan CLARKE about 12 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.

Actions
Copy link
 #18

Updated by Nicolas CHARLES about 12 years ago

  • Status changed from Discussion to Pending technical review
Actions
Copy link
 #19

Updated by Nicolas CHARLES about 12 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

Actions
Copy link
 #20

Updated by Nicolas CHARLES about 12 years ago

  • Status changed from Pending technical review to 10

This is valid, thank you Francois

Actions
Copy link
 #21

Updated by Jonathan CLARKE about 12 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.

Actions
Copy link
 #22

Updated by Jonathan CLARKE about 12 years ago

  • Target version changed from 2.4.0~rc1 to 2.4.0~beta5
Actions
Copy link

Also available in: Atom PDF