Make EmailAlert docs and code consistent #98

susam · 2019-04-22T04:18:04Z

Make EmailAlert docs and code consistent

The EmailAlert docstrings have been updated to make it look good from a
user's perspective. In some places, the docstrings have been fixed to
describe the functionality in the code correctly. In some other cases,
the code has been updated to be consistent with the updated docstrings.

The following changes have been made to the plugin parameters:

Rename sender to from_addr.
Rename to to to_addrs.
Remove body.
Add defaults for subject, host, and port.

The names from_addr and to_addrs have been picked from the Python
smtplib documentation. They have done the work of choosing good names,
so we just reuse it. The to_addrs name has an additional advantage
that it hints at the value being a list of addresses.

The body parameter has been removed because this is not a generic
emailer class. This is an alert plugin which will decide its own email
content based on the events it receives in its write() method.
There is never a situation where the email content can be specified via
configuration.

The defaults for host and port have also been picked from the Python
smtplib documentation. They implement sensible default behaviour for
these defaults, so we reuse that behaviour by choosing the same
defaults.

s-nirali

The commit message misses one backtick in the line

"Add defaults for subject, host, and port`."

baseconfig.py file params for emailalert need to be updated.
This may be ignored. I missed Remove EmailAlert configuration from base config #99.
Should the buffer be checked for an upper limit like 25 Mb by default for size?

s-nirali · 2019-04-22T14:07:30Z

cloudmarker/alerts/emailalert.py

+        :class:`smtplib.SMTP_SSL` documentation for more details on
+        this.
+
+        When ``use_ssl`` is ``False` and if ``host`` or ``port`` are not


Missing backtick

s-nirali · 2019-04-22T14:07:46Z

cloudmarker/alerts/emailalert.py

                 username=None, password=None):
        """Create an instance of :class:`EmailAlert` plugin.

+        When ``use_ssl` is ``True`` and ``host`` is not specified or


Missing backtick

s-nirali · 2019-04-22T14:29:04Z

cloudmarker/alerts/emailalert.py

-            smtp.sendmail(self._sender, self._to, message.as_string())
+            smtp.sendmail(self._from_addr,
+                          self._to_addrs,
+                          message.as_string())


Most of the other plugins have a success message logged which I personally find very convenient to verify if the method actually succeeded. The other alert, however, does not have it as well. You may choose to add it if you feel it is required.

I agree. I was trying to keep this change minimal, i.e., just focus on the documentation and necessary code updates. This plugin requires quite a bit of rework to move the SMTP code out to util module and make it consistent with the other plugins. I will do some of it as part of #53. I will address this suggestion of yours while resolving #53.

s-nirali · 2019-04-22T14:43:09Z

cloudmarker/alerts/emailalert.py

