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

Added support for pure Python RSA library when present. #1

Closed
wants to merge 5 commits into from
Closed

Added support for pure Python RSA library when present. #1

wants to merge 5 commits into from

Conversation

aeijdenberg
Copy link

Happy to add tests later once the tests are part of the main repo.

asn1_cert['tbsCertificate']['subjectPublicKeyInfo']['subjectPublicKey']), 'DER')
except ImportError:
raise NotImplementedError(
'Please install pyasn1 and pyasn1_module to parse x509.')

This comment was marked as spam.

This comment was marked as spam.

@craigcitro
Copy link
Contributor

i'm good with leaving serious testing for a later patch -- but if possible, could you throw a few example input/output pairs into the BitStringToByteString docstring? even if we're testing them by hand, i'd like to have a clear record of what we expected that to do.

@skelterjohn
Copy link
Contributor

I've added the test framework.

Please update your PR to include tests!

return RsaSigner(pkey)

except ImportError:
RsaSigner = None

This comment was marked as spam.

This comment was marked as spam.

pkey = RSA.importKey(key)
else:
raise NotImplementedError(
'PKCS12 format is not supported by the PyCrpto library. '

This comment was marked as spam.

This comment was marked as spam.

@skelterjohn
Copy link
Contributor

You'll probably have to redo this PR - I've updated this repo using the history from the old googlecode repo, so things will probably not match. On the other hand, that transition has now taken place, so we can actually act on this pull request once it's settled.

@aeijdenberg
Copy link
Author

John - it's unlikely I'll have time. Would you be able to pick it up?

On Thu, Apr 24, 2014 at 1:40 PM, John Asmuth notifications@github.comwrote:

You'll probably have to redo this PR - I've updated this repo using the
history from the old googlecode repo, so things will probably not match. On
the other hand, that transition has now taken place, so we can actually act
on this pull request once it's settled.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-41329388
.

@craigcitro
Copy link
Contributor

I can if John can't, but not for another week or so.


-cc

sent from my phone
On Apr 24, 2014 4:26 PM, "aeijdenberg" notifications@github.com wrote:

John - it's unlikely I'll have time. Would you be able to pick it up?

On Thu, Apr 24, 2014 at 1:40 PM, John Asmuth notifications@github.comwrote:

You'll probably have to redo this PR - I've updated this repo using the
history from the old googlecode repo, so things will probably not match.
On
the other hand, that transition has now taken place, so we can actually
act
on this pull request once it's settled.


Reply to this email directly or view it on GitHub<
https://github.com/google/oauth2client/pull/1#issuecomment-41329388>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-41344670
.

@aeijdenberg
Copy link
Author

Thank you.

On Thu, Apr 24, 2014 at 6:17 PM, Craig Citro notifications@github.comwrote:

I can if John can't, but not for another week or so.


-cc

sent from my phone
On Apr 24, 2014 4:26 PM, "aeijdenberg" notifications@github.com wrote:

John - it's unlikely I'll have time. Would you be able to pick it up?

On Thu, Apr 24, 2014 at 1:40 PM, John Asmuth notifications@github.comwrote:

You'll probably have to redo this PR - I've updated this repo using
the
history from the old googlecode repo, so things will probably not
match.
On
the other hand, that transition has now taken place, so we can
actually
act
on this pull request once it's settled.


Reply to this email directly or view it on GitHub<
https://github.com/google/oauth2client/pull/1#issuecomment-41329388>
.


Reply to this email directly or view it on GitHub<
https://github.com/google/oauth2client/pull/1#issuecomment-41344670>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-41350726
.

craigcitro pushed a commit that referenced this pull request Jan 30, 2015
Pull recent auth2client release (crypt + timeout)
@nathanielmanistaatgoogle
Copy link
Contributor

It looks like you haven't yet signed Google's Contributor License Agreement - please do so?

@aeijdenberg
Copy link
Author

Just submitted the GCLA today.

Cheers, Adam

On Tue, Mar 17, 2015 at 9:53 PM Nathaniel Manista notifications@github.com
wrote:

It looks like you haven't yet signed Google's Contributor License
Agreement - please do so?


Reply to this email directly or view it on GitHub
#1 (comment).

@dhermes dhermes closed this Aug 31, 2015
@dhermes
Copy link
Contributor

dhermes commented Aug 31, 2015

@aeijdenberg Is this not already supported? pyasn1-modules is already a dependency.

@aeijdenberg
Copy link
Author

It certainly wasn't in 2013 when I submitted the PR. :) I'll go check now.

@aeijdenberg
Copy link
Author

So it looks like the support Orest added in 2014 for AppDefaultCredentials uses the pure Python module which is a good thing, however SignedJwtAssertionCredentials appears to live on inside of client.py and as far as I can tell still won't use the pure Python libraries.

I'm happy to redo this PR when I'm next back at my desk. I actually know how to use git now so it might be easier!

@dhermes
Copy link
Contributor

dhermes commented Aug 31, 2015

Yeah I don't think Orest (or whoever else added the _service_account module) realized that SignedJwtAssertionCredentials existed (or that it could be used for service accounts).

I've been meaning to right that wrong with #211 but haven't had time (or at least time without higher priority issues). Let's continue this conversation over there (and maybe we'll end up filing a "use pure Python when necessary / possible" issue as well).

@anthmgoogle
Copy link

I have context with the changes to add the _service_account module. It was understood that SignedJwtAssertionCredentials existed, but it was tied to P12 and had a confusing name. The plan was that the new _ServiceAccountCredentials would replace as described in #211, but we kept it internal intially to avoid locking in the public surface before further review.

@dhermes
Copy link
Contributor

dhermes commented Sep 2, 2015

I like it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants