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

Add type annotations for the SSL module #1308

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Add type annotations for the SSL module #1308

merged 2 commits into from
Jun 21, 2024

Conversation

jlaine
Copy link
Contributor

@jlaine jlaine commented Jun 21, 2024 •
edited
Loading

The type annotations provided in types-OpenSSL are at best incomplete, and are bound to fall out of sync with PyOpenSSL. This PR brings type annotations to all public APIs in the SSL module.

It also changes references from unicode to str in the docstrings.

NOTE: I was not sure how to type the first argument of recv_into.

alex
alex reviewed Jun 21, 2024
View reviewed changes
@alex
Copy link
Member

alex commented Jun 21, 2024

If you rebase on main, CI should be green.

mhils
mhils reviewed Jun 21, 2024
View reviewed changes
Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

Strong +1 on inlining the annotations from types-pyopenssl, thank you for doing this @jlaine! 🍰

LGTM % @alex's comment.

@jlaine jlaine force-pushed the type-annotations branch 2 times, most recently from 2891fb2 to ab3f6a6 Compare June 21, 2024 12:42
alex
alex reviewed Jun 21, 2024
View reviewed changes
@jlaine
Copy link
Contributor Author

jlaine commented Jun 21, 2024 •
edited
Loading

I have rebased on top of main, let's see if CI is happy.

I also added int annotations to our constants, like types-OpenSSL does.

Some questions:

  • Do we only support passing a socket.socket to Connection, or is something that quacks like a socket OK too?
  • Related to the above : Connection.connect and Connection.sock_shutdown say they return "what the socket returns". If the socket is garanteed to be a socket.socket, that would be None, and there is no need to return anything. But as we might be supporting weird socket-like things, I'd rather not touch it.
  • Guidance on typing the first argument of Connection.recv_into would be welcome!
  • Do we want to ship a py.typed marker so that our typings take precedence over types-OpenSSL? => YES

As a stretch goal I was wondering whether we can get a clean mypy run on SSL.py. A couple of aspects of the code make this moderately hard:

  • _ffi.string returns bytes but mypy does know this. Do we want to add: _ffi_string: Callable[[Any], bytes] = _ffi.string ?
  • _raise_ssl_error is sometimes used in context where we know it will raise .. and sometimes not. This results in mypy complaining there is no return statement on some codepaths.
  • A number of functions are just thin wrapper around return _lib.SSL_SOMETHING(..), and mypy rightly complains.

@jlaine jlaine force-pushed the type-annotations branch 3 times, most recently from ac2cfa6 to 722931f Compare June 21, 2024 14:38
@jlaine
Add type annotations for the SSL module
6a4d360 
Instead of relying on the third-party `types-pyOpenSSL` package, provide
our own type annotations. This will ensure `pyOpenSSL` users always have
type annotations which matches their version.
@jlaine jlaine force-pushed the type-annotations branch from 722931f to 6a4d360 Compare June 21, 2024 14:41
@jlaine jlaine requested a review from alex June 21, 2024 16:26
alex
alex reviewed Jun 21, 2024
View reviewed changes
@jlaine
Copy link
Contributor Author

jlaine commented Jun 21, 2024

I can't remember what your preferred policy is, do you want me to squash the changes down or would you rather do it?

@alex alex merged commit 9eaa107 into pyca:main Jun 21, 2024
35 checks passed
@reaperhulk
Copy link
Member

We’re happy to squash ourselves.

@alex
Copy link
Member

alex commented Jun 21, 2024

We're fine to squash ourselves. Thanks.

If you want a side-quest, the next milestone would be to get mypy running over our tests directory (see tox.ini) :-)

@jlaine jlaine deleted the type-annotations branch June 24, 2024 12:46
jlaine added a commit to jlaine/pyopenssl that referenced this pull request Jun 24, 2024
@jlaine
Update changelog for PR pyca#1308 and pyca#1310
a9f833c
reaperhulk pushed a commit that referenced this pull request Jun 24, 2024
@jlaine
Update changelog for PR #1308 and #1310 (#1311)
caa1ab3
netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc Jul 21, 2024
@0-wiz-0
py-OpenSSL: update to 24.2.1.
df0df6f 
24.2.1 (2024-07-20)
-------------------

Changes:
^^^^^^^^

- Fixed changelog to remove sphinx specific restructured text strings.


24.2.0 (2024-07-20)
-------------------

Deprecations:
^^^^^^^^^^^^^

- Deprecated ``OpenSSL.crypto.X509Req``, ``OpenSSL.crypto.load_certificate_request``, ``OpenSSL.crypto.dump_certificate_request``. Instead, ``cryptography.x509.CertificateSigningRequest``, ``cryptography.x509.CertificateSigningRequestBuilder``, ``cryptography.x509.load_der_x509_csr``, or ``cryptography.x509.load_pem_x509_csr`` should be used.

Changes:
^^^^^^^^

