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

Merged

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.

@sfc-gh-dszmolka sfc-gh-dszmolka requested a review from a team January 13, 2025 09:33
@sfc-gh-dszmolka sfc-gh-dszmolka enabled auto-merge (squash) January 16, 2025 09:25
@sfc-gh-dszmolka sfc-gh-dszmolka enabled auto-merge (squash) January 16, 2025 10:44
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.25%. Comparing base (1663004) to head (cf5a2e5).

Files with missing lines Patch % Lines
auth.go 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1285      +/-   ##
==========================================
+ Coverage   82.23%   82.25%   +0.01%     
==========================================
  Files          55       55              
  Lines       13574    13577       +3     
==========================================
+ Hits        11163    11168       +5     
+ Misses       2411     2409       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sfc-gh-dszmolka sfc-gh-dszmolka merged commit 7f77aea into master Jan 16, 2025
38 of 39 checks passed
@sfc-gh-dszmolka sfc-gh-dszmolka deleted the SNOW-1869750-return-error-when-no-privatekey-provided branch January 16, 2025 13:26
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
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.

3 participants