add MFA authentication support to snowflake integration #2305
Conversation
github-actions
bot
added
integration:snowflake
type:documentation
There was a problem hiding this comment.
Thank you so much for your effort and contribution, @ChinmayBansal! I left a few comments, but overall this looks great. Would you mind moving the authentication code to a separate file outside of the main component to help keep things tidy? Also, were you able to test JWT and OAuth with real credentials?
| self.return_markdown = return_markdown | ||
|
|
||
| # Authentication parameters | ||
| self.authenticator = authenticator or "SNOWFLAKE" |
There was a problem hiding this comment.
If a user doesn't specify authenticator but provides JWT credentials, it will default to "SNOWFLAKE" and then validation will require api_key. I suggest making authenticator required rather than optional.
| return_markdown: bool = True, | ||
| authenticator: Optional[Literal["SNOWFLAKE", "SNOWFLAKE_JWT", "OAUTH"]] = None, | ||
| private_key_file: Optional[Union[str, Secret]] = None, | ||
| private_key_file_pwd: Optional[Union[str, Secret]] = None, |
There was a problem hiding this comment.
Sensitive credentials such as passwords and Secrets should always use Secrets and not combination of string and Secrets
| private_key_file: Optional[Union[str, Secret]] = None, | ||
| private_key_file_pwd: Optional[Union[str, Secret]] = None, | ||
| oauth_client_id: Optional[Union[str, Secret]] = None, | ||
| oauth_client_secret: Optional[Union[str, Secret]] = None, |
| if value is None: | ||
| return None | ||
| if isinstance(value, Secret): | ||
| return value.resolve_value() |
There was a problem hiding this comment.
Good idea to add an exception handling here
| deserialize_secrets_inplace(init_params, secret_fields) | ||
| return default_from_dict(cls, data) | ||
|
|
||
| def _snowflake_uri_constructor(self) -> str: |
There was a problem hiding this comment.
What happen if api_key, user, account etc have special charaters? Would it be a good idea if we encode the URI before sending it?
|
|
||
| # Validate authentication parameters | ||
| self._validate_auth_params() | ||
|
|
There was a problem hiding this comment.
What do you think of adding a test_connection function during initialization? It allows us to verify the credentials are valid without executing an actual query at runtime.
Related Issues
Proposed Changes:
This PR implements multi-factor authentication (MFA) support for the Snowflake
integration to address Snowflake's enhanced security requirements that mandate
MFA for connections. The implementation adds non-interactive authentication
methods while maintaining full backward compatibility.
Key Features Added:
environment variables, eliminating user prompts as specifically requested
remains unchanged with no breaking changes
How did you test it?
tests passing)
validation
Notes for the reviewer
(lines ~257-301)
(lines ~152-185)
sensitive data never appears in logs
prompts" requirement
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.