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

SNOW-1869750 add check for empty private key before trying to generate JWT from it #1285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sfc-gh-dszmolka
Copy link
Contributor

@sfc-gh-dszmolka sfc-gh-dszmolka commented Dec 31, 2024

Description

This is trying to address an Issue coming from a Snowflake Terraform Provider use-case (Snowflake-Labs/terraform-provider-snowflake#3322) but perhaps beneficial without the TF Provider as well.

The current behaviour is that the driver panics when no private key is specified to be used with keypair auth.

Users of the Provider can choose for keypair auth, and can specify the details needed to set up the connection (like user, account, etc, also the private key), multiple ways:

  • using a config file
  • using the provider block in their main.tf
  • (issue comes from this scenario) using environmental variables . Specifically for the private key, this is SNOWFLAKE_PRIVATE_KEY

It is entirely legal to omit the account configuration from config file, it's no problem if you omit it from the provider block as well, but then you need to make sure you have the corresponding envvar for each setting.

Now. When the authenticator (snowflake_jwt) is specified correctly, thus the driver will want to use AuthTypeJwt, but private key doesn't exist because the user forgot to specify it in all the possible places, then when driver attempts to parse the private key to prepare the JWT token, this step

pubBytes, err := x509.MarshalPKIXPublicKey(config.PrivateKey.Public())

will surely crash with a panic when config.PrivateKey is not specified, and is therefore nilwhen calling the Public() function on it.

The aim of this PR is to detect this condition before the panic can happen and error out gracefully with a descriptive error message so the user would know where to look for the fix.

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.

1 participant