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

Support Proxy client certificate #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

geonmo
Copy link

@geonmo geonmo commented Feb 8, 2023

I am applying for PR to deal with issue #13 .

This PR has been changed to support the processing of proxy certificates signed with the user's certificate.

When using a nss-linked libcurl for ssl backend, it check the issuing authority of the proxy certificate and handle it even if verifypeer is applied as False.

Because the proxy certificate is signed with a user certificate, the public key of the user certificate must be placed in the list of trusted certificates. For users who do not have root privileges, creating and applying a certificate bundle file is the easiest way to do this.

Copy link
Contributor

@vkuznet vkuznet left a comment

Choose a reason for hiding this comment

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

I suggest to have review from @amaltaro too.

src/python/RestClient/AuthHandling/X509Auth.py Outdated Show resolved Hide resolved
…dle not to create a new one.

In addition, for temporary bundle files, it was declared as an X509Auth class variable to be automatically deleted at the end of the program.
@geonmo
Copy link
Author

geonmo commented Feb 9, 2023

Is it okay if the Python version is limited to 3.6 or higher? As far as I know, the f-string works from 3.6 or higher.

@amaltaro
Copy link
Contributor

amaltaro commented Feb 9, 2023

@geonmo Hi Geonmo, yes, I'd say that is a fair assumption. Many CMS projects use it with Python 3.8.2.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

Thank you for providing this patch, Geonmo. It looks good to me.

Valentin, to be safer, I still think further tests - with multiple scenarios - should be performed before this change gets merged. I'd suggest at least:

  • on SLC7, test these changes within a test WMAgent box (using robot or service certificate)
  • on SLC7, test these changes with user proxy

@vkuznet vkuznet requested a review from amaltaro March 22, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants