Pmansour/snow 1927956 wif by sfc-gh-pfus · Pull Request #2195 · snowflakedb/snowflake-connector-python

sfc-gh-pfus · 2025-03-05T06:35:58Z

Please answer these questions before submitting your pull requests. Thanks!

What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Fixes #NNNN
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am adding a new telemetry message
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency
Please describe how your code solves the related issue.

Please write a short description of how your code change solves the related issue.
(Optional) PR for stored-proc connector:

…nection.py

github-actions · 2025-03-05T06:36:15Z

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

_{You can retrigger this bot by commenting recheck in this Pull Request.}_{Posted by the CLA Assistant Lite bot.}

sfc-gh-pfus · 2025-03-05T06:37:10Z

setup.cfg

 packages = find_namespace:
 install_requires =
    asn1crypto>0.24.0,<2.0.0
+    boto3>=1.0


Do we have risk assesment for these new libraries?

we do not -- what's the process for getting that?

FWIW they're the standard AWS client libraries and are maintained by AWS, so I suspect it'll be fine, but if you can point me at a process I'd be happy to follow that.

src/snowflake/connector/auth/__init__.py

sfc-gh-pfus · 2025-03-05T06:40:55Z

src/snowflake/connector/auth/workload_identity.py

+        return ValueError(f"Unknown attestation provider '{attestation.provider}'")
+
+
+class AuthByWorkloadIdentity(AuthByPlugin):


Is this class meant to be used in multithreaded environment? It is not thread safe.

could you please elaborate on this part? I believe it's using the same pattern as all the other authenticator plugins.

I am not sure if authenticator plugins generally need to be thread-safe, or if the other ones are (or aren't), or if this one is different.

src/snowflake/connector/connection.py

sfc-gh-pfus · 2025-03-05T06:43:33Z

src/snowflake/connector/connection.py


        # Set of authenticators allowing empty user.
-        empty_user_allowed_authenticators = {OAUTH_AUTHENTICATOR, NO_AUTH_AUTHENTICATOR}
+        empty_user_allowed_authenticators = {OAUTH_AUTHENTICATOR, NO_AUTH_AUTHENTICATOR, WORKLOAD_IDENTITY_AUTHENTICATOR}


Hmmm, I believe external browser and PAT also don't need user... @sfc-gh-mmishchenko wdyt?

Worth checking, but that's not probably a subject of this review.

src/snowflake/connector/wif_util.py

sfc-gh-pfus · 2025-03-05T06:49:44Z

src/snowflake/connector/wif_util.py

+        )
+
+
+def create_autodetect_attestation(account: str, token: str | None = None) -> Union[WorkloadIdentityAttestation, None]:


Having in mind 3s timeouts, that function is not a good idea IMHO :( What is the use case here?

The use-case is somebody writing the same application that uses the driver, and allowing it to run in multiple environments (e.g. on GCP, AWS, Azure, Kubernetes). It's a big part of the usability push here.

Of course this mode is optional, and customers who want to avoid potential startup delays should specify an explicit CSP provider. But this is a baseline for customers who just want to write once and run everywhere without configuration, and are OK with the potential latency hit.

sfc-gh-pfus · 2025-03-05T06:57:54Z

test/unit/test_auth_workload_identity.py

+def fake_sign_aws_req(request: AWSRequest):
+    """Produces a fake Sigv4 signature on an AWS request."""
+    request.headers.add_header("X-Amz-Date", datetime.time().isoformat())
+    request.headers.add_header("X-Amz-Security-Token", "<TOKEN>")


Is this <TOKEN> string somehow special?

It's just a random string for the test. The real (non-stubbed) implementation will put a real token here. I'm just trying to make my stub as close to reality as possible.

sfc-gh-pfus · 2025-03-05T06:59:15Z

test/unit/test_auth_workload_identity.py

+    )
+
+
+def verify_aws_token(token: str):


Is it a token or the whole HTTP response?

