Skip to content

Passing encrypted key as credential #270

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
Merged
merged 2 commits into from
Oct 30, 2020
Merged

Passing encrypted key as credential #270

merged 2 commits into from
Oct 30, 2020

Conversation

abhidnya13
Copy link
Contributor

@abhidnya13 abhidnya13 commented Oct 23, 2020 •
edited by rayluo
Loading

Usage: We add one more optional field passphrase in the existing client_credential parameter of ConfidentialClientApplication. The docs will also be updated here

@abhidnya13 abhidnya13 force-pushed the private-key-with-password branch 3 times, most recently from f614596 to 50800e2 Compare October 23, 2020 14:52
@abhidnya13 abhidnya13 requested a review from rayluo October 23, 2020 17:56
@abhidnya13 abhidnya13 linked an issue Oct 27, 2020 that may be closed by this pull request
Accept an encrypted key as client credential #232
Closed
henrik-me
henrik-me approved these changes Oct 27, 2020
View reviewed changes
Copy link

@henrik-me henrik-me left a comment

Choose a reason for hiding this comment

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

:shipit:

@henrik-me
Copy link

unit test?

rayluo
rayluo reviewed Oct 27, 2020
View reviewed changes
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Overall it looks good! Leave a couple minor comment below to hear your thoughts.

rayluo
rayluo requested changes Oct 28, 2020
View reviewed changes
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks! Per our discussion yesterday, please also add the two new dependencies into our setup.py (otherwise it would break). We can probably use the same version requirements used by Azure SDK.

After that, this would be good to go!

@abhidnya13 abhidnya13 requested a review from rayluo October 29, 2020 02:43
@abhidnya13
Copy link
Contributor Author

I see the unit tests are removed in this commit , can we get them back ?
@rayluo any reason not to have them there ?

@abhidnya13
Copy link
Contributor Author

Thanks for adding the tests @rayluo ! Besides, I have also manually tested this scenario e2e using an encrypted private key and verified it works as expected.

@rayluo rayluo force-pushed the private-key-with-password branch 2 times, most recently from cc3b696 to 2ded277 Compare October 30, 2020 19:06
abhidnya13 and others added 2 commits October 30, 2020 12:07
@abhidnya13 @rayluo
Initial commit
88e9b22 
Code refactoring

Changing reference doc string

Adding tests

PR review 1

Adding dependencies and polishing code

Python 2 compat
@rayluo
Removing dependency of six
2ded277 
Adding missing arguments to api call

Use cryptography lower bound as low as 0.6

Add test cases for _str2bytes()

Choose cryptography upper bound as <4
rayluo
rayluo approved these changes Oct 30, 2020
View reviewed changes
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks Abhi! We finished this together, in pair-programming style! :-D

@rayluo rayluo merged commit c37f36b into dev Oct 30, 2020
@rayluo rayluo deleted the private-key-with-password branch October 30, 2020 22:08
@rayluo rayluo mentioned this pull request Nov 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rayluo rayluo rayluo approved these changes

@henrik-me henrik-me henrik-me approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accept an encrypted key as client credential
3 participants
@abhidnya13 @henrik-me @rayluo