Added SMTP SSL/TLS support by sn0oz · Pull Request #7960 · home-assistant/core

sn0oz · 2017-06-08T15:40:24Z

Description:

Add SMTP SSL/TLS support from smtplib.py's class smtplib.SMTP_SSL
configurable option like 'starttls', default to False

I ran tox and had errors, not sure how to publish results, please help.

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2843

Example entry for `configuration.yaml` (if applicable):

notify:
  - name: mail
    platform: smtp
    server: !secret smtp_server
    port: 465
    sender: !secret mail_alert
    username: !secret mail_username
    password: !secret mail_password
    recipient: !secret mail_admin
    starttls: false
    ssltls: true
    debug: true

automation:
  trigger:
    platform: homeassistant
    event: start
  action:
    - service: notify.mail
      data:
        title: "HASSdev started"

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

homeassistant · 2017-06-08T15:40:26Z

Hi @sn0oz,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-06-08T15:40:30Z

+        if self.ssltls:
+            mail = smtplib.SMTP_SSL(self._server, self._port, timeout=self._timeout)
+        else:
+            mail = smtplib.SMTP(self._server, self._port, timeout=self._timeout)


line too long (80 > 79 characters)

houndci-bot · 2017-06-08T15:40:30Z

        """Connect/authenticate to SMTP Server."""
-        mail = smtplib.SMTP(self._server, self._port, timeout=self._timeout)
+        if self.ssltls:
+            mail = smtplib.SMTP_SSL(self._server, self._port, timeout=self._timeout)


line too long (84 > 79 characters)

houndci-bot · 2017-06-08T15:40:30Z

    """Implement the notification service for E-mail messages."""

