-
Notifications
You must be signed in to change notification settings - Fork 527
SNOW-762783: Introduce ruff formatting + improve logging formatting #2710
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
base: main
Are you sure you want to change the base?
Conversation
| k: v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******" | ||
| for (k, v) in body["data"].items() | ||
| }, | ||
| {k: v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******" for (k, v) in body["data"].items()}, |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
To prevent leaking sensitive information in logs, we should always mask known sensitive keys such as 'PASSCODE', 'PASSWORD', and similar. For authentication data, it is safest to explicitly blacklist known secret keys, overriding any whitelist mechanism.
The best solution for the problematic lines (149) is to ensure that sensitive keys are always masked, regardless of whitelist configuration. This means instead of using AUTHENTICATION_REQUEST_KEY_WHITELIST to decide what may be shown, we should create a blacklist set (e.g. SENSITIVE_AUTH_KEYS) for keys that must never be logged in clear text.
In the scope allowed, we can define a small set of keys at the top of the file (or with the function), for example:
SENSITIVE_AUTH_KEYS = {"PASSCODE", "PASSWORD", ...}Then, in the debug logging, we mask any key in SENSITIVE_AUTH_KEYS regardless of other conditions:
{k: ("******" if k in SENSITIVE_AUTH_KEYS else v) for k, v in body["data"].items()}This change affects only the debug logging statement in src/snowflake/connector/aio/auth/_auth.py, lines 147-150.
No dependency additions are required; all required functionality is available in standard Python.
-
Copy modified lines R49-R50 -
Copy modified line R52 -
Copy modified line R151
| @@ -46,7 +46,10 @@ | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # List of sensitive authentication keys that must never be logged in clear text. | ||
| SENSITIVE_AUTH_KEYS = {"PASSCODE", "PASSWORD"} | ||
|
|
||
|
|
||
| class Auth(AuthSync): | ||
| async def authenticate( | ||
| self, | ||
| @@ -146,7 +148,7 @@ | ||
|
|
||
| logger.debug( | ||
| "body['data']: %s", | ||
| {k: v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******" for (k, v) in body["data"].items()}, | ||
| {k: ("******" if k in SENSITIVE_AUTH_KEYS else v) for (k, v) in body["data"].items()}, | ||
| ) | ||
|
|
||
| try: |
| k: v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******" | ||
| for (k, v) in body["data"].items() | ||
| }, | ||
| {k: v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******" for (k, v) in body["data"].items()}, |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
The fix is to ensure that no sensitive data (such as passwords, passcodes, or tokens) is ever logged, regardless of the contents of AUTHENTICATION_REQUEST_KEY_WHITELIST. The logging block at line 245 should always redact known sensitive fields (e.g., "PASSCODE", "password", "access_token", etc.), using a hardcoded denylist of these keys. For all other keys, the current whitelist approach can be maintained if desired, but the denylist should override the whitelist.
Specifically, in src/snowflake/connector/auth/_auth.py, replace the log statement so that values for any sensitive key (from a hardcoded denylist) are always replaced with "******", regardless of their status in the whitelist. For flexibility and future-proofing, define the denylist (e.g., as AUTH_LOG_REDACT_KEYS) near the logging call or at module-level (as appropriate). Only true non-sensitive parameters may be logged for debugging.
No changes are required in the other files (src/snowflake/connector/auth/_oauth_base.py, src/snowflake/connector/connection.py) since the analysis only detects the data flow to the problematic log at line 245 in _auth.py (unless there are similar logging calls there; none shown in the provided snippets).
-
Copy modified lines R243-R244 -
Copy modified line R247
| @@ -240,9 +240,11 @@ | ||
| if session_parameters: | ||
| body["data"]["SESSION_PARAMETERS"] = session_parameters | ||
|
|
||
| # Always redact sensitive fields, regardless of whitelist | ||
| AUTH_LOG_REDACT_KEYS = {"PASSCODE", "PASSWORD", "ACCESS_TOKEN", "REFRESH_TOKEN", "TOKEN", "ID_TOKEN", "SECRET", "CLIENT_SECRET"} | ||
| logger.debug( | ||
| "body['data']: %s", | ||
| {k: v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******" for (k, v) in body["data"].items()}, | ||
| {k: "******" if k.upper() in AUTH_LOG_REDACT_KEYS else (v if k in AUTHENTICATION_REQUEST_KEY_WHITELIST else "******") for (k, v) in body["data"].items()}, | ||
| ) | ||
|
|
||
| try: |
| enc_kek = ( | ||
| encryptor.update(PKCS5_PAD(file_key, block_size)) + encryptor.finalize() | ||
| ) | ||
| enc_kek = encryptor.update(PKCS5_PAD(file_key, block_size)) + encryptor.finalize() |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic algorithm High
The block mode ECB
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
How to fix the problem:
Replace the use of AES in ECB mode to encrypt the file key with a secure key wrapping algorithm, such as AES Key Wrap (RFC 3394). Python's cryptography library provides standard-conforming key wrap/unwrap algorithms in cryptography.hazmat.primitives.keywrap.
Detailed fix:
- Import
aes_key_wrapfromcryptography.hazmat.primitives.keywrap. - At the relevant line, replace the construction of a raw AES-ECB cipher and the manual encryption + padding with a call to
aes_key_wrap, passing the KEK (decoded_key) and the key to be wrapped (file_key). - The output,
enc_kek, will be the wrapped key (there is no need for manual padding, cipher setup, or cipher finalization). - Remove any code related to the ECB cipher used for this purpose.
Which files/lines to change:
- In
src/snowflake/connector/encryption_util.py, add the import. - In the body of
encrypt_stream, replace both theCipher(algorithms.AES(...), modes.ECB(), ...)block and associated padding/manipulation with a call toaes_key_wrap. - Ensure the remainder of the function uses the output as before (returning base64-encoded wrapped keys).
-
Copy modified line R14 -
Copy modified lines R95-R96
| @@ -11,6 +11,7 @@ | ||
|
|
||
| from cryptography.hazmat.backends import default_backend | ||
| from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes | ||
| from cryptography.hazmat.primitives.keywrap import aes_key_wrap | ||
|
|
||
| from .compat import PKCS5_OFFSET, PKCS5_PAD, PKCS5_UNPAD | ||
| from .constants import UTF8, EncryptionMetadata, MaterialDescriptor, kilobyte | ||
| @@ -91,10 +92,8 @@ | ||
| out.write(encryptor.update(block_size * chr(block_size).encode(UTF8))) | ||
| out.write(encryptor.finalize()) | ||
|
|
||
| # encrypt key with QRMK | ||
| cipher = Cipher(algorithms.AES(decoded_key), modes.ECB(), backend=backend) | ||
| encryptor = cipher.encryptor() | ||
| enc_kek = encryptor.update(PKCS5_PAD(file_key, block_size)) + encryptor.finalize() | ||
| # encrypt key with QRMK using AES Key Wrap (RFC 3394) | ||
| enc_kek = aes_key_wrap(decoded_key, file_key, backend=backend) | ||
|
|
||
| mat_desc = MaterialDescriptor( | ||
| smk_id=encryption_material.smk_id, |
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:
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: