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.

Sign up for GitHub

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

Jump to bottom

Add key authentication option #11180

Merged
merged 13 commits into from
Jan 25, 2022
Merged

Add key authentication option #11180

merged 13 commits into from
Jan 25, 2022

Conversation

coignetp
Copy link
Contributor

@coignetp coignetp commented Jan 20, 2022 •
edited
Loading

What does this PR do?

Add private_key_path and private_key_password options so users can connect to snowflake using key pair authentication (see https://docs.snowflake.com/en/user-guide/python-connector-example.html#using-key-pair-authentication-key-pair-rotation)

Motivation

Feature request

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@coignetp coignetp changed the title Add kay authentication option Add key authentication option Jan 20, 2022
@coignetp coignetp mentioned this pull request Jan 21, 2022
Merged
4 tasks
coignetp
coignetp commented Jan 21, 2022
View reviewed changes
snowflake/tests/keys/rsa_key_example.p8
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCXeqIH2MXr6E7p
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used example key in voltdb

integrations-core/voltdb/tests/compose/certs/client.pem

Lines 5 to 32 in 119bbcc

-----BEGIN PRIVATE KEY-----
MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCXeqIH2MXr6E7p
utpe+ZC/31oMMgXi7EPqjeNdbEMkM5c1hCLSyy762mlafkvxqEaL+OIEamZXDtOq
/cenVogdu5gvP/mMkijxWnMg7CNOtg6ulME2UqOPiMRQRJm9iXFoqx1xLSVSUh08
0ydbh2k8+yav/eRqaAJyREOE0IvafyeG1ocHTL/D6eHos3epB9T4OLXS6UjOvLoJ
9eu4GLj7R6GuMB1X15RlmYP+XLWWO2pKjlfBdY8KxQ48Q5GQHpKV10ytuH36VO0q
UvGRy5svzQEcI7VX/HCLkiFtbL7yQ+gyKACYql2CpBaBR0IDYAyx3KjrFdLSVkZ+
goWNcTThAgMBAAECggEAYPKDsTHzukA8ASsth4uCMMrp/tQlIE7GSN/2nFwlvI2o
QQAIqZCQyMvwkZIzWL6yJ/Np9BmE1kMPBWjW1ypyg1UE2XjAZk9FFPMmq/N1xXKP
gvyBjBrnw31s51KAcuX8R1j8xup18HHNdJhPoCzSFc1HvWtpPRDEQA2giOhQGc40
xcz34161dj8BAeeESHaIfed7WTwBu1IBTpVmAKPJudlAf3EjiAcO/pclLS7tJwN5
qsX0K3oHkm4W1aaOLo/G4akUxJ72bS8ny9w8wvJmJKIJ93koHwJ5HnThL5UjGhCD
cAJPCdUYm1oUtKAsTkIh2SC/sGBH28Q20D5UGHms4QKBgQD4UgvXIQYIcUv0M0qA
vGxyRDsG474Q8PqnClRAEZc/NA3S0g/B+jq0kyq7J3DLBtLCDEPmjBKfAbm39D2t
TE4sebo2/CTYmJy5FgiJxdSaEoZOrPAZAJfpkN3lfohN6McIT+0YUTvdgnDk/5xo
NwPQQEfdN9OtWMqvaFMrr6gf8wKBgQCcKeSH2/t39r79LmZvxW42+dBQvhoDVR4J
OsTYx058YWdAbWBy2CccCkDdOQnuPBsqIkQy5YQ4E6dt5AqOP9Kcl4xrfyocvQpy
ULa/sLl5xZTtKf6iyRhOuZS+LSgychoc3Y50xyNKoVAMv2ddvQnEQrEDQizbO6gW
43idKEKg2wKBgAzukOlKMfs8kz0LcsTTiz5EKWLJd3uAYT1Tv2F6yQqklle1UtbC
Rk5jH6WRf0FDgLRUWTDneIzJVTesQ44D3EpaqIT2iqCxCfBloloycEj5z/7G6NYU
ftTOE5BBD64nAj5/kxRiHqEBiwmR+j4/JzawMk3l+2MarauG3lX3FuVbAoGADoVe
uLtd4MPS8pvz7oS/QOFt23Qx2wl5J4aNc1LlG2+7OCRziXpL+LGDYo7BO6PfKsXQ
7aKl7sj1EqTXzm5k2SbGaeCDO/TgGc0jkSOPu6EBviPfh6eHWRqsmBp+2GH/x5ta
ecVipLfnR6gspmzDkbpZ12G55hDgCnDQcFykBW0CgYBeYRRfvZuJVDbA7M3DygfI
DCNPpgcjaECt5Uz/3N0udBNsoXY9vRakRlhKANLYesMUd76mUjVWu6Kc+EJtyEcC
q2plTc4gowD/hhxT1Fw95p6yxz8m+eHFc8BWAaHtoGMMgK1/wm1UXYe/hQDu39g9
ojckZpT64MHda9CvnF814g==
-----END PRIVATE KEY-----

@coignetp coignetp marked this pull request as ready for review January 21, 2022 14:56
@coignetp coignetp requested review from a team as code owners January 21, 2022 14:56
sarina-dd
sarina-dd previously approved these changes Jan 21, 2022
View reviewed changes
hithwen
hithwen reviewed Jan 24, 2022
View reviewed changes
snowflake/datadog_checks/snowflake/config_models/validators.py
@@ -8,4 +8,10 @@ def initialize_instance(values, **kwargs):
if 'username' not in values and 'user' in values:
values['username'] = values['user']

if 'private_key_password' in values and 'private_key_path' not in values:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these values be used along username and password?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These values shouldn't be used alongside password
I'll open a new PR after this one is merged to refactor the whole config validation ; since I'm adding other authentication methods at the same time

@hithwen
Copy link
Contributor

hithwen commented Jan 24, 2022

Is it feasible to add an integration test that uses this authentication mechanism?

@coignetp
Copy link
Contributor Author

Is it feasible to add an integration test that uses this authentication mechanism?

Since there is no way to spin up a snowflake env with dd_environment, we can't really do that ; I improved the test and documented how to manually try the private key authentication

sarah-witt
sarah-witt requested changes Jan 24, 2022
View reviewed changes
snowflake/datadog_checks/snowflake/data/conf.yaml.example Outdated Show resolved Hide resolved
snowflake/datadog_checks/snowflake/data/conf.yaml.example Outdated Show resolved Hide resolved
snowflake/tests/test_unit.py Show resolved Hide resolved
snowflake/tests/test_unit.py Outdated Show resolved Hide resolved
snowflake/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
snowflake/datadog_checks/snowflake/check.py
@@ -82,6 +84,25 @@ def renew_token(self):
with open(self._config.token_path, 'rb', encoding="UTF-8") as f:
self._config.token = f.read()

def read_key(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

The token function is named renew_token and this is named read_key, could we harmonize these names? Additionally, why do we read the token before the connection, and read the key file while making it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to not store the key in the integration ; I'll do a bit of refactor in a next PR to do the same with the token alongside updating the validators

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@coignetp coignetp merged commit 73e8e38 into master Jan 25, 2022
@coignetp coignetp deleted the paul/sf-key-auth branch January 25, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hithwen hithwen hithwen left review comments

@sarah-witt sarah-witt sarah-witt approved these changes

@sarina-dd sarina-dd sarina-dd left review comments

Assignees
No one assigned
Labels
documentation integration/snowflake
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@coignetp @hithwen @sarah-witt @sarina-dd