Introduced Safer SMTP Alternative: Added `SMTP_SSL`

[fazledyn-or]

Details

While triaging your project, our bug fixing tool generated the following message(s)-

In file: elastalert.py, method: send_notification_email, a clear-text protocol such as FTP, Telnet or SMTP is used. These protocols transfer data without any encryption, which expose applications to a large range of risks. iCR suggested that data should be transferred over only secure transport channels.

Changes

• Added SMTP_SSL option with authentication
• Updated documentation for SMTP_SSL

Checklist

• [x] I have reviewed the contributing guidelines.
• [x] I have included unit tests for my changes or additions.
• [x] I have successfully run make test-docker with my changes.
• [x] I have manually tested all relevant modes of the change in this PR.
• [x] I have updated the documentation.
• [x] I have updated the changelog.

Questions or Comments

• Unit Test: It wasn't needed since SMTP is mocked.
• Relevant modes of change in PR: No idea what it means
• Documentation: I have updated one rst file.
• Changelog: No idea what to add. If you help me, I can add one.

Previously Found & Fixed

• https://www.github.com/google/timesketch/pull/2940
• https://www.github.com/nasa-gibs/onearth/pull/177
• https://www.github.com/geopython/pycsw/pull/917

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information). - Git Commit SignOff documentation

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

[jertel]

Hello and thanks for your interest in ElastAlert2!

There are two new if blocks that will need unit test coverage. While smtp may be mocked already, the use of that mock does not exempt the newly added logic. The unit test should cover the scenarios:

1. when SSL is not used (already covered by test_notify_email())
2. when SSL is used but without credentials
3. when SSL is used with credentials
4. (optionally) SSL is not used, but credentials are used (your code currently doesn't handle this situation).

Regarding the documentation, the new parameters should clarify that username/password both must be supplied in order to use authentication. Also, it needs to clarify that the credentials will only be provided if SSL is enabled, unless you choose to add support for insecure authentication (test scenario four above)

That said, the alerters in ElastAlert2 already support secure email and authentication. Yet they do so by using credentials stored in a separate file. This is done to avoid storing email secrets in a common configuration file that also contains non-secrets. So adding your changes will introduce a bit of confusion to the project since one email path (ElastAlert2 error notifications) will use email credentials stored in the config file/env, while the other email path (Trigger Alerts) will read credentials from a file dedicated to storing SMTP secrets. I suspect the reason no one has added SSL or authentication support to the former path is because it's rarely used. Given all that, I'm not opposed to adding this new logic as-is, since it does add some additional security, even if it doesn't get used very often. But it is something to consider if you were interested in changing the behavior to make it more consistent with the existing secure email path.

You can view the CHANGELOG.md file and observe the pattern we've been using for adding new changelog entries. Other contributors have been successfully able to discern the pattern and add their changes.

[jertel]

Hello, Is there a plan to continue this PR?

[fazledyn-or]

Hello, Is there a plan to continue this PR?

Yes, of course. Sorry for my inactiveness. I've thought of changes to this PR and would soon push them.

[fazledyn-or]

Pushed the latest changes. Also, I tested them without mocking too- it works.

[jertel]

I updated the documentation of the new fields to match your latest changes.

The unit tests should be verifying that the user/pass is provided to the login function, and should validate that the host/port is provided as configured.

[fazledyn-or]

I updated the documentation of the new fields to match your latest changes.

The unit tests should be verifying that the user/pass is provided to the login function, and should validate that the host/port is provided as configured.

Looks like there was a merge conflict. I have resolved it.

[jertel]

Looks like there was a merge conflict. I have resolved it.

Thanks for doing that. The unit tests will need some additional assertions to verify user/pass/host/port are passed as expected. After those assertions are implemented this should be ready to merge.

[jertel]

Closing due to inactivity. Feel free to re-open if the remaining work is ready to push.