connection: Add authentication mechanism support. #905 #1333

zeezdev · 2016-07-06T09:04:14Z

It is necessary to work with the mongodb-cluster and support authentication X.509, or other non-default mechamism.

This change is

…09 without password)

Conflicts: mongoengine/connection.py

wojcikstefan

Thanks for this @zeezdev ! I added some comments to the code. Let me know what you think.

wojcikstefan · 2016-11-30T01:53:41Z

mongoengine/connection.py

    from pymongo import MongoReplicaSetClient
    READ_PREFERENCE = False

+if IS_PYMONGO_27:


This could use a comment - why do we need to explicitly specify "MONGODB-CR" as the default authentication mechanism for PyMongo v2.7, while all the other PyMongo versions can use the "DEFAULT"?

mongoengine is supported pymongo >= 2.7. Is not it?
for pymogo v2.7 is required authmechanism = 'MONGODB-CR' by default:

https://github.com/mongodb/mongo-python-driver/blob/v2.7/pymongo/mongo_client.py#L372

While all the other pymongo versions can use the "DEFAULT", It is pymongo requires:

https://github.com/mongodb/mongo-python-driver/blob/v2.8/pymongo/mongo_client.py#L381
https://github.com/mongodb/mongo-python-driver/blob/v2.9/pymongo/mongo_client.py#L431

So in PyMongo v2.7 (https://github.com/mongodb/mongo-python-driver/blob/v2.7/pymongo/mongo_client.py#L372) I see:

mechanism = options.get('authmechanism', 'MONGODB-CR')

and in PyMongo v2.8+ I see:

mechanism = options.get('authmechanism', 'DEFAULT')

It seems that PyMongo v2.7 already defaults to MONGODB-CR and v2.8+ defaults to DEFAULT if authmechanism is not specified. I don't understand then why we have to explicitly specify it in MongoEngine... It seems that it should be enough to not pass it to PyMongo at all unless it was specified in the config. Am I missing something?

wojcikstefan · 2016-11-30T01:57:02Z

mongoengine/connection.py

-                            source=conn_settings['authentication_source'])
+                            source=conn_settings['authentication_source'],
+                            mechanism=conn_settings['authentication_mechanism']
+                            )


this closing parenthesis can be at the end of the previous line, i.e. mechanism=conn_settings['authentication_mechanism']).

wojcikstefan · 2016-11-30T01:57:43Z

mongoengine/connection.py

        conn_settings = _connection_settings[alias]
        db = conn[conn_settings['name']]
        # Authenticate if necessary
-        if conn_settings['username'] and conn_settings['password']:


Why is it ok to drop the password check here?

Because when use MONGODB-X509 authentication mechanism not need specify password argument, only username.

http://api.mongodb.com/python/3.0b0/examples/authentication.html#mongodb-x509

While I agree that it is necessary to check the username for other authentication mechanisms.

wojcikstefan · 2016-11-30T02:01:07Z

mongoengine/python_support.py

 import pymongo


+IS_PYMONGO_27 = False


I think we could be more explicit with this test. For example PyMongo v1.7, v2.6, etc. would make IS_PYMONGO_27 true right now (no matter if they actually exist/are used in the wild).

How about:

IS_PYMONGO_27 = pymongo.version_tuple[0] == 2 and pymongo.version_tuple[1] == 7

… conn_settings for connections with not 'MONGODB-X509' mechanism

zeezdev · 2016-11-30T12:18:43Z

mongoengine/python_support.py

    IS_PYMONGO_3 = False
-    if pymongo.version_tuple[1] < 8:
-        IS_PYMONGO_27 = True
+    IS_PYMONGO_27 = pymongo.version_tuple[0] == 2 and pymongo.version_tuple[1] == 7


I change it how @wojcikstefan showed.

zeezdev · 2016-11-30T12:20:28Z

mongoengine/connection.py

        db = conn[conn_settings['name']]
        # Authenticate if necessary
-        if conn_settings['username']:
+        if (conn_settings['authentication_mechanism'] == 'MONGODB-X509' and conn_settings['username']) or\


Check 'username' only for connections with 'MONGODB-X509' authentication mechanism. For others 'username' and 'password' are checked.

zeezdev · 2016-11-30T12:22:25Z

mongoengine/connection.py

                            source=conn_settings['authentication_source'],
-                            mechanism=conn_settings['authentication_mechanism']
-                            )
+                            mechanism=conn_settings['authentication_mechanism'])


Removed newline here.

wojcikstefan

Thanks for your comments @zeezdev, I think that wee can actually make this code simpler and rely on the defaults that PyMongo itself imposes.

wojcikstefan · 2016-12-01T01:49:35Z

mongoengine/connection.py

    from pymongo import MongoReplicaSetClient
    READ_PREFERENCE = False

+if IS_PYMONGO_27:


So in PyMongo v2.7 (https://github.com/mongodb/mongo-python-driver/blob/v2.7/pymongo/mongo_client.py#L372) I see:

mechanism = options.get('authmechanism', 'MONGODB-CR')

and in PyMongo v2.8+ I see:

mechanism = options.get('authmechanism', 'DEFAULT')

It seems that PyMongo v2.7 already defaults to MONGODB-CR and v2.8+ defaults to DEFAULT if authmechanism is not specified. I don't understand then why we have to explicitly specify it in MongoEngine... It seems that it should be enough to not pass it to PyMongo at all unless it was specified in the config. Am I missing something?

wojcikstefan · 2016-12-01T01:57:48Z

mongoengine/connection.py

        # Authenticate if necessary
-        if conn_settings['username'] and conn_settings['password']:
+        if (conn_settings['authentication_mechanism'] == 'MONGODB-X509' and conn_settings['username']) or\
+                (conn_settings['username'] and conn_settings['password']):


I think we could break it down into something more elegant, e.g.:

needs_auth = conn_settings['username'] and (conn_settings['password'] or conn_settings['authentication_mechanism'] == 'MONGODB-X509') if needs_auth: ...

wojcikstefan · 2016-12-01T02:02:05Z

mongoengine/connection.py

                            conn_settings['password'],
-                            source=conn_settings['authentication_source'])
+                            source=conn_settings['authentication_source'],
+                            mechanism=conn_settings['authentication_mechanism'])


Given my comment above about PyMongo's default behavior, I think that we could default authentication_mechanism to None and then we could modify this code to do:

# include the authentication mechanism in kwargs if specified auth_kwargs = {'source': conn_settings['authentication_source']} if conn_settings['authenticaltion_mechanism'] is not None: auth_kwargs['mechanism'] = conn_settings['authenticaltion_mechanism'] db.authenticate(conn_settings['username'], conn_settings['password'], **kwargs)

Yes, this is a great solution. Thank you!

https://github.com/mongodb/mongo-python-driver/blob/v2.7/pymongo/database.py#L832

Apparently, this is a clear indication of the mechanism in the authenticate method confused me on my first failed commits. And I try to set it explicitly:

https://travis-ci.org/MongoEngine/mongoengine/jobs/142701892#L1483

Please look fixes in my last commit.

wojcikstefan · 2016-12-02T16:41:46Z

Thanks @zeezdev, this looks good at a glance! Can you confirm that this branch works in a real life scenario with X.509 auth? I'll merge this when you confirm.

Thanks for your contribution!

zeezdev · 2016-12-02T18:31:41Z

I use the MongoEngine in one real commercial project. How can I show it? But I wrote about this problem in my article about "building mongodb ShardedCluster with X.509 authentication" (see last paragraph). I am sorry it is not in English:

https://habrahabr.ru/post/308740/

...sample code from the article:

import ssl
from mongoengine import DEFAULT_CONNECTION_NAME, register_connection


