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

deprecate signer/verifier on asymmetric keys #3663

Merged
merged 2 commits into from
Jun 4, 2017

Conversation

reaperhulk
Copy link
Member

fixes #3659

@mention-bot
Copy link

@reaperhulk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alex, @public and @gorisaka to be potential reviewers.


.. doctest::

>>> public_key = private_key.public_key()
Copy link
Member

Choose a reason for hiding this comment

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

Drop this assigment, we already have a public_key


.. doctest::

>>> public_key = private_key.public_key()
Copy link
Member

Choose a reason for hiding this comment

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

same


.. doctest::

>>> public_key = private_key.public_key()
Copy link
Member

Choose a reason for hiding this comment

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

once more, with feelign!

# Asymmetric signature and verification contexts were deprecated in 2.0
# However they are widely used and we will obey a longer than normal
# deprecation cycle for them.
PersistentlyDeprecatedIn20 = PendingDeprecationWarning
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to use the same PersistentlyDeprecated class, or is your expectation that we'll migrate to them once this switches from Pending to regular?

Copy link
Member Author

Choose a reason for hiding this comment

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

My main thinking was that it's useful to know at what point we persistently deprecated something. By grouping the persistent deprecations we have to look at the repository history or changelog to know how long it's been. Having written those words I think I'm going to just use PersistentlyDeprecated though, because looking at the changelog is not a burden.

@reaperhulk
Copy link
Member Author

I don't know how to write a test that catches this deprecation warning actually. It happens once and then is suppressed after that, but we also run a random test order job so we can't introduce an order dependent test. warnings.simplefilter("always", PendingDeprecationWarning) doesn't appear to work either. I assume I'm missing something -- ideas?

@alex
Copy link
Member

alex commented Jun 3, 2017

I'm very confused, I would have expected the code as written to work.

@reaperhulk
Copy link
Member Author

I can't reproduce this except in our tests (which I got down to two calls to signer). I have no idea what's going on.

@alex
Copy link
Member

alex commented Jun 4, 2017

Meaning if you copy the tests out of our overall suite, into it's own suite, you still can't reproduce?

@reaperhulk
Copy link
Member Author

This reproduces it in its own file:

import pytest

from cryptography.hazmat.backends.openssl.backend import backend
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.asymmetric import padding, rsa


def test_signer_deprecated():
    private_key = rsa.generate_private_key(65537, 512, backend)
    private_key.signer(
        padding.PKCS1v15(),
        hashes.SHA1()
    )


def test_signer_check_deprecated():
    private_key = rsa.generate_private_key(65537, 512, backend)
    with pytest.deprecated_call():
        private_key.signer(
            padding.PKCS1v15(),
            hashes.SHA1()
        )

@reaperhulk
Copy link
Member Author

Notably if you put the contextmanager around the first call then it all works.

@alex
Copy link
Member

alex commented Jun 4, 2017

Interesting! And what happens if you replace .signer() with just a function that does nothing but call warnings.warn?

@reaperhulk
Copy link
Member Author

It works then:

import warnings

import pytest

from cryptography.hazmat.backends.openssl.backend import backend
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.asymmetric import padding, rsa


def warn_me():
    warnings.warn("deprecated", PendingDeprecationWarning, stacklevel=2)


def test_signer_deprecated():
    private_key = rsa.generate_private_key(65537, 512, backend)
    private_key.signer(
        padding.PKCS1v15(),
        hashes.SHA1()
    )


def test_signer_check_deprecated():
    private_key = rsa.generate_private_key(65537, 512, backend)
    with pytest.deprecated_call():
        private_key.signer(
            padding.PKCS1v15(),
            hashes.SHA1()
        )


def test_warn_me():
    warn_me()


def test_warn_me_check_deprecated():
    with pytest.deprecated_call():
        warn_me()

So the output of that set of tests is: .F..

@alex
Copy link
Member

alex commented Jun 4, 2017

wat.

Does this mean that somewhere in the test suite there's a signer or verifier call that isn't marked as deprecated_call?

@reaperhulk
Copy link
Member Author

There's a ton of those actually (we call signer/verifier...a lot). But here's a minimal reproducer!

import warnings

import pytest


def warn_me():
    warnings.warn("deprecated", PendingDeprecationWarning, stacklevel=2)


def i_call_warn_me():
    warn_me()


def test_warn_me():
    i_call_warn_me()


def test_warn_me_check_deprecated():
    with pytest.deprecated_call():
        i_call_warn_me()

@alex
Copy link
Member

alex commented Jun 4, 2017

So.... wtf. File a pytest bug I guess?

@reaperhulk
Copy link
Member Author

pytest-dev/pytest#2469

@alex alex merged commit 1a5d70e into pyca:master Jun 4, 2017
@reaperhulk reaperhulk deleted the deprecate-asym-ctx branch June 4, 2017 03:12
ltm added a commit to ltm/boto3 that referenced this pull request Mar 30, 2018
The RSA `signer` was deprecated in favor of `sign` in version 2.0 of
cryptography (pyca/cryptography#3663) and currently emits a deprecation
warning. The single shot `sign` method has been available since version
1.4.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Asymmetric Signature and Verification Contexts
3 participants