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

Allow a user to disable peer check when using TLS/STARTTLS #4126

Merged

Conversation

vincentbernat
Copy link
Contributor

This is useful when developing and on Docker setups. Despite setting encryption to null, if a server supports STARTTLS with a self-signed certificate, the mailer try to upgrade the connection with STARTTLS.

Fix #3166

This is useful when developing and on Docker setups. Despite setting
encryption to null, if a server supports STARTTLS with a self-signed
certificate, the mailer try to upgrade the connection with STARTTLS.
@ssddanbrown
Copy link
Member

Thanks for offering this @vincentbernat,

Could you confirm this has change has been tested in a real actual use here, where the environment would error without this change? I just want to be sure we're adding what's needed here (Since there are many options in PHP relating to possible mail/TLS connection verification) and that this is not being added without being sure to cover the scenario.

I'll also need to test the scenario myself so just want to confirm this has been tested before I spend a lot of time to replicate a similar scenario.

Additionally, it would be ideal if we could have some level of testing in place within the codebase, if possible, to ensure the mailer at least receives these options, so that our testing would alert if the config API changes like it did for Laravel 9.

@vincentbernat
Copy link
Contributor Author

I have tested it myself. Our internal mail server exposes STARTTLS and use a self-signed certificate. So, it's working. I am using the Docker setup (so I suppose with Laravel 9).

@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Apr 6, 2023
@ssddanbrown ssddanbrown merged commit f2293a7 into BookStackApp:development Apr 23, 2023
@ssddanbrown
Copy link
Member

Thanks again @vincentbernat, now merged.

I followed this up with 1de72d0, just to tweak the name to be a bit more generic so we can bundle additional ssl/tls-verification-disabling options if required in future, and to add some testing to ensure the option affects the underlying mailer.

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

Successfully merging this pull request may close these issues.

[Feature Request]: Allow disable SMTP certificate verification
2 participants