- Added type hints for the ``SSL`` module.
  `#1308 <https://github.com/pyca/pyopenssl/pull/1308>`_.
- Changed ``OpenSSL.crypto.PKey.from_cryptography_key`` to accept public and private EC, ED25519, ED448 keys.
  `#1310 <https://github.com/pyca/pyopenssl/pull/1310>`_.
devonh pushed a commit to element-hq/synapse that referenced this pull request Jul 22, 2024
@dependabot
Bump pyopenssl from 24.1.0 to 24.2.1 (#17465)
e3a0681 
Bumps [pyopenssl](https://github.com/pyca/pyopenssl) from 24.1.0 to
24.2.1.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pyca/pyopenssl/blob/main/CHANGELOG.rst">pyopenssl's
changelog</a>.</em></p>
<blockquote>
<h2>24.2.1 (2024-07-20)</h2>
<p>Backward-incompatible changes:
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^</p>
<p>Deprecations:
^^^^^^^^^^^^^</p>
<p>Changes:
^^^^^^^^</p>
<ul>
<li>Fixed changelog to remove sphinx specific restructured text
strings.</li>
</ul>
<h2>24.2.0 (2024-07-20)</h2>
<p>Backward-incompatible changes:
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^</p>
<p>Deprecations:
^^^^^^^^^^^^^</p>
<ul>
<li>Deprecated <code>OpenSSL.crypto.X509Req</code>,
<code>OpenSSL.crypto.load_certificate_request</code>,
<code>OpenSSL.crypto.dump_certificate_request</code>. Instead,
<code>cryptography.x509.CertificateSigningRequest</code>,
<code>cryptography.x509.CertificateSigningRequestBuilder</code>,
<code>cryptography.x509.load_der_x509_csr</code>, or
<code>cryptography.x509.load_pem_x509_csr</code> should be used.</li>
</ul>
<p>Changes:
^^^^^^^^</p>
<ul>
<li>Added type hints for the <code>SSL</code> module.
<code>[#1308](pyca/pyopenssl#1308)
&lt;https://github.com/pyca/pyopenssl/pull/1308&gt;</code>_.</li>
<li>Changed <code>OpenSSL.crypto.PKey.from_cryptography_key</code> to
accept public and private EC, ED25519, ED448 keys.
<code>[#1310](pyca/pyopenssl#1310)
&lt;https://github.com/pyca/pyopenssl/pull/1310&gt;</code>_.</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pyca/pyopenssl/commit/8dd94578656d47f854c087783b72431aaeb73d8e"><code>8dd9457</code></a>
24.2.1 (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1320">#1320</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/19f093e0c3db5d7354df0a5256344d9ebc5d931f"><code>19f093e</code></a>
make changelog vanilla rst (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1319">#1319</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/e265b2867bbc4f45a14b31affe4a0be710d2b293"><code>e265b28</code></a>
Prepare for 24.2.0 release (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1318">#1318</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/6943ee524e511138dd78fde6000e6f8e2bdd4b0c"><code>6943ee5</code></a>
Deprecate CSR support in pyOpenSSL (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1316">#1316</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/01b9b5637318bde2490e3f8bb243723546dbcc62"><code>01b9b56</code></a>
Add more type definitions for <code>SSL</code> module, check with mypy
(<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1313">#1313</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/cdcb48baf7d5dd0f040f244934c2967b04b9f4bc"><code>cdcb48b</code></a>
Prune redundant <code>:rtype:</code> from SSL module (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1315">#1315</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/b86914d37fc6c0e138e2a0f733dd65dec2efc96c"><code>b86914d</code></a>
Fix <code>ruff</code> invocation (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1314">#1314</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/caa1ab3ac5c5096b20b69fac2e42cb67c6f716d1"><code>caa1ab3</code></a>
Update changelog for PR <a
href="https://redirect.github.com/pyca/pyopenssl/issues/1308">#1308</a>
and <a
href="https://redirect.github.com/pyca/pyopenssl/issues/1310">#1310</a>
(<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1311">#1311</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/9a2105501f601798bb2fb1928cee5ed8b21e32e4"><code>9a21055</code></a>
Allow loading EC, ED25519, ED448 public keys from cryptography (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1310">#1310</a>)</li>
<li><a
href="https://github.com/pyca/pyopenssl/commit/9eaa1073621dd46b3e721183efc318c95aba0d74"><code>9eaa107</code></a>
Add type annotations for the <code>SSL</code> module (<a
href="https://redirect.github.com/pyca/pyopenssl/issues/1308">#1308</a>)</li>
<li>Additional commits viewable in <a
href="https://github.com/pyca/pyopenssl/compare/24.1.0...24.2.1">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pyopenssl&package-manager=pip&previous-version=24.1.0&new-version=24.2.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhils mhils mhils left review comments

@alex alex alex approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jlaine @alex @reaperhulk @mhils