-    def __init__(self, server, port, timeout, sender, starttls, username,
+    def __init__(self, server, port, timeout, sender, starttls, ssltls, username,


line too long (81 > 79 characters)

amelchio · 2017-06-08T18:57:45Z

I think this is confusing:

    starttls: false
    ssltls: true

I wonder if a single security config with these options could work instead:

auto (default)
starttls
ssl
none (not by default if a password is used)

(I know it's much more work to make it like that :-)

sn0oz · 2017-06-09T09:43:29Z

Totally agree, this is just a quick fix. It should not be possible to set the starttls and ssltls options to true simultaneously.
Could you specify how the "auto" option would work ?

amelchio · 2017-06-09T12:29:14Z

I was thinking "auto" could work like this:

if port is 465: pick "ssl"
otherwise, if server supports SMTP STARTTLS: pick "starttls"
otherwise, if password is specified: report failure
otherwise, pick "none"

If that is too complicated, the first two should work for many users and the rest would have to specify "none" to confirm that they want unencrypted communication.

sn0oz · 2017-06-09T15:04:12Z

In my understanding, the encryption type is not related to password authentication.
You can authenticate by password on a non-encrypted SMTP session.
The encryption protocol may use any port depending on the server, SSL/TLS on port 587 is possible.

So the security option could be any of:

ssltls (default for enhanced security)
starttls
none

And port would be optional, default depending on the security option.

amelchio · 2017-06-09T21:00:32Z

You can absolutely send your password in cleartext but it's a bad idea and I don't think we should encourage it (even if it is the current default).

sn0oz · 2017-06-12T07:34:59Z

We could set the default security to ssltls and add a warning on the documentation about sending password on a non encrypted session.
This would break existing configurations, is everybody ok with this ?

amelchio · 2017-06-12T21:17:24Z

I don't think that's a good default, port 465 has been deprecated for 20 years. The standard these days is port 587 with STARTTLS.

sn0oz · 2017-06-13T08:50:00Z

It seems STARTTLS is less secure than SSL/TLS, as it is subject to man-in-the-middle attacks, and depending on the client if encryption negotiation fails it may fallback to non-encrypted session. https://en.wikipedia.org/wiki/Opportunistic_TLS
If I understand correctly port 465 as been deprecated for SMTP over SSL/TLS but is still in use on most mail servers (Gmail, etc...). More on this: https://www.fastmail.com/help/technical/ssltlsstarttls.html

sn0oz · 2017-06-16T13:05:10Z

Commited new proposal with "encryption" option, defaults to ssltls, and port defaults to 465.
Test configuration:

automation:
  trigger:
    platform: homeassistant
    event: start
  action:
    - service: notify.mail
      data:
        title: "HASSdev started"
        message: "yep"

notify:
  - name: mail
    platform: smtp
    server: !secret smtp_server
    sender: !secret mail_alert
    username: !secret mail_username
    password: !secret mail_password
    recipient: !secret mail_admin
    encryption: ssltls
    debug: true

houndci-bot · 2017-06-16T13:28:57Z

+        if self.encryption == "ssltls":
+            mail = smtplib.SMTP_SSL(self._server, self._port, timeout=self._timeout)
+        elif self.encryption == "none" or self.encryption == "starttls":
+            mail = smtplib.SMTP(self._server, self._port, timeout=self._timeout)


line too long (80 > 79 characters)

houndci-bot · 2017-06-16T13:28:57Z

        """Connect/authenticate to SMTP Server."""
-        mail = smtplib.SMTP(self._server, self._port, timeout=self._timeout)
+        if self.encryption == "ssltls":
+            mail = smtplib.SMTP_SSL(self._server, self._port, timeout=self._timeout)


line too long (84 > 79 characters)

fabaff · 2017-06-17T10:56:12Z

    vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
    vol.Optional(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,
-    vol.Optional(CONF_STARTTLS, default=DEFAULT_STARTTLS): cv.boolean,
+    vol.Optional(CONF_ENCRYPTION, default=DEFAULT_ENCRYPTION): cv.string,


We should limit the options to valid entries. See our validation docs.

fabaff · 2017-06-17T10:58:37Z

-        mail = smtplib.SMTP(self._server, self._port, timeout=self._timeout)
+        if self.encryption == "ssltls":
+            mail = smtplib.SMTP_SSL(self._server, self._port, timeout=self._timeout)
+        elif self.encryption == "none" or self.encryption == "starttls":


This could be an else as you are covering the the two other options.

houndci-bot · 2017-06-18T09:20:48Z

    vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
    vol.Optional(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,
-    vol.Optional(CONF_STARTTLS, default=DEFAULT_STARTTLS): cv.boolean,
+    vol.Optional(CONF_ENCRYPTION, default=DEFAULT_ENCRYPTION): vol.In(['ssltls','starttls','none']),


line too long (100 > 79 characters)
missing whitespace after ','

sn0oz · 2017-06-18T09:23:50Z

Thanks. Now the component fails to load with a nice error message:

2017-06-18 11:17:08 ERROR (MainThread) [homeassistant.config] Invalid config for [notify.smtp]: value is not allowed for dictionary value @ data['encryption']. Got 'wrong'. (See ?, line ?). Please check the docs at https://home-assistant.io/components/notify.smtp/

amelchio · 2017-06-19T12:24:16Z

STARTTLS is not less secure than the deprecated SSL wrapping. True, it can be "depending on the client" but in this case the client is HA, so we could choose not to be vulnerable.

* Added SMTP SSL/TLS support * added new encryption option * validation of encryption option * Fix lint issues * Rename var

Added SMTP SSL/TLS support

eec60e8

homeassistant added platform: notify.smtp cla-needed labels Jun 8, 2017

houndci-bot reviewed Jun 8, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Jun 8, 2017

andreasjacobsen93 approved these changes Jun 8, 2017

View reviewed changes

added new encryption option

3a898b8

homeassistant added the cla-signed label Jun 16, 2017

balloobbot added the platform label Jun 16, 2017

houndci-bot reviewed Jun 16, 2017

View reviewed changes

fabaff requested changes Jun 17, 2017

View reviewed changes

validation of encryption option

9c5fb73

homeassistant added the cla-signed label Jun 18, 2017

houndci-bot reviewed Jun 18, 2017

View reviewed changes

fabaff added the breaking-change label Jun 19, 2017

Fix lint issues

98ef096

homeassistant added the cla-signed label Jun 19, 2017

Rename var

0714c4a

fabaff mentioned this pull request Jun 19, 2017

Add new conf options home-assistant/home-assistant.io#2843

Merged

fabaff approved these changes Jun 19, 2017

View reviewed changes

fabaff merged commit 8e34c27 into home-assistant:dev Jun 19, 2017

balloob mentioned this pull request Jul 1, 2017

0.48 #8270

Merged

sn0oz deleted the smtp-ssltls branch July 8, 2017 08:40

dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

Added SMTP SSL/TLS support (home-assistant#7960)

2a653f8

* Added SMTP SSL/TLS support * added new encryption option * validation of encryption option * Fix lint issues * Rename var

home-assistant locked and limited conversation to collaborators Oct 20, 2017

ghost added integration: smtp and removed platform: notify.smtp labels Mar 21, 2019

Uh oh!

Conversation

sn0oz commented Jun 8, 2017 • edited by fabaff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

homeassistant commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio commented Jun 8, 2017

Uh oh!

sn0oz commented Jun 9, 2017

Uh oh!

amelchio commented Jun 9, 2017

Uh oh!

sn0oz commented Jun 9, 2017

Uh oh!

amelchio commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sn0oz commented Jun 12, 2017

Uh oh!

amelchio commented Jun 12, 2017

Uh oh!

sn0oz commented Jun 13, 2017

Uh oh!

sn0oz commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sn0oz commented Jun 18, 2017

Uh oh!

amelchio commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sn0oz commented Jun 8, 2017 •

edited by fabaff

Loading

Example entry for `configuration.yaml` (if applicable):

amelchio commented Jun 9, 2017 •

edited

Loading