-
Notifications
You must be signed in to change notification settings - Fork 20
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
Adding support for Private key file based authentication #149
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just left a few clarifying questions, perhaps due to my unfamiliarity with private key sign-in.
@@ -169,6 +169,28 @@ set SNOWFLAKE_AUTHENTICATOR=externalbrowser | |||
os.environ['SNOWFLAKE_AUTHENTICATOR'] = 'externalbrowser' | |||
``` | |||
|
|||
4. Private Key based authentication | |||
```bash | |||
SNOWFLAKE_AUTHENTICATOR= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be confusing to some folks about setting this to the empty string, I'd just leave a comment that this doesn't need to be set:
SNOWFLAKE_AUTHENTICATOR= | |
# no SNOWFLAKE_AUTHENTICATOR needed |
if not SNOWFLAKE_PASSWORD and not SNOWFLAKE_AUTHENTICATOR: | ||
missing_env_vars.append("SNOWFLAKE_PASSWORD/SNOWFLAKE_AUTHENTICATOR") | ||
if not SNOWFLAKE_AUTHENTICATOR: | ||
if not SNOWFLAKE_PASSWORD and not SNOWFLAKE_PRIVATE_KEY_FILE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming SNOWFLAKE_PASSWORD
and SNOWFLAKE_PRIVATE_KEY_FILE
are mutually exclusive (i.e. you can have one or the other but not both)? If that's the case, it might be nice to make the message a little clearer to the user:
if not SNOWFLAKE_PASSWORD and not SNOWFLAKE_PRIVATE_KEY_FILE: | |
if not SNOWFLAKE_PASSWORD: | |
missing_env_vars.append("SNOWFLAKE_PASSWORD/SNOWFLAKE_AUTHENTICATOR") | |
elif not SNOWFLAKE_PRIVATE_KEY_FILE: | |
missing_env_vars.append("SNOWFLAKE_PRIVATE_KEY_FILE") |
@@ -417,9 +417,18 @@ def _get_user(self) -> str: | |||
) | |||
return user | |||
|
|||
def _is_authentication_setup(self) -> bool: | |||
password = env_vars.SNOWFLAKE_PASSWORD | |||
private_key_file = env_vars.SNOWFLAKE_PRIVATE_KEY_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to be also verifying the presence of SNOWFLAKE_PRIVATE_KEY_FILE_PWD
?
Please review my changes for adding private key file based authentication.
I have tested the changes locally.