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

Add client credential caching option #1913

Closed

Conversation

jcourteau
Copy link

@jcourteau jcourteau commented Jun 27, 2023

  • Add new provider configuration option - cache_temporary_credential, default false

This is normally automatically enabled or disabled based on the platform on the upstream gosnowflake SDK. However, even on mac, with the option enabled on the Snowflake account, the provider wasn't using caching.

This PR adds an explicit flag to enable / disable credential caching so that the exact behaviour can be set. Since Linux doesn't offer a password store like Windows and Mac do, this feature is a bit of a security risk there.

This must be enabled on the snowflake account

Specifically, as ACCOUNTADMIN:

alter account set allow_id_token = true;

Test Plan

  • acceptance tests
  • added unit tests for the generated DSN, following existing examples
  • updated existing unit tests to reflect new config option

I also verified that caching worked with a provider config like this:

provider "snowflake" {
  account  = "<my SF account>"
  username = "<me>"
  region   = "us-west-2"

  cache_temporary_credential = true
  browser_auth               = true

  role = "SECURITYADMIN"
}

With cache_temporary_credential=true I was prompted to save the password in the OSX keychain; and subsequent runs didn't prompt for credentials.

With it set to false, I went through the normal browser auth cycle, with no caching; the same as the pre-patch behaviour.

(I would appreciate if someone could test this on Windows, as I don't have that readily available.)

References

This is normally _disabled_ for Linux on the upstream gosnowflake SDK;
because Linux doesn't offer a password store like Windows and Mac do. I
didn't add platform-detecting behaviour here to avoid surprises; instead
hopefully the docs should indicate whether this is something you'd want
or not.

This must be enabled on the snowflake account. See https://docs.snowflake.com/en/user-guide/admin-security-fed-auth-use#using-connection-caching-to-minimize-the-number-of-prompts-for-authentication-optional

Specifically, as ACCOUNTADMINISTRATOR:

```
alter account set allow_id_token = true;
```
@cablespaghetti
Copy link

@sfc-gh-jcieslak @sfc-gh-asawicki @sfc-gh-swinkler sorry for the mention but is there any possibility of this getting merged soon? Without it the terraform provider unusable with SSO/Browser auth.

@mlavaert
Copy link

I hoped that this was going to be included in PR #2109 to use the authenticator. Sadly browser authentication is still impossible to use as it opens 100s of browser tabs right now.

@sfc-gh-swinkler
Copy link
Collaborator

sfc-gh-swinkler commented Jan 23, 2024

This has already been added: https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs#client_store_temporary_credential

Closing as completed

Note that we are still having issues with the token for Windows / Darwin not caching token despite adding this. We need to build these two binaries with CGO enabled, and this is proving to be somewhat challenging to do in github-actions. We hope to get a solution out soon

@cablespaghetti
Copy link

This has already been added: https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs#client_store_temporary_credential

Closing as completed

Note that we are still having issues with the token for Windows / Darwin not caching token despite adding this. We need to build these two binaries with CGO enabled, and this is proving to be somewhat challenging to do in github-actions. We hope to get a solution out soon

As this feature is not working correctly, is this being tracked in an issue I can watch?

Thanks

@sfc-gh-swinkler
Copy link
Collaborator

@cablespaghetti you can check the related issue #2047. We will close this issue only once the feature is working correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants