fix persistence error mongo authentication #1851

srrspace · 2020-04-01T18:51:48Z

Please check this PR that solve the issue #1849 ([cygnus][bug] Persistence Error Mongo Authentication and side effect #1849)

I made exhaustive tests and it work fine with various configuration and various users created by different way and with several data source (iotagents) send to the Orion Context Broker.

Now the databases (based on "Fiware-Service") and corresponding collections (based on "Fiware-ServicePath") are created automatically on the fly without manual action on mongodb when cygnus receive the data by notification (i.e. by subscription).

With this modification using auth_source option for mongo authentication, there is no side effect when more than one data source is received with different "Fiware-Service" and different "Fiware-ServicePath" at the same time or concurrently.

The "agent_1.conf" must add the following entry as example:

# a valid user in the MongoDB server (or empty if authentication is not enabled in MongoDB)
cygnus-ngsi.sinks.mongo-sink.mongo_username = TheUser
# password for the user above (or empty if authentication is not enabled in MongoDB)
cygnus-ngsi.sinks.mongo-sink.mongo_password = ThePassword
# Mongo option to specify which common user database source to use for authentication
cygnus-ngsi.sinks.mongo-sink.mongo_auth_source = TheAuthSource

The new entry in the agent configuration file for mongo sink is:

# Mongo option to specify which common user database source to use for authentication
cygnus-ngsi.sinks.mongo-sink.mongo_auth_source = TheAuthSource

If this new entry is not specified in the agent configuration file and authentication with the credentials is enabled, then the cygnus will work the same as before with the side effect as described in the issue (because it will use the corresponding dbName as before). I mean if we want to access mongodb with authentication mechanism auth_source must be specified and all will work fine.

Usually mongo_auth_source could be "admin" but could be another database where the user created must have the correct rights and correct privileges to access read/write the other DB based on the "Fiware-Service" value (i.e. DB name starting with "sth_").

The pertinent user should be created in the auth source DB:

For example:
use admin

db.createUser(
  {
    user: "TheUser",
    pwd: "ThePassword",
    roles : ["readWriteAnyDatabase"],
    mechanisms : [
      "SCRAM-SHA-1",
      "SCRAM-SHA-256"
    ]
  }
)

Or to adding more restrictions on the DB, if you know in advance the name of the database based on the "Fiware-Service" value:

db.createUser(
  {
    user: "TheUser",
    pwd: "ThePassword",
    roles : [
      {
        role : "readWrite",
        db : "sth_service1"
      },
      {
        role : "readWrite",
        db : "sth_service2"
      }
    ],
    mechanisms : [
      "SCRAM-SHA-1",
      "SCRAM-SHA-256"
    ]
  }
)

Notice that just after creating the user in the DB auth source, it is recommended to restart the mongo database, just in case, to apply the changes correctly (by using "sudo service mongod restart").

The only file modifies are:

cygnus-common - MongoBackendImpl.java
cygnus-ngsi - NGSIMongoBaseSink.java

fgalan · 2020-04-02T09:09:43Z

Thank you for your contribution! Some comments:

Please modify documentation according to the changes in the PR. For instance, if you have added a new configuration parameter for the mongo sink (not sure...), then it should be properly described in the sink section in the documentation.
I understand this PR covers the mongo sink authentication. However, the sth sink works in a very similar way (it is also persisting information in MongoDB, but with a different data structure). Is your PR solving also the auth in sth sink?
An entry in CHANGES_NEXT_RELEASE describing the change (and citing the issue number) is needed. Have a look to current CHANGEX_NEXT_RELEASE in master to see which style we use.

IvanHdzC · 2020-04-02T10:19:32Z

cygnus-common/src/main/java/com/telefonica/iot/cygnus/backends/mongo/MongoBackendImpl.java

+     * @param mongoAuthSource
+     * @param dataModel
+     */
+    public MongoBackendImpl(String mongoHosts, String mongoUsername, String mongoPassword,


There is no need to overload the costructor in this case, since mongoAuthSource has a not null validation on getDatabase.

From my point of view it is better to have this new constructor to avoid side effect and maintain the backward compatibility. There is no problem to have 2 constructors in OOP and it is common to do it allowing to initialize and/or construct objects with different parameters depending of the use cases and future needs and maintaining the interface.

I agree, but the constructor is actually only used in NGSIMongoBaseSink and in tests, so there is no need to have both.

I agree with @IvanHdzC . The constructor is only used in one point of the code, so lest's adapt constructor and calling code better than adding a second constructor. In addition, the lesser constructor we have, the easier it would be to have good code coverage in tests.

IvanHdzC · 2020-04-02T10:24:19Z

cygnus-ngsi/src/main/java/com/telefonica/iot/cygnus/sinks/NGSIMongoBaseSink.java

        mongoHosts = context.getString("mongo_hosts", "localhost:27017");
        LOGGER.debug("[" + this.getName() + "] Reading configuration (mongo_hosts=" + mongoHosts + ")");
        mongoUsername = context.getString("mongo_username", "");
        LOGGER.debug("[" + this.getName() + "] Reading configuration (mongo_username=" + mongoUsername + ")");
        // FIXME: mongoPassword should be read as a SHA1 and decoded here
        mongoPassword = context.getString("mongo_password", "");
        LOGGER.debug("[" + this.getName() + "] Reading configuration (mongo_password=" + mongoPassword + ")");
-
+        mongoAuthSource = context.getString("mongo_auth_source", "");


I thing it's more usuall that the default authSource is "admin". It would be better if the default value of mongo_auth_source is "admin".

IMHO authSource is necessary to be configured and not fixed as default constant value to admin for security reason. Admin is a sensible and critical database with rights and high privileges. Giving the possibility to configure the authentication database will allow to provide other specific databases with some restrictions and with just the necesary privileges with a set of user depending of the uses and applications needed and it is more secure tha using only the admin database.

You are right, I misunderstood the "defaultauthdb" value, it actually is the db where the connection is created.

NTC

IvanHdzC · 2020-04-02T10:27:58Z

cygnus-common/src/main/java/com/telefonica/iot/cygnus/backends/mongo/MongoBackendImpl.java

+                if ((mongoAuthSource != null) && !mongoAuthSource.isEmpty()) {
+                    authSource = mongoAuthSource;
+                } else {
+                    authSource = dbName;


Not sure if the dbName is the right authSource in case it is not specified.

Before this modification dbName was the value used in the previous version. If authSource is not configured then the behavior of the code is the same as before maintaining the backward compatibility. From my point of view it is better to keep the code as it is and not setting authSource as a constant to "admin".

I misunderstood the "defaultauthdb" value, it actually is the db where the connection is created.

NTC

Yes, dbName is one thing and authSource is another. The parameter dbName is the name of the database to create and based on "Fiware-Service" where the collections will be stored. And authSource is used to specify a user DB where the credentials are defined, as documented by Mongo and for the credentials objects. These are 2 different things.
dbName is used in client.getDatabase(dbName) (class MongoClient) and authSource is used by credential object (MongoCredential) if only there is a need of authentication.

IvanHdzC · 2020-04-02T10:29:34Z

Missing tests for this PR.

IvanHdzC · 2020-04-02T10:53:00Z

cygnus-common/src/main/java/com/telefonica/iot/cygnus/backends/mongo/MongoBackendImpl.java

+    /**
+     * Gets the mongo auth_source
+     * @return
+     */
+    public String getAuthSource() {
+        return mongoAuthSource;
+    } // getAuthSource
+
+    /**
+     * Set the mongo auth_source
+     * @param authSource DB used for mongo authentication
+     */
+    public void setAuthSource(String authSource) {
+        if (authSource != null) {
+            this.mongoAuthSource = authSource;
+        } else {
+            this.mongoAuthSource = "";
+        }
+    } // setAuthSource
+


is this ever used?

Again, if somebody or other piece of code (allowing other new sink in the future to use mongo as a DB with authentication), and want to use the first constructor without authSource in parameter, it will have the opportunity to set the authSource after to build the backend object using the setAuthSource(...) operation when needed and in some place in the code. It is typical and best practices to do it in OOP (overload constructors and get/set operations), and give more possibilities.
Notice that in this case mongoAuthSource shoud not be annotated as "final" in the declaration.

get/setAuthSource(...) could be used, if somebody build the object without authSource parameter and if after they want to initialize this parameter.
This is what I tested successfully and put in comment as documentation (see start() operation), and use of the new constructor.

BTW, What does it mean "NTC" and "LGTM"?

NTC = Nothing to change
LGTM = Looks good to me

More on "the NTC and LGTM protocol" review protocol here: https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/contribution_guidelines.md#pull-request-protocol

srrspace · 2020-04-02T12:01:06Z

@fgalan:

Yes I will update the documentation in this cygnus repository (but do not know to do it in readthedocs.org).

And yes there is new configuration parameters:

In MongoSink:

# Mongo option to specify which common user database source to use for authentication
cygnus-ngsi.sinks.mongo-sink.mongo_auth_source =

And probably In STHSink:

# Mongo option to specify which common user database source to use for authentication
cygnus-ngsi.sinks.sth-sink.mongo_auth_source =

This PR covers the mongo sink authentication issues.
According to my analysis of the code of STH Sink, YES this PR solve also the authentication issues in STH Sink.

Yes, I will update CHANGES_NEXT_RELEASE

srrspace · 2020-04-02T14:34:19Z

In order to be aligned to your thoughts concerning the backend constructor I will review the code removing the old backend constructor and removing get/setAuthSource.

But impact on the tests, i.e. need to modify "MongoBackendImplTest.java"

…ameter to built Mongo backend

…auth_source parameter

srrspace · 2020-04-02T18:00:48Z

Please could you check the PR with the new commit related to the code, documentation, docker, agent conf,...

IvanHdzC

LGTM

fgalan · 2020-04-03T14:52:27Z

doc/cygnus-ngsi/flume_extensions_catalogue/ngsi_mongo_sink.md

@@ -332,6 +332,7 @@ A configuration example could be:
    cygnus-ngsi.sinks.mongo-sink.mongo_hosts = 192.168.80.34:27017
    cygnus-ngsi.sinks.mongo-sink.mongo_username = myuser
    cygnus-ngsi.sinks.mongo-sink.mongo_password = mypassword
+    cygnus-ngsi.sinks.mongo-sink.mongo_auth_source = admin
    cygnus-ngsi.sinks.mongo-sink.db_prefix = cygnus_


The new parameter mongo_auth_source should be also added to the configuration table included in this file .md file.

fgalan · 2020-04-03T14:52:33Z

doc/cygnus-ngsi/flume_extensions_catalogue/ngsi_sth_sink.md

@@ -330,6 +330,7 @@ A configuration example could be:
    cygnus-ngsi.sinks.sth-sink.mongo_hosts = 192.168.80.34:27017
    cygnus-ngsi.sinks.sth-sink.mongo_username = myuser
    cygnus-ngsi.sinks.sth-sink.mongo_password = mypassword
+    cygnus-ngsi.sinks.sth-sink.mongo_auth_source = admin


The new parameter mongo_auth_source should be also added to the configuration table included in this file .md file.

The new parameter mongo_auth_source have been added to the configuration table for both sinks.

CHANGES_NEXT_RELEASE

fgalan · 2020-04-03T14:58:27Z

Please could you check the PR with the new commit related to the code, documentation, docker, agent conf,...

The PR looks good after the modification :)

I provided some extra comments (in CHANGEX_NEXT_RELEASE and documentation files). Once they get addressed I think the PR could be merged.

With regards to readthedocs.org don't worry about it. RTD is generated automatically from .md files, once the PR is merged.

CHANGES_NEXT_RELEASE

…n table

srrspace · 2020-04-06T15:03:54Z

Please could you check if now this PR is OK to be merged.
Thanks

IvanHdzC

LGTM

fgalan · 2020-04-13T15:27:23Z

Please could you check if now this PR is OK to be merged.
Thanks

I think the PR is mostly fine. However, I'm still seeing two suggesiton in the CHANGES_NEXT_RELEASE that hasn't been addressed. Please confirm them (buttom "Commit suggestion") or explain why do you think are not valid, please.

Co-Authored-By: Fermín Galán Márquez <[email protected]>

srrspace · 2020-04-14T08:32:29Z

I confirmed the commit suggestion. Please check the PR to proceed with the merge.

fgalan · 2020-04-14T10:06:30Z

I confirmed the commit suggestion. Please check the PR to proceed with the merge.

I think there is still one pending:

Co-Authored-By: Fermín Galán Márquez <[email protected]>

fgalan

LGTM. Thanks!

fix persistence error mongo authentication

e0ebd92

IvanHdzC reviewed Apr 2, 2020

View reviewed changes

Said Rahma added 6 commits April 2, 2020 18:36

Update CHANGES_NEXT_RELEASE

99025be

Fix persistence error mongo authentication adding mongoAuthSource par…

59b52cf

…ameter to built Mongo backend

Add a new mongoAuthSource parameter to create the Mongo backend

2a8ddff

Update the documentation to reference the mongo_auth_source parameter

3de97a8

Update docker file, configuration, entrypoint to reference the mongo_…

83e6db0

…auth_source parameter

Update cygnus-ngsi agent configuration template, test files

773eb00

srrspace mentioned this pull request Apr 3, 2020

added authSource and Replicaset to the ngsi_sink #1852

Closed

IvanHdzC approved these changes Apr 3, 2020

View reviewed changes

fgalan reviewed Apr 3, 2020

View reviewed changes

CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved

fgalan reviewed Apr 3, 2020

View reviewed changes

CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved

Said Rahma added 2 commits April 6, 2020 16:52

Update the documentation, mongo_auth_source added to the configuratio…

c3a06b5

…n table

Update the documentation, mongo_auth_source added to the configuratio…

83c5ddb

…n table

srrspace requested review from IvanHdzC and fgalan April 8, 2020 09:12

IvanHdzC approved these changes Apr 8, 2020

View reviewed changes

Update CHANGES_NEXT_RELEASE

c19f6df

Co-Authored-By: Fermín Galán Márquez <[email protected]>

Update CHANGES_NEXT_RELEASE

75914ad

Co-Authored-By: Fermín Galán Márquez <[email protected]>

fgalan approved these changes Apr 14, 2020

View reviewed changes

fgalan merged commit a4e1807 into telefonicaid:master Apr 14, 2020

fgalan mentioned this pull request Apr 14, 2020

[cygnus][bug] Persistence Error Mongo Authentication and side effect #1849

Closed

srrspace deleted the fix/1849_persistence_error_mongo_authentication branch April 22, 2020 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix persistence error mongo authentication #1851

fix persistence error mongo authentication #1851

srrspace commented Apr 1, 2020 •

edited

Loading

fgalan commented Apr 2, 2020

IvanHdzC Apr 2, 2020

srrspace Apr 2, 2020

IvanHdzC Apr 2, 2020

fgalan Apr 2, 2020

IvanHdzC Apr 2, 2020 •

edited

Loading

srrspace Apr 2, 2020

IvanHdzC Apr 2, 2020

IvanHdzC Apr 2, 2020

srrspace Apr 2, 2020

IvanHdzC Apr 2, 2020 •

edited

Loading

srrspace Apr 2, 2020

IvanHdzC commented Apr 2, 2020

IvanHdzC Apr 2, 2020

srrspace Apr 2, 2020

srrspace Apr 2, 2020

IvanHdzC Apr 2, 2020

fgalan Apr 2, 2020

srrspace commented Apr 2, 2020

srrspace commented Apr 2, 2020

srrspace commented Apr 2, 2020

IvanHdzC left a comment

fgalan Apr 3, 2020

fgalan Apr 3, 2020

srrspace Apr 6, 2020

fgalan commented Apr 3, 2020

srrspace commented Apr 6, 2020

IvanHdzC left a comment

fgalan commented Apr 13, 2020

srrspace commented Apr 14, 2020

fgalan commented Apr 14, 2020

fgalan left a comment

fix persistence error mongo authentication #1851

fix persistence error mongo authentication #1851

Conversation

srrspace commented Apr 1, 2020 • edited Loading

fgalan commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanHdzC Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanHdzC Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanHdzC commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srrspace commented Apr 2, 2020

srrspace commented Apr 2, 2020

srrspace commented Apr 2, 2020

IvanHdzC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Apr 3, 2020

srrspace commented Apr 6, 2020

IvanHdzC left a comment

Choose a reason for hiding this comment

fgalan commented Apr 13, 2020

srrspace commented Apr 14, 2020

fgalan commented Apr 14, 2020

fgalan left a comment

Choose a reason for hiding this comment

srrspace commented Apr 1, 2020 •

edited

Loading

IvanHdzC Apr 2, 2020 •

edited

Loading

IvanHdzC Apr 2, 2020 •

edited

Loading