Skip to content
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

Fix connection caching for MFA and external browser authenticators #705

Merged
merged 4 commits into from
Jan 24, 2023

Conversation

sfc-gh-ext-simba-lb
Copy link
Contributor

@sfc-gh-ext-simba-lb sfc-gh-ext-simba-lb commented Jan 23, 2023

Description

https://github.com/snowflakedb/snowflake-sdks-drivers-issues-teamwork/issues/119

Connection caching was added in version 1.6.15 as part of #661 but is not working as expected.

The PR added CLIENT_REQUEST_MFA_TOKEN/CLIENT_STORE_TEMPORARY_CREDENTIAL properties in the authentication request so the server would send back MFA/ID token. The implementation was sending the session parameter value as string "true" instead of boolean true.

This PR is to change the type of CLIENT_REQUEST_MFA_TOKEN/CLIENT_STORE_TEMPORARY_CREDENTIAL from string to boolean so that it is recognized by the server, and returns the token so it can be cached.

Checklist

  • Code compiles correctly
  • Run make fmt to fix inconsistent formats
  • Run make lint to get lint errors and fix all of them
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@sfc-gh-ext-simba-lb sfc-gh-ext-simba-lb marked this pull request as ready for review January 23, 2023 18:26
Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. As a side notes, if we don't support mfa caching for Linux , as JDBC/ODBC support then how do we track it? Can you create a ticket to track it? we need to support mfa caching on Linux as other drivers although it's not most secure way of doing it.

@sfc-gh-ext-simba-lb
Copy link
Contributor Author

@sfc-gh-igarish the go driver supports writing to local cache file the same as ODBC and JDBC: https://github.com/snowflakedb/gosnowflake/blob/master/secure_storage_manager.go#L239

For Linux it's disabled by default the same as JDBC and ODBC. We use the CLIENT_STORE_TEMPORARY_CREDENTIAL and CLIENT_REQUEST_MFA_TOKEN session parameters to decide whether to cache the token or not. For Windows and Mac OS these properties will be set to true by default if the authenticator is external browser or MFA, but for Linux they are disabled by default.

i.e JDBC: https://github.com/snowflakedb/snowflake-jdbc/blob/master/src/main/java/net/snowflake/client/core/SessionUtil.java#L261

@sfc-gh-igarish
Copy link
Collaborator

Can we make sure this feature work on all supported platforms? i.e. Windows, Linux, and Mac.

@sfc-gh-ext-simba-lb
Copy link
Contributor Author

@sfc-gh-igarish Added the Linux option to the test and confirmed the feature on Windows, Linux and OSX.

@sfc-gh-ext-simba-lb sfc-gh-ext-simba-lb merged commit c0b333e into master Jan 24, 2023
@sfc-gh-ext-simba-lb sfc-gh-ext-simba-lb deleted the fixConnectionCaching branch January 24, 2023 22:49
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants