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

Add configuration to use a different CA cert bundle or disable verification #101

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

Conversation

AlekseyIvanov199
Copy link

No description provided.

appended expalanation verify parm

Signed-off-by: AlekseyIvanov199 <[email protected]>
Signed-off-by: AlekseyIvanov199 <[email protected]>
Signed-off-by: AlekseyIvanov199 <[email protected]>
Signed-off-by: AlekseyIvanov199 <[email protected]>
@AlekseyIvanov199 AlekseyIvanov199 requested a review from a team as a code owner August 10, 2023 06:29
@AlekseyIvanov199
Copy link
Author

AlekseyIvanov199 commented Aug 11, 2023

Applyed this commit you can get ability set off SSL verify for botocore (SSL will still be used (unless use_ssl is False)), or you can specify ca.pem file in config if you want to use a different CA cert bundle than the one used by botocore

@AlekseyIvanov199 AlekseyIvanov199 changed the title appended ability set verify param for botocore appended ability set verify param for botocore. you can specify ca.pem file in config if you want to use a different CA cert bundle than the one used by botocore Aug 11, 2023
@clokep clokep changed the title appended ability set verify param for botocore. you can specify ca.pem file in config if you want to use a different CA cert bundle than the one used by botocore Add configuration to use a different CA cert bundle or disable verification Aug 24, 2023
Comment on lines +83 to +84
if "verify" in config:
self.api_kwargs["verify"]=config["verify"]
Copy link
Member

Choose a reason for hiding this comment

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

For reference, this is the set of allowed properties passed to the client.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if "verify" in config:
self.api_kwargs["verify"]=config["verify"]
if "verify" in config:
self.api_kwargs["verify"] = config["verify"]

@@ -179,6 +182,9 @@ def parse_config(config):
if "secret_access_key" in config:
result["secret_access_key"] = config["secret_access_key"]

if "verify" in config:
result["verify"]=config["verify"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result["verify"]=config["verify"]
result["verify"] = config["verify"]

Comment on lines +23 to +26
verify: <boolean/string>
#Whether or not to verify SSL certificates. By default SSL certificates are verified. You can provide the following values:
#False - do not validate SSL certificates. SSL will still be used (unless use_ssl is False), but SSL certificates will not be verified.
#path/to/cert/bundle.pem - A filename of the CA cert bundle to uses. You can specify this argument if you want to use a different CA cert bundle than the one used by botocore.
Copy link
Member

@clokep clokep Aug 24, 2023

Choose a reason for hiding this comment

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

Please move this below secret_access_key.

I do wonder if this should just point back to the AWS docs or something?

@col-panic
Copy link

Waiting for this, just seeing the following message in the logs

synapse-1  | 2024-09-19T13:03:08.355090777Z botocore.exceptions.SSLError: SSL validation failed for https://192.168.54.3:9000/elexis-environment/synapse/remote_content/medelexis.ch/tu/bx/YPJNVttkDNDCRDbkwyJb [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate (_ssl.c:1006)

@col-panic
Copy link

@clokep your proposed changes seem rather straightforward, if @AlekseyIvanov199 can not find the time to integrate those, maybe you could directly patch them? It would be great to have this code included! Thank you guys!

@col-panic
Copy link

bump - please integrate this. I tested it on my site, and it works as expected!

@col-panic
Copy link

bump - please consider integrating this. @AlekseyIvanov199 do you mind integrating @clokep s changes?
@clokep if there is no feedback from @AlekseyIvanov199 , are you willing to ingerate it if I do a new fork and branch with your required changes? Thank you both!

@col-panic
Copy link

@vlaborie sorry to cold call you into this, but it seems that @clokep is dormant and you are actively commiting - would you mind integrating this or give a statement of non-acceptance? The problem is, that I always have to manually patch this after updates happend, and it would be much more convenient to have it integrated!

@vlaborie
Copy link
Contributor

@vlaborie sorry to cold call you into this, but it seems that @clokep is dormant and you are actively commiting - would you mind integrating this or give a statement of non-acceptance? The problem is, that I always have to manually patch this after updates happend, and it would be much more convenient to have it integrated!

@col-panic i have only sent one PR, i have no maintainer nor review privileges on this repository.

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.

4 participants