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 support for Snowflake Key Pair Authentication #1232

Merged

Conversation

alexyer
Copy link

@alexyer alexyer commented Jan 10, 2019

This PR adds support for Snowflake Key Pair Authentication.

Unit tests verify everything's getting passed through correctly.

@drewbanin
Copy link
Contributor

Thanks for the PR @alexyer! I love this feature, but the cryptography.hazmat imports are giving me some pause. It looks like Snowflake recommends doing it this way, but I wonder if there isn't a higher-level approach that we could take here. @beckjake do you have any thoughts on this?

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

hazmat is where the cryptography library stores all the crytpo primitives people actually use because they want you to use higher-level constructs. This is just encoding/serialization so there is no higher level construct to use.

I don't think I've ever accomplished a single useful thing from that library without calling into hazmat. This looks good to me.

@drewbanin drewbanin added this to the Stephen Girard milestone Jan 10, 2019
@drewbanin
Copy link
Contributor

@alexyer I gave this a spin locally and it works great! I'll kick off the full test suite and then I think we'll be good to merge :)

@alexyer
Copy link
Author

alexyer commented Jan 10, 2019

@drewbanin great, thanks for the review!

@drewbanin
Copy link
Contributor

Looks like the tests failed on the unittest.mock import. dbt is currently tested on python 2 & 3, and I think unittest.mock is exclusive to py3

@alexyer
Copy link
Author

alexyer commented Jan 10, 2019

Oops, my bad. I'll update my PR.

@alexyer alexyer force-pushed the feature/snowflake-ssh-login branch 2 times, most recently from 6df8a73 to 5edeb1a Compare January 11, 2019 09:13
@drewbanin
Copy link
Contributor

hey @alexyer - I just kicked off another build for this, and it looks like there's a minor error with the unit tests. I'd love to get this merged, let me know how I can help!

This PR adds support for Snowflake Key Pair Authentication.

Unit tests verify everything's getting passed through correctly.
@alexyer
Copy link
Author

alexyer commented Jan 17, 2019

@drewbanin just a minor typo. Should be fixed now.

@drewbanin drewbanin merged commit b7d9eec into dbt-labs:dev/stephen-girard Jan 21, 2019
@drewbanin
Copy link
Contributor

Thanks @alexyer - merged :)

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.

3 participants