Add SSL & auth support to MongoDB client #1299

tomconte · 2017-03-17T08:22:59Z

This a first stab at providing additional MongoDB connection options (issue #1281).

Turning SSL on is just an option to add in the MongoClient() constructor.

Authentication needs to be performed at the database level, so that is a separate call to authenticate().

Note: currently the MongoDB backend requires admin access (not sure this is a good idea BTW, this might not be possible in shared/hosted environments), so the user must have the right privileges/role.

Here is how to setup a test MongoDB instance:

Run MongoDB with auth:

docker run --name dev-mongo -d mongo --auth --replSet bigchain-rs

Create root user:

docker exec -it dev-mongo mongo admin
rs.initiate()
db.createUser({ user: 'tconte', pwd: 'Pass1234', roles: [ { role: "root", db: "admin" } ] });

(exit mongo shell)

docker exec -it dev-mongo mongo -u tconte -p Pass1234 admin
use bigchain
db.createUser({ user: 'bcdb', pwd: 'Pass1234', roles: [ { role: "root", db: "admin" } ] });

Now the bcdb user in the bigchain database has root role.

A couple (optional) parameters can be added to .bigchaindb to provide the new values, e.g.

        "ssl": false,
        "login": "bcdb",
        "password": "Pass1234",

krish7919 · 2017-03-17T08:55:49Z

@tomconte To clarify, this is between the pymongo driver in BigchainDB and the MongoDB instance?

codecov-io · 2017-03-17T09:05:58Z

Codecov Report

Merging #1299 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
+ Coverage   98.19%   98.19%   +<.01%     
==========================================
  Files          54       54              
  Lines        2433     2444      +11     
==========================================
+ Hits         2389     2400      +11     
  Misses         44       44

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0edb1c1...0471080. Read the comment docs.

tomconte · 2017-03-17T09:06:36Z

@krish7919 yes, so that BigchainDB can connect to a secured MongoDB instance.

ttmc · 2017-03-17T09:21:29Z

I'm not super-familiar with the code for establishing the connection to MongoDB, so I think I'll ask @vrde to take a look at this pull request too.

We'll also have to update the docs page about the BigchainDB Server settings, i.e. https://docs.bigchaindb.com/projects/server/en/latest/server-reference/configuration.html
the source of which is in docs/server/source/server-reference/configuration.md.
We'll also have to document how to set up and use authentication (as you did above in this pull request, but we don't consider the set of GitHub comments on pull requests to be our official documentation 😄 ).
I could do that documentation if you like.

Codecov is also complaining about a decline in unit test coverage...

tomconte · 2017-03-17T10:18:49Z

Looks like I need to work on these tests :)

vrde

Hi, thanks for your PR, looks pretty good! There are a couple of things to change, then we are ready to go 💃.

vrde · 2017-03-24T12:47:40Z

bigchaindb/backend/connection.py

@@ -16,7 +16,7 @@


 def connect(backend=None, host=None, port=None, name=None, max_tries=None,
-            connection_timeout=None, replicaset=None):
+            connection_timeout=None, replicaset=None, ssl=False, login=None, password=None):


The params in the function signature have higher priority than the one in the configuration. This means that if your config defines 'ssl': True, but the connect function is called with ssl=False, then False has priority. It's better to have ssl=None here. See next comment for the other part of the explanation.

vrde · 2017-03-24T12:52:01Z

bigchaindb/backend/connection.py

