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

Adding cryptography Signer. #185

Merged
merged 6 commits into from
Feb 8, 2018
Merged

Adding cryptography Signer. #185

merged 6 commits into from
Feb 8, 2018

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 5, 2017 •
edited
Loading

Fixes #183.

NOTE: This change is incomplete. It needs unit tests and a Verifier implementation.

Big H/T to @arthurdarcet for the implementation.

dhermes
dhermes commented Aug 5, 2017
View reviewed changes
google/auth/_service_account_info.py Outdated
if _cryptography_impl is None:
DefaultSigner = crypt.RSASigner
else:
DefaultSigner = _cryptography_impl.CryptographySigner

This comment was marked as spam.

Sign in to view

google/auth/crypt.py Outdated
@@ -206,7 +206,59 @@ def sign(self, message):
raise NotImplementedError('Sign must be implemented')


class RSASigner(Signer):
class _FromServiceAccountMixin(object):

This comment was marked as spam.

Sign in to view

@theacodes
Copy link
Contributor

@dhermes how would you feel about splitting crypt into a package with the following structure?

crypt/
    __init__.py
    _helpers.py
    base.py
    rsa.py
    _python_rsa.py
    _cryptography_rsa.py
    ecc.py

__init__.py could keep the current module's interface and rsa could conditionally import the classes from _cryptography_rsa.py or fallback to _python_rsa.py.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 7, 2017

SGTM. Care to do the re-org in a separate PR without the cryptography impl. and then I can rebase this PR after that?

@theacodes
Copy link
Contributor

Ya. Will do (soonish, I hope)

@theacodes
Copy link
Contributor

Alright, with #189 in you should be good to rebase. :)

dhermes
dhermes commented Aug 11, 2017
View reviewed changes
google/auth/crypt/base.py Outdated
@@ -62,3 +68,51 @@ def sign(self, message):
# pylint: disable=missing-raises-doc,redundant-returns-doc
# (pylint doesn't recognize that this is abstract)
raise NotImplementedError('Sign must be implemented')


class _FromServiceAccountMixin(object):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes
dhermes commented Aug 11, 2017
View reviewed changes
google/auth/crypt/rsa.py Outdated
from google.auth.crypt import _python_rsa

RSASigner = _python_rsa.RSASigner
RSAVerifier = _python_rsa.RSAVerifier

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor Author

dhermes commented Aug 11, 2017

@jonparrott Officially rebased (unit tests and Verifier impl. still not done)

theacodes
theacodes reviewed Aug 14, 2017
View reviewed changes
Copy link
Contributor

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

Looks mostly good so far.

google/auth/crypt/_cryptography_rsa.py Outdated
# See the License for the specific language governing permissions and
# limitations under the License.

"""Verifier and signer that use the ``cryptography`` library.

This comment was marked as spam.

Sign in to view

google/auth/crypt/_cryptography_rsa.py Outdated
_SHA256 = hashes.SHA256()


class CryptographySigner(base.Signer, base._FromServiceAccountMixin):

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

This comment was marked as spam.

Sign in to view

google/auth/crypt/base.py Outdated
@@ -62,3 +68,51 @@ def sign(self, message):
# pylint: disable=missing-raises-doc,redundant-returns-doc
# (pylint doesn't recognize that this is abstract)
raise NotImplementedError('Sign must be implemented')


class _FromServiceAccountMixin(object):

This comment was marked as spam.

Sign in to view

google/auth/crypt/rsa.py Outdated
from google.auth.crypt import _python_rsa

RSASigner = _python_rsa.RSASigner
RSAVerifier = _python_rsa.RSAVerifier

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor Author

dhermes commented Aug 14, 2017

@jonparrott Updates sent for your review comments (including CryptographySigner -> RSASigner)

@thomas-riccardi
Copy link

Is there any plan to complete this PR?

@theacodes
Copy link
Contributor

@thomas-riccardi yes, it's just low on the list as we already have working implementation here and this is just a "speed up" PR.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 2, 2018

@thomas-riccardi We don't actually have a local "expert" that knows the relevant methods to use for Verifier, so changes are welcome.

@jonparrott We could factor out the base class parts so this PR was more focused, but I don't know if it's worth it.

@theacodes
Copy link
Contributor

@jonparrott We could factor out the base class parts so this PR was more focused, but I don't know if it's worth it.

Up to you.

dhermes and others added 5 commits February 8, 2018 15:09
@dhermes
Adding cryptography Signer.
b70bb8e 
Fixes googleapis#183.

NOTE: This change is incomplete. It needs unit tests and a Verifier
      implementation.
@dhermes
Renaming cryptography signer.
feb9676 
Also making "RSA" explicit in `_cryptography_rsa` module docstring.
A little reorganization and a few tests
b20aaa2
Flesh out verifier implementation
5644abb
Final cleanup
b26c210
@theacodes
Copy link
Contributor

Alright, I've brought this to the finish line with tests and a verifier implementation. Thank you @dhermes for getting this party started.

Will merge once CI is green. :)

@theacodes theacodes merged commit 1cd8390 into googleapis:master Feb 8, 2018
@dhermes dhermes deleted the fix-183 branch February 12, 2018 18:51
@dhermes
Copy link
Contributor Author

dhermes commented Feb 12, 2018

Thanks @jonparrott!

@ktdreyer ktdreyer mentioned this pull request Jan 12, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theacodes theacodes theacodes 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.
3 participants
@dhermes @theacodes @thomas-riccardi