Fix connection test API to restore masked password/extra from existing connections

[choo121600]

This PR fixes an issue where the connection test API (/connections/test) fails when testing existing connections that have masked sensitive fields (password, extra).

Previously, when a user tried to test an existing connection, the API would receive masked values like "***" for password and extra fields, causing the connection test to fail because the actual credentials were not available.

Before

After

closes: #59298

---

^ 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.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[choo121600]

It seems to be a CI failure unrelated to my changes🤔

[jason810496]

Thanks for the fix!

Not sure will option 2 be a more compatible approach, as current patch might result in breaking change behavior for the scenario I described.

Since we don't need to ensure the Frontend compatibility ( UI is out of scope of public interface ), but we have to ensure the Public API compatibility.

[choo121600]

considered a couple of options:

• Option 1: Determine whether to merge based on the existence of connection_id
• Option 2: The approach implemented in this PR

When testing the API, there is a scenario where users want to test the connection using a newly provided password, rather than the credential stored in the database.
Considering this use case, I concluded that Option 2 handles this case more accurately than Option 1, and therefore chose Option 2.

To support this, I added a use_existing_credentials query parameter, with the default value set to false.

• When calling the API directly: the test is performed using the credentials provided in the request
• When testing from the UI: this scenario does not occur, so the parameter is fixed to true

If, in the future, the UI adds functionality similar to the API test, where connections can be tested using user-provided input this parameter can be switched tofalse to support that use case in a natural and extensible way.

[choo121600]

It seem a bit complex, but I think this is the better option.
Please let me know if you see a better alternative :)

[choo121600]

It looks like this happens when it’s generated with prek. I’ll fix it manually.

[jason810496]

To support this, I added a use_existing_credentials query parameter, with the default value set to false.

Yes, that make sense to me.
However, we still required test for the corresponding change for public API, thanks!

[choo121600]

However, we still required test for the corresponding change for public API, thanks!

Thanks for review!
I’ll try to fix the open-api-gen issue and add tests for use_existing_credentials ;)

[potiuk]

Actually - the best way to fix it is to do it properly - i.e. run "test_connection" from workers - not from the "api-server" component. This is not the question of only the connection password masking but also execution environment.

A bit more context.

Running "test_connection" in the api_server context has several problems:

1. the api-server does not need to connect to outside world. In many cases where security is important, the api_server will be running in the container / network /security perimeter which cannot even make such outside connections to external systems becasue it will be behind restrictive firewall. Api-server is usually somewhat exposed to outside - ie. it's theorethicallly easier for the attacker to break into api-server, so a number of of deployments out-there have api-server in quite well separated environment.

This is a functional problem with "test connection" run from api-server

2. No User-provided code (not even DAG-authored code) should be runnnable on api-server. Running test-connection is inherently unsafe - because there are a number of connections and drivers that are impossible to secure. This is the case with ODBC for example or JDBC - where you can often pass a configuration to such driver that might cause RCE in the environment where the driver is initialized - we had MANY such security issues in the past and this was the main reason why we disabled the test-connection by default https://airflow.apache.org/docs/apache-airflow/stable/release_notes.html#disable-default-allowing-the-testing-of-connections-in-ui-api-and-cli-32052 in Airflow 2.7.0 and warned our users that they should not enable it if they care about security. This was advised to our users in this CVE https://nvd.nist.gov/vuln/detail/cve-2023-37379 - and in the configuration we told them in the blog as well:

https://airflow.apache.org/blog/airflow-2.7.0/

Airflow 2.7.0 is a release that focuses on security. The Airflow security team, working together with security researchers, identified a number of areas that required strengthening of security. This resulted in, among others things, an improved description of the Airflow security model, a better explanation of our security policy and the disabling of certain, potentially dangerous, features by default - like, for example, connection testing (#32052).

This is a security problem with running "Test-connection" from the api-server.

The conditions of triggering the vulnerability and dangers have not changed a bit since 2.7.0 - even with task isolation - those are exactly the same dangers that were in 2.7.0 when test-connection was disabled by default. Users are exactly in the same - dangerous - place where they enable it back today.

What can we do about with it ?

Build on generic workflow feature introduced in Deadline alerts and instead of running "Test-connection" in the api-server, schedule workflow to be executed to actually do this in "worker" or "triggerer". This is what I was sceptical about when test-connection was introduced: #15795 (comment) and further discussions. What HAS changed since then is the argument of runing "api-server" and "workers" together does not stand any more (soon). We do everything possible now to enable the case where workers have not only different dependencies but simply - different airflow distribiutions ("airflow-task-sdk" rather than "airflow-core") - so the main argument in that discusison that running Airflow in such isolated environments is an edge case - but in Airflow 3.2 hopefully - this will become essentially "reocmmended" way of running airflow. Also we have something more thna we had 4 year ago - we finally are clearing the path of being able to run more generic workloads and run them through wokers/triggerer and provide feedback from those workloads (with deadline feature) - and we could follow the suite and implement it in similar way.

This does not solve all the security issues but it makes our isolation way more consistent. With test connection on api_server you basically make UI users capable of running arbitrary code in api_server, and arbitrary modification of the database. If you move it's execution to workers- those components of airflow are way more isolated and are supposed to run dangerous code already, and usually is done in ephemeral (say k8s pods in k8s executor) - rather than long running environment of api_server. Which has IMHO acceptable security properties to even enable it by default with some security model description changes.

My recommendation would be - not to "fix" this feature - but declare it "broken" and fix it properly by moving test-connection workflows to "workers".

[choo121600]

Wow! Thanks for the detailed background.
While working on the patch, I also felt this part was quite complex, and your explanation helped clarify why.

I agree that instead of incrementally fixing test_connection, it makes more sense to remove it and re-implement it on the worker side.
The more I touched it, the more complex and delicate it felt🥲

That said, I don’t yet have a enough understanding of backend architecture, and this doesn’t look like a small change, so it’s hard for me to implement it right away.
After Jan 9, I’ll take a closer look at the relevant architecture and start by outlining a possible approach😉

[pierrejeambrun]

I agree with Jarek on the long term fix, but that's a much bigger effort.

In the meantime I think we should still fix the feature for sensitive value. We can just use update_orm_from_pydantic and test the connection on the mutated orm_conn, and then discard the updated orm_conn object to not mutate the db.

No need for an extra param, update_orm_from_pydantic takes care of everything. (if sensitive values were updated take those new sensitive values, if they were not restore them from db)

[jason810496]

Nice! Thanks for the update.

One final nit: Could we assert the existed connection is not modified after calling test connection API? Since we called the update_orm_from_pydantic after change. Thanks!

[pierrejeambrun]

Code looks good.

Test need adjustments I believe.

[pierrejeambrun]

LGMT, I rebased it to solve a merge conflict (and adjusted a really small nit).

Should be good to go, thanks 🎉

[github-actions]

Backport failed to create: v3-1-test. View the failure log Run details

Status	Branch	Result
❌	v3-1-test

You can attempt to backport this manually by running:

cherry_picker efb27dc v3-1-test

This should apply the commit to the v3-1-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

If you don't have cherry-picker installed, see the installation guide.

[pierrejeambrun]

Manual backport #60873