fix(update): re execute missing PHP update from 2.8.27 #7434
Conversation
|
branch rebased and requested changes applied ;) |
| ); | ||
|
|
||
| if ($result->rowCount() == 0) { | ||
| $uniqueKey = md5(uniqid(rand(), true)); |
There was a problem hiding this comment.
MD5 is broken, uniqid() is not cryptographically secure and I do not even speak of rand().
WTF is this random number generation ? It is either over kill or totally inadequate. What is appKey used for ?
There was a problem hiding this comment.
@ganoze I use exactly was it was plained for Remote Server in 18.10.0 (https://github.com/centreon/centreon/blob/master/www/install/php/Update-18.10.0.php)
There was a problem hiding this comment.
OK but why ? What are the security implications of the appKey ?
There was a problem hiding this comment.
Is mandatory to use Remote Server but not used really....
There was a problem hiding this comment.
Is this a joke ? I am reviewing some useless stuff ? If so, maybe instead of adding new code we might instead remove dead code ?
There was a problem hiding this comment.
The appKey is an extra identifier for remote server, because naigos_server table is related to remote_server by IP and sometimes that is not enough. This identifier is not used for authentication, validation, or other activity, and is nothing more than an additional ID of a server for internal recognition and furthermore is not sendable/readable by the end user, so for now I believe we can leave it as it is. I do agree the logic to generate keys can be changed in the future, but that is not the subject of this PR. The PR is blocking a release and should go through. Any changes to the key generations can be groomed as a change task and executed as soon as the priority comes to that.
There was a problem hiding this comment.
Still I am afraid that either this identifier is 1) useless or 2) overly complex or 3) used in some kind of unclear authorization scheme that is a potential security breach (most likely).
I am also afraid that if this PR go through, the technical debt added by it will never be processed.
There was a problem hiding this comment.
I will create a ticket to manage this technical debt
lpinsivy
force-pushed
the
continue-pr-7416
branch
from
4f75013 to
6e58f9d
Compare
Description
Complete the PR #7416
But the previous PR doesn't make the job for platform already in Centreon 18.10.4 version migrating from 2.8.27.
Type of change
Target serie
How this pull request can be tested ?
Install a Centreon 3.4.x version (Centreon Web 2.8.27)
In centreon.informations table, 'appKey, isCentral & isRemote entries are missing
Update to 18.10.5, information must be present.
Checklist
Community contributors & Centreon team
Centreon team only