Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

authsource configuration for mongo credentials #823

Closed
johanseto opened this issue Apr 3, 2023 · 9 comments
Closed

authsource configuration for mongo credentials #823

johanseto opened this issue Apr 3, 2023 · 9 comments
Labels
bug Bugs will be investigated and fixed as quickly as possible.

Comments

@johanseto
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Hi, I was configuring my mongo credentials for my openedx instance but I found an error related to the authSource key.

In the first approach, I configurated with the MONGODB_AUTH_SOURCE in my config.yml but I got an authentication error in the LMS.
image

This is because while researching I find that the key is in camelcase.
https://github.com/overhangio/tutor/blob/master/tutor/templates/apps/openedx/settings/partials/common_all.py#L16
Something I think is ok because pymongo use it in camelcase.
2023-04-03_10-52

However, the error was presented because edx-platform use another variable when credentials are provided.
https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L72

Looking more , the auth_source is configured with another key name: authsource from the kwargs.
https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L34
So for my second approach, and provisional solution I added authsource kwarg in a patch:

 openedx-common-settings: |

  DOC_STORE_CONFIG['authsource'] = "{{ MONGODB_AUTHSOURCE }}"
  CONTENTSTORE["DOC_STORE_CONFIG"] = DOC_STORE_CONFIG
  update_module_store_settings(MODULESTORE,doc_store_settings=DOC_STORE_CONFIG)

And then the connection was successful and my mongo atlas instance started working with my tutor openedx lms and cms.

Describe the solution you'd like

Describe alternatives you've considered
I think that this is very confusing due mix of credentials keys because pymongo recommends that kwarg would be in camelcase(authSource) but edx-platform used as authsource. Also in the python logic, you can find it in snake_case auth_source https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L39.

Additional context

Something interesting that I test, is that the mongo connection also work if I remove the kwarg in camelcase authSource but keeping the authsourcekwarg. But this is something strange because as I said upper pymongo should work with the camelcase.
https://pymongo.readthedocs.io/en/3.12.1/api/pymongo/mongo_client.html

@regisb
Copy link
Contributor

regisb commented Apr 13, 2023

Thanks for this detailed writeup @johanv26! I do agree that this is a very confusing situation 😅

Something interesting that I test, is that the mongo connection also work if I remove the kwarg in camelcase authSource but keeping the authsource kwarg.

It seems to me that this is the most appropriate fix.

But this is something strange because as I said upper pymongo should work with the camelcase.
https://pymongo.readthedocs.io/en/3.12.1/api/pymongo/mongo_client.html

Right. But according to the pymongo source code, the MongoClient kwargs are sent through a _CaseInsensitiveDictionary filter: https://github.com/mongodb/mongo-python-driver/blob/3.12.3/pymongo/mongo_client.py#L651 As a consequence, it's probably OK to pass authsource instead of authSource as a keyword argument.

Ping @zakum1 and @keithgg who are actually using MONGODB_AUTH_SOURCE and who commented on the initial implementation. Did setting up MONGODB_AUTH_SOURCE every break for you in edx-platform?

@regisb regisb added the bug Bugs will be investigated and fixed as quickly as possible. label Apr 13, 2023
@johanseto
Copy link
Contributor Author

johanseto commented Apr 13, 2023

@regisb thanks for your attention. You are right is OK for Pymongo due to the _CaseInsensitiveDictionary. So the authsource for my case skips the break in edx-platform.

Mainly when is extracted here:
https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L34
and then used here: https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L72
Also, this is only when credentials were provided.
https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L70-L71_

Let's see if authSource broke other implementations, and test if authsource guarantee compatibility for others.

@keithgg
Copy link
Contributor

keithgg commented May 24, 2023

@regisb sorry about the late reply here. I was only holiday.

It's never broken for me (or should I say, not broken in unexpected ways). In fact after the changes were added I could remove our overrides. 99% of the time the authSource=admin, so we actually default to it when not provided by the MongoDB host.

@regisb
Copy link
Contributor

regisb commented May 24, 2023

If I understand correctly, the current authSource argument is purely and simply ignored, so there is no way that the MONGODB_AUTHSOURCE setting works for anyone...

@keithgg your commit confirms that you were indeed using authsource, and not authSource.

So the fix should be a single character change. @johanv26 would you like to open a PR? If not I can take care of it.

@keithgg
Copy link
Contributor

keithgg commented May 24, 2023

@regisb I'm okay with making that change. The forum is where I had issues with the authSource parameter, so as long as that stays as is, I'm happy.

But please ping me on the PR and I'll do some extra testing before merging.

@johanseto
Copy link
Contributor Author

If I understand correctly, the current authSource argument is purely and simply ignored, so there is no way that the MONGODB_AUTHSOURCE setting works for anyone...

@keithgg your commit confirms that you were indeed using authsource, and not authSource.

So the fix should be a single character change. @johanv26 would you like to open a PR? If not I can take care of it.

Yes I would open the PR ASAP

@keithgg
Copy link
Contributor

keithgg commented May 26, 2023

I've approved the PR, but there's an issue with the way mongodb_utils.py authenticates. Adding this comment for other folks like me that will run into this issue.

The function definition is:

def connect_to_mongodb(
    db, host,
    port=27017, tz_aware=True, user=None, password=None,
    retry_wait_time=0.1, proxy=True, **kwargs
):

The block that actually connects is:

    mongo_conn = pymongo.database.Database(
        pymongo.MongoClient(
            host=host,
            port=port,
            tz_aware=tz_aware,
            document_class=dict,
            **kwargs
        ),
        db
    )

For Digital Ocean Clusters (didn't check Atlas), you get an error when running the migrations from scratch

  File "/openedx/venv/lib/python3.8/site-packages/pymongo/auth.py", line 113, in _build_credentials_tuple
    raise ConfigurationError("%s requires a username." % (mech,))
pymongo.errors.ConfigurationError: SCRAM-SHA-1 requires a username.

With a DOC_STORE_CONFIG:

{
    "db": "mongo-test",
    "host": "mongodb+srv://kgculstermongo.ondigitalocean.com",
    "port": 27017,
    "user": "mongo-test",
    "password": "password",
    # Connection/Authentication
    "ssl": True,
    "authsource": "admin",
    "replicaSet": "kgcluster-mongodb",
    "authMechanism": "SCRAM-SHA-1",
}

It fails because the username and password is only added later in the mongo_conn.authenticate invocation. This isn't an issue for us, because we include the username and password in the URL so it looks like mongodb+srv://username:password@host. This allow that initial connection the authenticate successfully.

@regisb
Copy link
Contributor

regisb commented May 26, 2023

Thanks for the detailed write-up @keithgg, this will be helpful if anyone stumbles upon the same issue. At this point I must confess I'm a bit tired of this little song-and-dance that we need to do because of an outdated dependency and obnoxious custom API layer... Well, I guess I could try to rip it out of edx-platform...

@regisb
Copy link
Contributor

regisb commented May 29, 2023

Closed by #841

@regisb regisb closed this as completed May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs will be investigated and fixed as quickly as possible.
Projects
Development

No branches or pull requests

3 participants