Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Adding protected method to convert PKCS12 key to PEM. #115

Merged
merged 2 commits into from
Jan 15, 2015
Merged

Adding protected method to convert PKCS12 key to PEM. #115

merged 2 commits into from
Jan 15, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jan 14, 2015

@craigcitro LMK if you think this should be a standalone function (possibly in crypt.py) instead of a protected method on SignedJwtAssertionCredentials.

@dhermes dhermes mentioned this pull request Jan 14, 2015
Closed
@dhermes dhermes force-pushed the convert-pkcs12-to-pem branch from d89836d to fcff4dc Compare January 14, 2015 03:10
@craigcitro
Copy link
Contributor

+1, i'd rather see it as a separate function (which i suspect could be used elsewhere).

@dhermes dhermes force-pushed the convert-pkcs12-to-pem branch from fcff4dc to bb2e770 Compare January 15, 2015 01:02
@dhermes
Copy link
Contributor Author

dhermes commented Jan 15, 2015

@craigcitro I factored out and made it public in oauth2client.crypt. I'm not sure how I feel about

  • Putting the tests in test_jwt (but there is no test_crypt).
  • Raising an ImportError at runtime. I can "resolve" this by just having the function convert PKCS12 instead of trying to also handle existing PEM keys.

@craigcitro
Copy link
Contributor

  • for the tests, why not just make a test_crypt.py?
  • i agree about the ImportError, but i don't see an obvious clean answer. i'm fine with (1) doing either an ImportError or NotImplementedError and (2) filing an issue to deal with the openssl dependency. (ultimately it'd be nice to just drop it.)

@dhermes
Copy link
Contributor Author

dhermes commented Jan 15, 2015

OpenSSL is a conditional dependency, that's the whole point of crypt.py (graceful degradation).

If I tear our the PEM part, then I can define this conditionally as OpenSSLVerifier and OpenSSLSigner are done.

(I have talked myself out of the ImportError.)

@craigcitro
Copy link
Contributor

wfm

@dhermes
Copy link
Contributor Author

dhermes commented Jan 15, 2015

@craigcitro PTAL

@dhermes
Moving private_key_as_pem->pkcs12_key_as_pem.
4d02099 
Also only defining if OpenSSL is installed and conditionally
defining a method which raises NotImplementedError if not
defined.
@dhermes dhermes force-pushed the convert-pkcs12-to-pem branch from 4889315 to 4d02099 Compare January 15, 2015 04:25
craigcitro
craigcitro reviewed Jan 15, 2015
View reviewed changes
tests/test_crypt.py


def datafile(filename):
f = open(os.path.join(os.path.dirname(__file__), 'data', filename), 'rb')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

craigcitro added a commit that referenced this pull request Jan 15, 2015
@craigcitro
Merge pull request #115 from dhermes/convert-pkcs12-to-pem
f230907 
Adding protected method to convert PKCS12 key to PEM.
@craigcitro craigcitro merged commit f230907 into googleapis:master Jan 15, 2015
@dhermes dhermes deleted the convert-pkcs12-to-pem branch January 15, 2015 08:10
@dhermes
Copy link
Contributor Author

dhermes commented Jan 15, 2015

Thanks Craig!

In the interest of closing googleapis/google-cloud-python#537, when do you think a new release will be cut?

@craigcitro
Copy link
Contributor

i should be able to cut a release tomorrow.

@craigcitro
Copy link
Contributor

actually, let's make that friday -- so that i can be teaching two other folks to do it at the same time.

@craigcitro
Copy link
Contributor

new release is out!

@dhermes
Copy link
Contributor Author

dhermes commented Jan 16, 2015

Thanks! I noticed: googleapis/google-cloud-python#559

@dhermes dhermes mentioned this pull request Jan 16, 2015
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhermes @craigcitro