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

Fetch IDP metadata using requests to support custom server certificates root CAs #415

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fetch IDP metadata using requests to support custom server certificates root CAs #415

wants to merge 1 commit into from
+22 −16

Conversation

sergei-maertens
Copy link

Closes #403

Using requests allows us to easily customize the CA_BUNDLE to use when verifying the server certificate, instead of having to disable SSL certificate verification alltogether.

@sergei-maertens
Copy link
Author

@pitbulk I didn't realize that requests is not a dependency yet, but IMO this is the easiest way to support this. Happy to discuss alternatives though, if you feel that adding the library is too much.

@pitbulk
Copy link
Contributor

pitbulk commented Jul 10, 2024 •
edited
Loading

@sergei-maertens if I'm not wrong, urllib.request.urlopen accepts cafile and capath parameters, now that we are forcing in the new release to use python3 > 3.7, you can adapt your PR to keep using urllib.request.urlopen and accept in the method those new parameters.

Test it and let me know if works as well as requests.

@sergei-maertens
Copy link
Author

sergei-maertens commented Jul 11, 2024 •
edited
Loading

I'll look into it asap, thanks for the feedback!

edit: what would your approach be to provide the cafile/capath parameters. An envvar, or something in the settings dict? I'm leaning towards the former. not relevant, doesn't seem like this code is called interally in many places

@pitbulk
Copy link
Contributor

pitbulk commented Oct 1, 2024

@sergei-maertens do you plan to rework on the PR?

@sergei-maertens
Copy link
Author

I had completely forgotten about it, sorry! I can still pick it up

@sergei-maertens sergei-maertens force-pushed the issue/403-support-self-signed-certificates branch from 78033d9 to f3d61ae Compare October 3, 2024 07:43
@sergei-maertens
✨ [SAML-Toolkits#403] Support cafile and capath parameters
ed63c57 
When retrieving the IDP metadata, you can now optionally specify the the
capath or cafile to use for certificate verification, rather than just
enabling/disabling it.

This allows TLS verification of server certificates that are not in the
system root store (such as when using private CAs).
@sergei-maertens sergei-maertens force-pushed the issue/403-support-self-signed-certificates branch from f3d61ae to ed63c57 Compare October 3, 2024 07:44
@sergei-maertens
Copy link
Author

@pitbulk I've updated the PR with the suggested changes, now only the stdlib is used :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OneLogin_Saml2_IdPMetadataParser.get_metadata not compatible with self-signed certificates
2 participants
@sergei-maertens @pitbulk