@@ -50,6 +50,10 @@ def connect(backend=None, host=None, port=None, name=None, max_tries=None,
    # to handle these these additional args. In case of RethinkDBConnection
    # it just does not do anything with it.
    replicaset = replicaset or bigchaindb.config['database'].get('replicaset')
+    ssl = bigchaindb.config['database'].get('ssl') if bigchaindb.config['database'].get('ssl') is not None \


This should do the trick:

ssl = ssl if ssl is not None else bigchaindb.config['database'].get('ssl', False)

It says: if ssl has a value different than none, then use that value, read the configuration otherwise. If in the configuration the value is not defined, assume False.

vrde · 2017-03-24T12:52:56Z

bigchaindb/backend/mongodb/connection.py

@@ -16,7 +16,7 @@

 class MongoDBConnection(Connection):

-    def __init__(self, replicaset=None, **kwargs):
+    def __init__(self, replicaset=None, ssl=False, login=None, password=None, **kwargs):


Same as before ssl=None.

vrde · 2017-03-24T12:53:20Z

bigchaindb/backend/mongodb/connection.py

@@ -28,6 +28,10 @@ def __init__(self, replicaset=None, **kwargs):

        super().__init__(**kwargs)
        self.replicaset = replicaset or bigchaindb.config['database']['replicaset']
+        self.ssl = bigchaindb.config['database'].get('ssl') if bigchaindb.config['database'].get('ssl') is not None \


ssl = ssl if ssl is not None else bigchaindb.config['database'].get('ssl', False)

vrde · 2017-03-24T13:44:46Z

bigchaindb/backend/mongodb/connection.py

+            if self.login is not None and self.password is not None:
+                client[self.dbname].authenticate(self.login, self.password)
+
+            return client

        # `initialize_replica_set` might raise `ConnectionFailure` or `OperationFailure`.
        except (pymongo.errors.ConnectionFailure,
                pymongo.errors.OperationFailure) as exc:
            raise ConnectionError() from exc


Unfortunately pymongo doesn't provide a specific exception when the authentication fails, but a generic OperationFailure. This might trigger a retry, but it's not the correct behavior: if credentials are wrong we want BigchainDB to fail and give an explicit error message to the user.

You don't have to care about it in this PR, some of us can do it after we merge your PR 😄

See #1336

vrde · 2017-03-27T09:56:37Z

@tomconte thanks for updating your PR. If you allow us to change your PR I can fix the test coverage and get this merged soon.

tomconte · 2017-03-27T11:18:54Z

@vrde many thanks! The "Allow edits from maintainers" option is enabled on this PR.

This reverts commit 699e615.

vrde · 2017-03-28T13:12:42Z

A couple (optional) parameters can be added to .bigchaindb to provide the new values, e.g.

I addressed it in 699e615, but I noticed I was adding too much scope, so I reverted it in 0471080. I've created #1346 to keep track of this issue.

chrisckchang · 2017-04-25T15:59:03Z

Hi folks, not sure if this is the right place for this comment but the best practice for connecting to replica sets is to include the host&port information for each primary-electable replica set member (no arbiter or hidden secondary nodes) in the connection string.

You may want to consider supporting the MongoDB connection URI, which can be passed directly into the driver: https://docs.mongodb.com/manual/reference/connection-string/. For example, Meteor.js allows users to pass an entire URI via CLI: https://github.com/meteor/meteor/blob/master/tools/cli/commands.js#L374. You can then use the pymongo uri_parser lib if you need specific parts of the URI.

ttmc · 2017-04-26T09:52:48Z

@vrde Maybe we could create a new issue to look into using the MongoDB connection URI as suggested by @chrisckchang (so that idea doesn't get lost in the comments of a merged PR)? ( #1299 (comment) )

vrde · 2017-04-26T14:38:28Z

@chrisckchang wrote:

Hi folks, not sure if this is the right place for this comment but the best practice for connecting to replica sets is to include the host&port information for each primary-electable replica set member (no arbiter or hidden secondary nodes) in the connection string.

AFAIK having multiple servers in the connection string allows the client to have multiple choices if one server is not reachable. Since BigchainDB is a federated system, when the driver fails to connect to the mongod it is supposed to talk with, then the driver should not try to connect to other instances, and BigchainDB should ~~not~~ fail to start.

tomconte added 2 commits March 14, 2017 14:23

SSL connection support

696dbe7

Authentication support

08f040d

tomconte added 2 commits March 17, 2017 10:01

Fix unit test

8526246

Formatting

3b1e6ad

ttmc requested review from vrde and ttmc March 17, 2017 09:21

Fix unit test

550b9cb

vrde suggested changes Mar 24, 2017

View reviewed changes

Fix ssl param default value

58d80e9

vrde approved these changes Mar 27, 2017

View reviewed changes

vrde added 3 commits March 28, 2017 11:24

Improve test coverage

441ad91

Add ssl, login, and passwd to configure command

699e615

Revert "Add ssl, login, and passwd to configure command"

0471080

This reverts commit 699e615.

vrde mentioned this pull request Mar 28, 2017

Add ssl, login, and password fields to configuration #1346

Closed

vrde merged commit 0471080 into bigchaindb:master Mar 28, 2017

vrde mentioned this pull request Mar 28, 2017

Add missing config options #1348

Merged

vrde mentioned this pull request Apr 5, 2017

Add support for MongoDB authentication #1371

Closed

krish7919 mentioned this pull request Jun 26, 2017

Add MongoDB connection options for SSL and authentication #1281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSL & auth support to MongoDB client #1299

Add SSL & auth support to MongoDB client #1299

tomconte commented Mar 17, 2017

krish7919 commented Mar 17, 2017 •

edited

codecov-io commented Mar 17, 2017 •

edited

tomconte commented Mar 17, 2017

ttmc commented Mar 17, 2017 •

edited

tomconte commented Mar 17, 2017

vrde left a comment

vrde Mar 24, 2017

vrde Mar 24, 2017

vrde Mar 24, 2017

vrde Mar 24, 2017

vrde Mar 24, 2017

vrde commented Mar 27, 2017

tomconte commented Mar 27, 2017

vrde commented Mar 28, 2017

chrisckchang commented Apr 25, 2017 •

edited

ttmc commented Apr 26, 2017

vrde commented Apr 26, 2017 •

edited by krish7919

Add SSL & auth support to MongoDB client #1299

Add SSL & auth support to MongoDB client #1299

Conversation

tomconte commented Mar 17, 2017

krish7919 commented Mar 17, 2017 • edited

codecov-io commented Mar 17, 2017 • edited

Codecov Report

tomconte commented Mar 17, 2017

ttmc commented Mar 17, 2017 • edited

tomconte commented Mar 17, 2017

vrde left a comment

Choose a reason for hiding this comment

vrde Mar 24, 2017

Choose a reason for hiding this comment

vrde Mar 24, 2017

Choose a reason for hiding this comment

vrde Mar 24, 2017

Choose a reason for hiding this comment

vrde Mar 24, 2017

Choose a reason for hiding this comment

vrde Mar 24, 2017

Choose a reason for hiding this comment

vrde commented Mar 27, 2017

tomconte commented Mar 27, 2017

vrde commented Mar 28, 2017

chrisckchang commented Apr 25, 2017 • edited

ttmc commented Apr 26, 2017

vrde commented Apr 26, 2017 • edited by krish7919

krish7919 commented Mar 17, 2017 •

edited

codecov-io commented Mar 17, 2017 •

edited

ttmc commented Mar 17, 2017 •

edited

chrisckchang commented Apr 25, 2017 •

edited

vrde commented Apr 26, 2017 •

edited by krish7919