✨♻️ Differentiate between TLS and STARTTLS in web-mailserver (⚠️ devops) #2965
Conversation
mrnicegyu11
added
t:enhancement
Codecov Report
@@ Coverage Diff @@
## master #2965 +/- ##
========================================
+ Coverage 83.5% 84.4% +0.8%
========================================
Files 848 647 -201
Lines 35763 28529 -7234
Branches 748 178 -570
========================================
- Hits 29876 24079 -5797
+ Misses 5697 4392 -1305
+ Partials 190 58 -132
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mrnicegyu11
changed the title
.pre-commit-config.yaml
Outdated
| @@ -22,6 +22,6 @@ repos: | |||
| hooks: | |||
| - id: isort | |||
| - repo: https://github.com/psf/black | |||
| rev: 21.9b0 | |||
| rev: 22.3.0 | |||
There was a problem hiding this comment.
Not sure about bumping this here. @sanderegg @pcrespov ?
There was a problem hiding this comment.
The pre-commit scripts run on their own virtualenv (with is different from our .venv) so bumping the version there is ok. Nonetheless there should be some level of compatibility with the tools used in vscode (which are installed in our .venv).
This number should be at least compatible (if possible identical) with the version we have in the requirements/_tool.txt (see screenshot below)... otherwise it is very confusing to format in one way with vscode tooling and then again with the pre-commit. Same applies for isort.
That said, as you can see we use 22.1.0 so that hsould be the version you put there.
| log.info("Unencrypted connection attempt to mailserver ...") | ||
| await smtp.connect(use_tls=False, port=cfg.SMTP_PORT) | ||
| log.info("Starting STARTTLS ...") | ||
| log.warning("Certificates are not validated for STARTTLS on port 587!") |
There was a problem hiding this comment.
not sure I understand the reason for this warning here
There was a problem hiding this comment.
It is not really perfect to use encrypted communication but trust all certificates, the gain in security is marginal. I just thought the least I can do at this point (didnt wanna open up a whole big pandoras box and follow up on why it is coded like this now), was to add a warning.
But of course this log can be nerfed or removed
There was a problem hiding this comment.
I would suggest something like the following. Please use the actual port from the configuration.
log.warning(
f"Certificates are not validated for STARTTLS (as it happens for TLS) on port {cfg.SMTP_PORT}!"
)
There was a problem hiding this comment.
I am not sure I understand the whole thing here. Is this a warning that no one will check even though we should tackle it? in which case please create an issue.
Is this workaround here because of the library? Is that the way it works? then I would not show any warning.
And if this is a security issue, should we really use starttls?
There was a problem hiding this comment.
It's slightly better than unencrypted. In theory we should get a better mail server, since we run it, which supports TLS and be done with it.
Co-authored-by: Andrei Neagu <5694077+GitHK@users.noreply.github.com>
.pre-commit-config.yaml
Outdated
| @@ -22,6 +22,6 @@ repos: | |||
| hooks: | |||
| - id: isort | |||
| - repo: https://github.com/psf/black | |||
| rev: 21.9b0 | |||
| rev: 22.3.0 | |||
There was a problem hiding this comment.
The pre-commit scripts run on their own virtualenv (with is different from our .venv) so bumping the version there is ok. Nonetheless there should be some level of compatibility with the tools used in vscode (which are installed in our .venv).
This number should be at least compatible (if possible identical) with the version we have in the requirements/_tool.txt (see screenshot below)... otherwise it is very confusing to format in one way with vscode tooling and then again with the pre-commit. Same applies for isort.
That said, as you can see we use 22.1.0 so that hsould be the version you put there.
| @root_validator(pre=True) | ||
| @classmethod | ||
| def not_both_tls_and_starttls(cls, values): | ||
| if values.get("SMTP_TLS_ENABLED") and values.get("SMTP_STARTTLS_ENABLED"): |
There was a problem hiding this comment.
when you have multiple exclusive choices it is better if you can a single field (i.e. env variable) associated with an enum that has all the choice. Can you afford that?
services/web/server/src/simcore_service_webserver/login/utils.py
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed!
|
services/web/server/src/simcore_service_webserver/login/utils.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/application_settings_utils.py
Outdated
Show resolved
Hide resolved
…ARTTLSsupportForMailserver
services/web/server/src/simcore_service_webserver/login/utils.py
Outdated
Show resolved
Hide resolved
…erver connections
|
Kudos, SonarCloud Quality Gate passed!
|
What do these changes do?
This enables the mail-sending functionality of the webserver to handle
STARTTLSandTLS. Before, onlySTARTTLSwas possible. Furthermore, there were naming ambigouitites , as the env-vars referred toTLS, which is different fromSTARTTLSBonus:
STARTTLSandTLSare simultaneously requestedDeprecates the Env-Var:
SMTP_TLS_ENABLEDIntroduces the Env-Var:
SMTP_PROTOCOLwhich can have the values
UNENCRYPTEDTLSSTARTTLSRelated issue/s
How to test
I did a dry-run test, but since we do not fully emulate an external mail-server in our tests this will have to be properly checked in master. The dry-run test involved running the
send_mail(app: web.Application, msg: MIMEText)function vanilla in python, outside oSparc.Checklist