Allow unverified SSL and additional HTTP headers#89
Allow unverified SSL and additional HTTP headers#89jlubawy wants to merge 2 commits intokiwitcms:masterfrom
Conversation
atodorov
left a comment
atodorov
left a comment
There was a problem hiding this comment.
Thanks for this contribution. It needs some changes to make it better.
| ssl._create_default_https_context = ssl._create_unverified_context | ||
|
|
||
| if len(extra_headers) > 0: | ||
| self.transport._headers += extra_headers |
There was a problem hiding this comment.
This possibly overrides an existing value set for this attribute.
You can pass headers in Transport.__init__() so better add this in lines 128 & 130.
| url, | ||
| allow_unverified_ssl: bool = False, | ||
| extra_headers: list[tuple[str, str]] = [], | ||
| ): |
There was a problem hiding this comment.
Will have to define these for the inherited class TCMSKerbXmlrpc, and take care to pass them down appropriately otherwise we have broken method signatures in the inheritance chain.
| raise RuntimeError("Unrecognized URL scheme") | ||
|
|
||
| if allow_unverified_ssl: | ||
| ssl._create_default_https_context = ssl._create_unverified_context |
There was a problem hiding this comment.
This is modifying the global state of the ssl module which potentially can lead to side effects depending on how that module and the tcms_api module are used by the developer. This could lead to very difficult to debug issues.
This also affects the SSL module even if for whatever reason the user is connecting to a plain-text endpoint (e.g. reusing the same code base).
Instead of doing it like this, see the context argument of SafeTransport.__init__().
|
Thank you for your review and comments. I'll try to make your suggestions and will let you know if I have any questions. |
2 participants
Hi, thank you for creating this API client and Kiwi TCMS itself.
I've added two new features that I've found useful:
Proxy-Authorizationheader).The changes should be backwards compatible due to setting default values for these new parameters.
Please let me know if there is anything you'd like to change though before you'd consider merging.