◐ Shell
reader mode source ↗
Skip to content

gh-118658: Return consistent types from get_un/verified_chain in SSLObject and SSLSocket#118669

Merged
Yhg1s merged 27 commits into
python:mainfrom
matiuszka:main
Aug 16, 2024
Merged

gh-118658: Return consistent types from get_un/verified_chain in SSLObject and SSLSocket#118669
Yhg1s merged 27 commits into
python:mainfrom
matiuszka:main

Conversation

@matiuszka

@matiuszka matiuszka commented May 6, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Fixses inconsistent return types between SSLObject and SSLSocket as described in gh-118656

@bedevere-app bedevere-app Bot mentioned this pull request May 6, 2024
Closed
@matiuszka

matiuszka commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

I restored my old branch from which I created the original PR #109113. There is one file missing about NEWS, I am not sure if that was removed on purpose or not.

sethmlarson
sethmlarson reviewed May 22, 2024
View reviewed changes

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Seems good to me, since these methods are now documented would it make sense to have a test case?

@matiuszka

matiuszka commented May 22, 2024

Copy link
Copy Markdown
Contributor Author

Seems good to me, since these methods are now documented would it make sense to have a test case?

Why not, it will not hurt. I will add it.

@sethmlarson sethmlarson mentioned this pull request Jun 17, 2024
Merged
@felixfontein felixfontein mentioned this pull request Jul 9, 2024
Merged
@felixfontein

felixfontein commented Jul 9, 2024

Copy link
Copy Markdown
Contributor

@matiuszka do you plan to add the test cases soon? I would love to see this PR merged.

@matiuszka

matiuszka commented Jul 10, 2024

Copy link
Copy Markdown
Contributor Author

@matiuszka do you plan to add the test cases soon? I would love to see this PR merged.

Sorry, I forgot about this issue. I will be available next week, so probably next Monday I will have it ready.

@matiuszka

matiuszka commented Jul 17, 2024

Copy link
Copy Markdown
Contributor Author

@sethmlarson, @felixfontein I tried to write tests for it but I am not sure how to start. There are no tests for the SSL module, so there is nothing that I can use to start.

I would need a boilerplate code to see how to execute such a test.

16 hidden items Load more…
@matiuszka

matiuszka commented Aug 14, 2024

Copy link
Copy Markdown
Contributor Author

@felixfontein I applied your remarks.

felixfontein
felixfontein approved these changes Aug 14, 2024
View reviewed changes

@felixfontein felixfontein left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

From my POV this is ready to go. (I have no idea who actually has to approve/merge this though, and how to get it backported to 3.13...)

sethmlarson
sethmlarson approved these changes Aug 14, 2024
View reviewed changes

@sethmlarson sethmlarson left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Thanks for this, LGTM. I'm not a core developer so can't merge this myself, unfortunately.

@sethmlarson

sethmlarson commented Aug 14, 2024

Copy link
Copy Markdown
Contributor

As far as backporting to 3.13, I'll tag in @Yhg1s. The gist is that this API solidified, but didn't get applied to both places where peer certificates can be fetched. Not backporting this will be confusing to other Python implementations which will start implementing the API now that it's public in 3.13 and potentially leave the APIs in a strange place wrt backwards compatibility.

There are only two projects which used these APIs that I'm aware of prior to their public availability and they both should be robust to a backport of this API.

@felixfontein

felixfontein commented Aug 14, 2024

Copy link
Copy Markdown
Contributor

I assume you mean https://github.com/sethmlarson/truststore/pull/137/files#diff-64ad03ae2eedc4414b98845398d17597aa584eccc854cabd4efe9f9445e6a9faR285 and https://github.com/ansible-collections/community.crypto/pull/784/files#diff-635a318cc493470aa9c8734ff6623a4ff998bf00cf457197469268f21bea52faR524 ?

@Yhg1s Yhg1s added the needs backport to 3.13 bugs and security fixes label Aug 16, 2024
@Yhg1s Yhg1s merged commit 8ef358d into python:main Aug 16, 2024
@miss-islington-app

miss-islington-app Bot commented Aug 16, 2024

Copy link
Copy Markdown

Thanks @matiuszka for the PR, and @Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 16, 2024
@matiuszka @gpshead @miss-islington
pythongh-118658: Return consistent types from get_un/verified_chain… …
28d5aa4 
… in `SSLObject` and `SSLSocket` (pythonGH-118669)

(cherry picked from commit 8ef358d)

Co-authored-by: Mateusz Nowak <nowak.mateusz@hotmail.com>
Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
@bedevere-app

bedevere-app Bot commented Aug 16, 2024

Copy link
Copy Markdown

GH-123082 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Aug 16, 2024
sethmlarson pushed a commit that referenced this pull request Aug 19, 2024
@miss-islington @matiuszka @gpshead
[3.13] gh-118658: Return consistent types from `get_un/verified_chain… …
21399a0 
…` in `SSLObject` and `SSLSocket` (GH-118669) (#123082)

gh-118658: Return consistent types from `get_un/verified_chain` in `SSLObject` and `SSLSocket` (GH-118669)
(cherry picked from commit 8ef358d)

Co-authored-by: Mateusz Nowak <nowak.mateusz@hotmail.com>
Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
jeremyhylton pushed a commit to jeremyhylton/cpython that referenced this pull request Aug 19, 2024
@matiuszka @gpshead @jeremyhylton
pythongh-118658: Return consistent types from get_un/verified_chain… …
9fa2aae 
… in `SSLObject` and `SSLSocket` (python#118669)

Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
@matiuszka @gpshead @blhsing
pythongh-118658: Return consistent types from get_un/verified_chain… …
14d5d53 
… in `SSLObject` and `SSLSocket` (python#118669)

Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
@kanavin

kanavin commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

Sadly, I think this has to be reverted, as it broke certificate generation script, and doesn't contain any other way to generate cert3.pem:
#107594 (comment)

Alternatively, you need to produce a fix to that script that includes correct regeneration of cert3.pem.

kanavin added a commit to kanavin/cpython that referenced this pull request Sep 26, 2024
@kanavin
Revert "pythongh-118658: Return consistent types from `get_un/verifie… …
2a2d1dc 
…d_chain` in `SSLObject` and `SSLSocket` (python#118669)"

This reverts commit 8ef358d.
@felixfontein

felixfontein commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

@kanavin how about simply disabling the broken test instead of reverting the whole PR? It would be pretty sad if the interface would be re-broken by reverting this.

@felixfontein

felixfontein commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

I looked at the certs; cert3.pem is simply the last certificate in keycert3.pem, so extracting it from there should be easy.

@felixfontein felixfontein mentioned this pull request Sep 26, 2024
Merged
@felixfontein

felixfontein commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

#124598 should fix Lib/test/certdata/make_ssl_certs.py to extract cert3.pem from keycert3.pem.

@felixfontein

felixfontein commented Sep 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

#124599 is simpler and modifies cert3.pem (by adding the comment that's part of keycert3.pem). It shouldn't affect the test. Unfortunately it doesn't work since the comment breaks PEM_cert_to_DER_cert...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yhg1s Yhg1s Yhg1s approved these changes

@sethmlarson sethmlarson sethmlarson approved these changes

+1 more reviewer

@felixfontein felixfontein felixfontein approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@matiuszka @felixfontein @sethmlarson @kanavin @Yhg1s @gpshead