db_hosts="server1.cluster.com:27017,server2.cluster.com:27017"
db_port=None

ssl_config = {
    'ssl': True,
    'ssl_certfile': "/path_to_certs/analyticsuser.PEM",
    'ssl_cert_reqs': ssl.CERT_REQUIRED,
    'ssl_ca_certs': "/path_to_certs/mongodb-CA-cert.crt",
  }

register_connection(alias=DEFAULT_CONNECTION_NAME,
                  name="statistic",
                  host=db_hosts,
                  port=db_port,
                  username="CN=username,OU=StatisticsClient,O=SomeSystems,L=Moscow,ST=MoscowRegion,C=RU",
                  password=None,
                  read_preference=ReadPreference.NEAREST,
                  authentication_source="$external",
                  authentication_mechanism="MONGODB-X509",
                  **ssl_config)

I will be happy if this will merged and appeared in official version.

wojcikstefan · 2016-12-03T02:41:19Z

Thanks! What I meant when I asked you for confirmation is simply to say "yes, I ran this branch of mongoengine against a running mongod process and I was successfully able to authenticate via X.509". It seems that you have, is that correct?

zeezdev · 2016-12-03T09:44:16Z

Yes. (but small remark: connection to mongodb cluster performed by means mongos process not mongod). Thanks!

wojcikstefan · 2016-12-03T21:08:31Z

Thanks @zeezdev !

Stanislav Tomilov and others added 8 commits July 6, 2016 13:57

connection: Add authentication mechanism support. MongoEngine#905

c55d986

connection: remove check password before db.authenticate. (MONGODB-X5…

83bf50d

…09 without password)

connection: add default authentication_mechanism

cc09081

check IS_PYMONGO_27 for AUTHENTICATION_MECHANISM

ce55d27

fix import IS_PYMONGO_27

ef37c66

fix import IS_PYMONGO_27

7e1a017

Merge branch 'master' of github.com:MongoEngine/mongoengine

21f0aa6

Conflicts: mongoengine/connection.py

Fix wrong order of imported names

2e06e1b

wojcikstefan requested changes Nov 30, 2016

View reviewed changes

Stanislav Tomilov added 2 commits November 30, 2016 16:30

Fix IS_PYMONGO_27 definition and check both 'username' and 'password'…

130cbec

… conn_settings for connections with not 'MONGODB-X509' mechanism

Merge branch 'master' of github.com:MongoEngine/mongoengine

1b0d478

zeezdev commented Nov 30, 2016

View reviewed changes

wojcikstefan requested changes Dec 1, 2016

View reviewed changes

Stanislav Tomilov and others added 2 commits December 1, 2016 21:01

No set value for authentication_mechanism by default

6f2e9a0

pass source and mechanism as kwargs in authenticate() method

a3fff95

wojcikstefan approved these changes Dec 2, 2016

View reviewed changes

wojcikstefan merged commit 02fb3b9 into MongoEngine:master Dec 3, 2016

wojcikstefan mentioned this pull request Dec 3, 2016

Added authentication mechanism support. Transport mechanism to pymongo d... #905

Closed

Uh oh!

connection: Add authentication mechanism support. #905 #1333

connection: Add authentication mechanism support. #905 #1333

Uh oh!

Conversation

zeezdev commented Jul 6, 2016 • edited by thedrow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wojcikstefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeezdev Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeezdev Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojcikstefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojcikstefan commented Dec 2, 2016

Uh oh!

zeezdev commented Dec 2, 2016

Uh oh!

wojcikstefan commented Dec 3, 2016

Uh oh!

zeezdev commented Dec 3, 2016

Uh oh!

wojcikstefan commented Dec 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zeezdev commented Jul 6, 2016 •

edited by thedrow

Loading

zeezdev Nov 30, 2016 •

edited

Loading

zeezdev Nov 30, 2016 •

edited

Loading