testing idea to migrate from pyopenssl onto cryptography (#1)

[jvanasco]

I was testing this idea to migrate from pyopenssl onto cryptography, before being pointed to #182

This is a less backwards incompatible change than that PR.

ComparableX509 is retained, but the underlying storage is migrated from pyopenssl onto cryptography.

The init still accepts the legacy objects, but transcodes them.

I patched in support for the only getattr(ComparableX509.wrapped) in the tests. If one wanted to go crazy, there could be a full mapping there for compatibility - however I don't think wrapped attributes should be interacted with.

The actual tests pass, but precommit fails (even though i have precommit locally and i manually reordered the imports based on github runner output).

I think this approach might make it easier to move completely away from pyopenssl over a few iterations of the related projects.

[ohemorange]

Thanks for testing this out!

My main question is if the primary benefit of this PR is to preserve backwards compatibility, and if so, is that needed? Assume we want to remove ComparableX509 eventually, is there a benefit to doing this intermediary step rather than removing it now, other than not requiring a major release? Because we are ok with doing a major release and changing the API. Basically, I think that given that we want to remove ComparableX509, this PR doesn't reduce the future conversion work that will be required to do that.

Or is the goal to just remove the pyopenssl usage now, without worrying about the larger project of decoupling certbot from josepy custom types?

On a more practical note, ComparableX509LegacyTest and ComparableX509Test seem to be failing.

[jvanasco]

On a more practical note, ComparableX509LegacyTest and ComparableX509Test seem to be failing.

Ack. those were failing after submitting this PR, and supposed to be committed into my in-process branch - not the PR one. I'll get those working even if just for me.

Or is the goal to just remove the pyopenssl usage now, without worrying about the larger project of decoupling certbot from josepy custom types?

Basically that. I wanted to offer the most minimally breaking change conceivable to get Certbot off pyopenssl immediately, as I do think the concerns raised in #5 over third-party libraries utilizing certbot/acme are still relevant.

Rather than giving impacted projects a mandate to "pin or upgrade", anything affected could do a temporary migration to the .wrapped_legacy object - although that should be created on-demand if it doesn't exist. Alongside a stated deprecation policy - e.g. this interface is supported until 2.0.0 and that larger change is expected asap - impacted projects could (i) pin to "<2.0.0"; (ii) transition to use the legacy object in the interim, and (iii) work on porting their code to using Cryptography.x509 though the updated interface. Without a strategy like this, third party libraries that break will already need to put out an emergency version bump that at least has the version pinning - but something like this would give them a quick compatibility layer (anything affected references the legacy attribute), along with the new Cryptography object to migrate their code onto.

I am perhaps too influenced with SqlAlchemy, which goes to extreme measures to prepare users for API changes well in advance of breaking changes.

[jvanasco]

On a more practical note, ComparableX509LegacyTest and ComparableX509Test seem to be failing.

it seems my local virtualenv got broken or conflicted, which kept the pre-commits from running correctly or at all. rebuilding got everything to work correctly. I'm not sure why the external test is failing here for openssl build errors; it passes elsewhere.

the minimum version of cryptography had to be bumped to 42, to support timezone aware datetime objects. that version bump should probably happen in this project no matter what, as the timezone agnostic interfaces are deprecated. i was dealing with randomly failing tests, because each testrunner used a different cryptography version.

[ohemorange]

That seems like a valid motivation; I confess I was only thinking of josepy in terms of its use as a certbot dependency and hasn't realized that other projects do seem to use it as well. It's more popular than I realized.

As for the error: https://community.letsencrypt.org/t/certbot-renew-fails-no-attribute-x509-v-flag-notify-policy/215629/2

The X509_V_FLAG_NOTIFY_POLICY flag was removed from PyOpenSSL [version 23.2.0](https://github.com/pyca/pyopenssl/commit/d788a4f1ee2cfe0642ea9e0533bb840077a88ca6) and afterwards also removed from cryptography [version 42.0.0](https://github.com/pyca/cryptography/commit/654dccb8e9d26f4b95e19831f976fc53bef98544).

Thus, if your Raspbian is running a PyOpenSSL version before 23.2.0, but a cryptography version of 42.0.0, there's a mismatch. One solution would be to either upgrade PyOpenSSL to 23.2.0 or later or downgrade cryptography to a version before 42.0.0.

Sounds like since we need cryptography 42, gotta use a newer pyopenssl as well.

[jvanasco]

Ah, that explains why it works on some runs but not others.

Something I’ll do tomorrow is drop pyopenssl from requirements, and make it a conditional import (in try block). Then try to make tests run with and without pyopenssl, dropping some skip-unless options in there.

I don’t know if any of this is useful to you all, but I could use the mental exercise:)

[ohemorange]

If nothing else this discussion has certainly helped me realize how many people are using josepy which helps contextualize my thinking about it.

I'm surprised that this PR gets us to where we could just drop pyopenssl as a requirement, even if conditionally. Or is that just to get a clearer picture of where it's still needed?

[jvanasco]

I'm surprised that this PR gets us to where we could just drop pyopenssl as a requirement, even if conditionally.

That's exactly the plan. I just need to figure out how to get tox and poetry to handle this correctly. My thought was to migrate openssl into an optional dependency (i.e. pip install josepy[legacy]), then run unittests against both.

josepy would then not require openssl, but use it if available (and compatible, I've got the version check handled). affected projects would then have to specify the openssl requirement in their code - which they already should be doing as they'd be doing something with the .wrapped attribute.

[jvanasco]

ok, so this works! cryptography is optional via a josepy[legacy] extra

The test matrix is now doubled, running in "noopenssl" and "legacy" modes.

tox is invoked by github (check.yaml) into the github-noopenssl and github-legacy environments. These just set an envvar for tests JOSEPY_EXPECT_OPENSSL, and install the extras via overriding the commands_pre install. The commands needed to be repeated there too.

In legacy mode, there is no openssl installed, and an environment variable (JOSEPY_EXPECT_OPENSSL) is used for checks to ensure things work as expected AND the pyopenssl Filetype attributes are still equivalent. In order to drop the dependency on pyopenssl, I copied the FileTypes over manuall.

In noopenssl mode (default), i use conditional imports - with modules failing over to None if it's not installed. i don't think we need to worry about the version conflict (above), but maybe I understand this wrong.

The only thing not supported is the pyopenssl text filetype, but I can't imagine that being used much.

Anyways, my suggestion for an approach like this would be :

• breaking change with a 2.0.0 release
• legacy support of pyopenssl through 2.1.0, then drop it completely

That would allow for certbot (and related projects) to drop pyopenssl for cryptography ASAP, and to drop pyopenssl from josepy in the near future - while giving any 3rd party projects a migration tool and minimally breaking their projects.

As mentioned above - any breaking changes against unpinned projects will already require a hotfix version bump release, so instead of pinning to ==1.15.0 they can pin to <2.1.0 (or whatever is planned). Most affected projects should be able to unbreak by switching to the .wrapped_legacy attribute - which should give them what they expect.

This was annoying, but fun, to get github+tox+poetry to work together nicely.

[ohemorange]

Appreciate all the work you put in here!

While I love the thoughtfulness for helping people migrate, I don't personally see the benefit of having people switch to wrapped_legacy and pin to <2.1.0 vs. just having them pin to <2.0.0. It's an extra step that doesn't actually move the code closer to being migrated. Especially since, as you've now seen, it makes testing and packaging more complicated -- and being a security-focused project, I'm always hesitant to increase complexity. I therefore think this change would be cleaner overall if we just go ahead and drop pyopenssl in one go.

[jvanasco]

I'm fine with dropping .wrapped_legacy, but there would still be the packaging/testing concerns with accepting PyOpenSSL objects and transcoding them to Cryptography. While I would like to not do that (and that would just mean deleting a lot of lines in this PR - so it's super easy), there would need to be a coordinated release strategy with certbot and acme - because they would break against this.

I think most of the breakage in those would be through tests; from what I remember the library code mostly doesn't rely on the internals of this object.

[ohemorange]

Not to worry about certbot/acme; both are pinned to josepy <2.0 for exactly this reason. What do you mean the packaging/testing concerns about accepting pyopenssl objects? Ideally we would no longer do so, no?

[jvanasco]

This PR accepts the pyopenssl objects and converts them to cryptography objects. I can generate another pr that doesn’t do any of that.The main idea behind this was that it automatically upgrades legacy objects, and also offers a legacy interface, so projects can drop pyopenssl asap.  I wanted to make it as easy and fast for everyone to get off it asap.I’ll make a no OpenSSL pr in another branch.On Dec 20, 2024, at 8:14 PM, ohemorange ***@***.***> wrote: Not to worry about certbot/acme; both are pinned to josepy <2.0 for exactly this reason. What do you mean the packaging/testing concerns about accepting pyopenssl objects? Ideally we would no longer do so, no? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>