Adding certificate validation for ssl in mongo hook #37214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amoghrajesh
commented
Feb 7, 2024
amoghrajesh
commented
Feb 7, 2024
eladkal
reviewed
Feb 7, 2024
|
Right now I am testing with the DAG: Connection: For some reason, the connection is not going through, to mongodb Error log: Point to note is that when I use pymongo directly, it works |
|
Also tried adding mongo://127.0.0.1, this doesnt work either for uri |
I've wondering, did you run Airflow into the docker? |
Taragolis
reviewed
Feb 7, 2024
I am running it using breeze, so yes, you might be right. |
|
Thanks @Taragolis ! |
|
Awesome. I was able to test it. Results: My connections defined: Logs for my_mong (ssl is not present): Logs for my_mong_secure (explicitly passed my ssl: true) |
Yeah in breeze only available simple integration, which need to be run with ❯ breeze start-airflow --backend postgres --postgres-version 15 --integration mongoAfter that you could communicate with Mongo 3 thought the host from airflow import DAG
from airflow.decorators import task
from airflow.providers.mongo.hooks.mongo import MongoHook
with DAG("pr_37214", schedule=None, tags=["37214", "mongodb"]):
@task
def test_mongo():
# Create a connection into the runtime, might be replaced by connection created into the UI
import os
from airflow.models.connection import Connection
mongo_conn_id = "mongo_test"
conn = Connection(conn_id=mongo_conn_id, conn_type="mongo", host="mongo", port=27017, schema="test")
os.environ[f"AIRFLOW_CONN_{conn.conn_id.upper()}"] = conn.as_json()
# Actual execution
mongo_hook = MongoHook(mongo_conn_id=mongo_conn_id)
mongo_hook.insert_one("users", {"name": "John", "surname": "Wick"}, mongo_db="test")
collection = mongo_hook.get_collection("users", mongo_db="test")
print(collection.count_documents({}))
test_mongo()And it is expected that if you try to communicate in breeze with |
Perfect! I wasn't aware of this, thank you! |
potiuk
reviewed
Feb 7, 2024
|
@potiuk to maintain the older behaviour, I think one thing we can do is: and in get_conn: Thoughts? |
|
I think having "alllow_insecure" while setting "ssl=True" is precisely the vulnerability here, because it is unexpected. So i think when user sets "ssl=True" then allow_insecure should be false by default. And it's ok to make breaking change while fixing security bug. We want people to find out they have security issue and make them fix it. |
|
Sure, thanks for your review. I will take a look at this soon, mostly tomorrow ist morning |
|
@potiuk this has been tested out with the following combinations: It works as expected. |
potiuk
approved these changes
Feb 8, 2024
potiuk
reviewed
Feb 8, 2024
potiuk
reviewed
Feb 8, 2024
potiuk
approved these changes
Feb 8, 2024
Just ran it locally and pushed a fix, we should be good! |
Taragolis
reviewed
Feb 8, 2024
Comment on lines
+30
to
+31
| 3.7.0 | ||
| ...... |
There was a problem hiding this comment.
Post merge comment:
I guess the version should be 4.0.0 in case of breaking changes
There was a problem hiding this comment.
I will raise a follow up for this. Sorry missed it
81 tasks
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we are skipping tls certificate validation in mongo hook even if ssl is set to true. This needs to be handled better
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.