We're calling this a "token" for consistency purposes with Azure and GCP, but in reality the attestation we produce for AWS isn't exactly a token (though for authentication purposes it fulfills the same purpose). In reality it's a description of a signed HTTP request that our backend can execute and get the caller's identity. This is described in go/wif

sfc-gh-pfus · 2025-03-05T07:00:07Z

test/unit/test_auth_workload_identity.py

+    assert decoded_token["method"] == "POST"
+
+    headers = decoded_token["headers"]
+    assert set(headers.keys()) == set(["Host", "X-Snowflake-Audience", "X-Amz-Date", "X-Amz-Security-Token", "Authorization"])


Why do we need to verify all headers? Maybe instead we can check if the relevant header exists? But what is the point of this check at all?

That's exactly what this is doing -- it's checking that the relevant headers exist (aka the set of keys is the expected set), but not checking their values.

The only header whose value is being verified in the test is the "X-Snowflake-Audience" header, because we're setting it ourselves in the driver code.

…ntials, updated / standardized unique assertion_content

sfc-gh-pmansour · 2025-03-06T17:32:07Z

Will create a new PR shortly with a full description, all completed TODOs, a short accompanying doc and a broader list of reviewers.

sfc-gh-pmansour added 20 commits March 3, 2025 17:57

Add AWS dependencies

8f2adc0

add basic attestation loading

605c636

improve the contract for fetching WIF attestations

a0eb05c

Add explicit attestation provider type, support for OIDC tokens

d6d8bdd

add associated error codes

2d6efc1

fix small bugs, clean-up data structures, add auto-detect provider

cd49eaf

fix docstrings

06d74bd

add an entry point

36b7e20

add WLID authenticator plugin

0c17d31

Add glue for WLID authenticator

e245c9c

Remove explicit handling of token_file_path since this is done in con…

4c6159f

…nection.py

Add unit tests for connection plumbing

a3f9f60

fix bug with default value of workload_identity_provider

2ee89bb

small bug fix in HTTP request

2601335

update logic for assertion_content

ca1f0a6

fix whitespace

22d3cd7

update error message

8b2d45f

wrap metadata server requests in a try..except block

e850268

include issuer in Azure identifier string

aac04ed

add unit tests

bae5987

sfc-gh-pfus commented Mar 5, 2025

View reviewed changes

sfc-gh-mhofman requested a review from sfc-gh-mmishchenko March 5, 2025 09:07

sfc-gh-pmansour added 7 commits March 5, 2025 11:59

change order of constants

e6c6a22

Slight refactor of metadata calls, added debug logs for missing crede…

5ed7ad7

…ntials, updated / standardized unique assertion_content

refactor token parsing

fc1577d

gate the feature on the preview env variable

5ccc5c6

update autodetect logic for OIDC

85b57b4

allow explicit setting of entra resource, don't ask for account

d8739d1

add tests for Entra resource plumbing

fd38e2f

sfc-gh-pmansour added 2 commits March 5, 2025 16:31

add support for AWS region and arn loading

1a0e3af

add support for azure functions

c4570e3

sfc-gh-pmansour closed this Mar 6, 2025

github-actions bot locked and limited conversation to collaborators Mar 6, 2025

		return ValueError(f"Unknown attestation provider '{attestation.provider}'")


		class AuthByWorkloadIdentity(AuthByPlugin):

		)


		def create_autodetect_attestation(account: str, token: str \| None = None) -> Union[WorkloadIdentityAttestation, None]:

Conversation

sfc-gh-pfus commented Mar 5, 2025

Uh oh!

github-actions bot commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-mmishchenko Mar 5, 2025 • 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!

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!

sfc-gh-pmansour Mar 5, 2025 • 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!

sfc-gh-pmansour Mar 5, 2025 • 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!

sfc-gh-pmansour Mar 5, 2025 • 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!

sfc-gh-pmansour commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sfc-gh-mmishchenko Mar 5, 2025 •

edited

Loading

sfc-gh-pmansour Mar 5, 2025 •

edited

Loading

sfc-gh-pmansour Mar 5, 2025 •

edited

Loading

sfc-gh-pmansour Mar 5, 2025 •

edited

Loading