+
+        Returns:
+            smtplib.SMTP or smtplib.SMTP_SSL: An SMTP connection
+                with its session ready to send messages.

        """
        if self._use_ssl:


In lines 99 and 102, enabling the debug level by default leads to a lot of email body data in the logs. Should this flag be moved to the config as an internal debug parameter?

At line 98, with the following config, I ran into an ssl.SSLError.

Config:

alerts: emailalert: params: use_ssl: True host: smtp.gmail.com port: 587 from_addr: CloudMarker Alert <no-reply@gmail.com> to_addrs: - <recipient@example.com> subject: Anomaly notification username: <alice> password: <alice@123>

Exception stack trace:

Traceback (most recent call last): File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap self.run() File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/multiprocessing/process.py", line 99, in run self._target(*self._args, **self._kwargs) File "/Users/git/cloudmarker/cloudmarker/workers.py", line 68, in store_worker store_plugin.done() File "/Users/git/cloudmarker/cloudmarker/alerts/emailalert.py", line 79, in done smtp = self._prepare_smtp_session() File "/Users/git/cloudmarker/cloudmarker/alerts/emailalert.py", line 98, in _prepare_smtp_session smtp = smtplib.SMTP_SSL(host=self._host, port=self._port) File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/smtplib.py", line 1031, in __init__ source_address) File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/smtplib.py", line 251, in __init__ (code, msg) = self.connect(host, port) File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/smtplib.py", line 336, in connect self.sock = self._get_socket(host, port, self.timeout) File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/smtplib.py", line 1039, in _get_socket server_hostname=self._host) File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ssl.py", line 412, in wrap_socket session=session File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ssl.py", line 853, in _create self.do_handshake() File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ssl.py", line 1117, in do_handshake self._sslobj.do_handshake() ssl.SSLError: [SSL: UNKNOWN_PROTOCOL] unknown protocol (_ssl.c:1056)

If the use_ssl key is set to False, the mail is successfully sent. This could probably be handled in another PR. There is a solution mentioned here.

In lines 99 and 102, enabling the debug level by default leads to a lot of email body data in the logs. Should this flag be moved to the config as an internal debug parameter?

I agree. This should be config driven. I will do this as part of #53.

At line 98, with the following config, I ran into an ssl.SSLError.

Config:

alerts: emailalert: params: use_ssl: True host: smtp.gmail.com port: 587

For use_ssl set to True, set port to either 465 or 0 (to choose default port which is 465).

Also, see the docstring in line 26 for more details on this.

The docstring does not say that use_ssl can only be used for port 465. I'm not sure if someone running into this error would be able to figure out that both the port and use_ssl work together. This is not a breaking change so I'm fine if we don't do error handling here.

I think I ended up testing this the wrong way because of the existing config in base config. Since that config is not meant to exist, this comment may be ignored 😅 .

I tried to address this by adding more details in the util.send_email() docstring in PR #108. See the following two URLs on the Read the Docs:

cloudmarker.alerts.emailalert.EmailAlert()

cloudmarker.util.send_email()

Please let me know if this helps of if we need to rework on these docstrings.

s-nirali · 2019-04-22T14:44:32Z

cloudmarker/alerts/emailalert.py

-        if self._stringbuffer:
-            self._body = ''.join(self._stringbuffer)
-        message.attach(MIMEText(self._body))
+        body = ''.join(self._stringbuffer)


Is it okay to send raw json data in the email body?

It is not okay to send raw JSON in the email body. I would like record['com']['description'] and record['com']['recommendation'] to be picked out from the event record (record) and formatted neatly in the email alerts. This will be done in a separate pull request.

susam · 2019-04-22T16:31:29Z

2. Should the buffer be checked for an upper limit like 25 Mb by default for size?

Hi Nirali,

We can do something like this. I would prefer to do this in a separate pull request. By the way, did you come up with the 25 MB limit as an arbitrary limit or did you use some reference to come up with this limit?

s-nirali · 2019-04-22T16:36:55Z

Hi Nirali,

We can do something like this. I would prefer to do this in a separate pull request. By the way, did you come up with the 25 MB limit as an arbitrary limit or did you use some reference to come up with this limit?

I used this, https://www.outlook-apps.com/maximum-email-size/.

sunnysharmagts

Changes looks good to me.

The EmailAlert docstrings have been updated to make it look good from a user's perspective. In some places, the docstrings have been fixed to describe the functionality in the code correctly. In some other cases, the code has been updated to be consistent with the updated docstrings. The following changes have been made to the plugin parameters: - Rename `sender` to `from_addr`. - Rename `to` to `to_addrs`. - Remove `body`. - Add defaults for `subject`, `host`, and `port`. The names `from_addr` and `to_addrs` have been picked from the Python `smtplib` documentation. They have done the work of choosing good names, so we just reuse it. The `to_addrs` name has an additional advantage that it hints at the value being a list of addresses. The `body` parameter has been removed because this is not a generic emailer class. This is an alert plugin which will decide its own email content based on the events it receives in its `write()` method. There is never a situation where the email content can be specified via configuration. The defaults for `host` and `port` have also been picked from the Python `smtplib` documentation. They implement sensible default behaviour for these defaults, so we reuse that behaviour by choosing the same defaults.

susam requested review from sunnysharmagts, rosehgal and s-nirali April 22, 2019 04:18

susam added the documentation Fixes and improvements in documentation label Apr 22, 2019

susam force-pushed the emaildocs branch from defc35d to ba56330 Compare April 22, 2019 12:08

s-nirali requested changes Apr 22, 2019

View reviewed changes

susam force-pushed the emaildocs branch 2 times, most recently from 7eeb81d to b019735 Compare April 22, 2019 17:35

susam changed the title ~~Polish EmailAlert docs; update code to agree~~ Make EmailAlert docs and code consistent Apr 22, 2019

This was referenced Apr 23, 2019

Check email payload size limit in EmailAlert #103

Open

Log a success message in EmailAlert on successful email alert #104

Closed

Enable SMTP debuglevel via configuration #105

Closed

EmailAlert payload should be human readable #106

Open

s-nirali approved these changes Apr 23, 2019

View reviewed changes

sunnysharmagts approved these changes Apr 23, 2019

View reviewed changes

susam force-pushed the emaildocs branch from b019735 to 30da5c2 Compare April 23, 2019 16:14

susam merged commit 30da5c2 into master Apr 23, 2019

susam deleted the emaildocs branch April 23, 2019 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make EmailAlert docs and code consistent #98

Make EmailAlert docs and code consistent #98

susam commented Apr 22, 2019 •

edited

s-nirali left a comment •

edited

s-nirali Apr 22, 2019

susam Apr 22, 2019

s-nirali Apr 22, 2019

susam Apr 22, 2019

s-nirali Apr 22, 2019

susam Apr 22, 2019

s-nirali Apr 22, 2019

s-nirali Apr 22, 2019

susam Apr 22, 2019

susam Apr 22, 2019

s-nirali Apr 22, 2019 •

edited

susam May 6, 2019

s-nirali Apr 22, 2019

susam Apr 22, 2019

susam commented Apr 22, 2019

s-nirali commented Apr 22, 2019 •

edited

sunnysharmagts left a comment

Make EmailAlert docs and code consistent #98

Make EmailAlert docs and code consistent #98

Conversation

susam commented Apr 22, 2019 • edited

s-nirali left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-nirali Apr 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susam commented Apr 22, 2019

s-nirali commented Apr 22, 2019 • edited

sunnysharmagts left a comment

Choose a reason for hiding this comment

susam commented Apr 22, 2019 •

edited

s-nirali left a comment •

edited

s-nirali Apr 22, 2019 •

edited

s-nirali commented Apr 22, 2019 •

edited