STOMP+SSL-related changes: security fixes + convenience enhancement (+ doc updates/improvements) #2414
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
zuo
force-pushed
the
jk/ssl-improvements-related-to-stomp-and-n6
branch
3 times, most recently
from
cdda076
to
425944f
Compare
zuo
force-pushed
the
jk/ssl-improvements-related-to-stomp-and-n6
branch
4 times, most recently
from
a32e17f
to
4d758a7
Compare
docs/user/bots.rst
Outdated
| @@ -947,7 +947,7 @@ Install the `stomp.py` library from PyPI: | |||
| * **Feed parameters** (see above) | |||
| * `server`: STOMP server's hostname or IP, e.g. "n6stream.cert.pl" (which is default) | |||
| * `port`: STOMP server's port number (default: 61614) | |||
| * `exchange`: STOMP *destination* to subscribe to, e.g. "/exchange/my.example.org/*.*.*.*" | |||
| * `exchange`: STOMP *destination* to subscribe to, e.g. ``"/exchange/my.example.org/*.*.*.*"`` | |||
There was a problem hiding this comment.
Without the `` marks rendering of the *.*.*.* fragment was wrong.
There was a problem hiding this comment.
the backtick markup is correct, to display it as code/preformatted text
There was a problem hiding this comment.
I have started the review, but I after a while of analysing it, I'd like to start with challenging some architectural decisions you've made.
I love the improvements in the security, however please see that the solution is really crafted for a very specific conditions, and can easily break with any upstream changes, as well as duplicate a lot of checks. As we support a library mode, I can imagine creating two objects of bots connecting to two different hosts in a one process - and then the Proxy will keep the first hostname and raise an exception.
Consider we re-design the solution in following way:
- Do not check things that are already granted, like minimum version of Python, if it doesn't change the code flow.
- Create a subclass of SSLContext, and override related methods there, instead of patching them. Keep the default behaviour any time the call isn't the one marking a special behaviour, but just use the
super()....for that. - The proxy should override only the expected objects (SSLContext class, create_default_context etc.), and do not change the
sslbehaviour in any other way.
If the conditions doesn't look like a call from expected stomp, do not raise - assume it was another legit use of the stomp etc., just not the one we want to secure.
|
|
||
| # Used if `auth_by_ssl_client_certificate` is false (otherwise ignored): | ||
| username: str = 'guest' # (STOMP auth *login*) | ||
| password: str = 'guest' # (STOMP auth *passcode*) |
There was a problem hiding this comment.
More just a FYI, but basically, if parameters are set by StompMixIn, it shouldn't be necessary to repeat them in bot's class. As there is quite a long part with parameters' explanation, I'd put it in the Mixin class, and just leave a note in bots' classes where to look for them. On the other hand, here they are easier accessible. What do you think, @zuo & @sebix?
There was a problem hiding this comment.
Yes, re-defining them is not necessary
intelmq/lib/mixins/stomp.py
Outdated
|
|
||
| def __patch_ssl_related_stomp_stuff(self, | ||
| host_and_ports: List[Tuple[str, int]]) -> None: | ||
| assert len(host_and_ports) == 1 |
There was a problem hiding this comment.
Do not rely on assertions in non-test code, they are intended for the debug purposes only, and can be easily disabled with optimizations.
intelmq/lib/mixins/stomp.py
Outdated
| host_and_ports = [(self.server, int(self.port))] | ||
| self.__patch_ssl_related_stomp_stuff(host_and_ports) |
There was a problem hiding this comment.
| self.__patch_ssl_related_stomp_stuff(host_and_ports) | |
| self.__patch_ssl_in_stomp(host_and_ports) |
Or so, to make it look better ;)
intelmq/lib/mixins/stomp.py
Outdated
| def __patch_ssl_related_stomp_stuff(self, | ||
| host_and_ports: List[Tuple[str, int]]) -> None: | ||
| assert len(host_and_ports) == 1 | ||
| [(server_hostname, _)] = host_and_ports |
There was a problem hiding this comment.
Instead of pack (L99), call, assert, unpack, ignore one, call please just write in L100:
_StompPyDedicatedSSLProxy.patch_stomp_transport_ssl(self.server)
and remove this function.
intelmq/lib/mixins/stomp.py
Outdated
| assert sys.version_info[:2] >= (3, 7) | ||
| assert stomp is not None | ||
| assert stomp.__version__ >= (4, 1, 8) | ||
| assert getattr(stomp, 'transport', None) is not None |
There was a problem hiding this comment.
Please remove. Those conditions are considered already guaranteed, so verifying them again with the debug-only asserts should be considered unnecessary - it would make sense in a mathematical proof, but not in a service. It introduces an additional place, where we possibly should maintenance the check - e.g., for the consistency, if we drop the Python 3.7 support, this place should also be updated.
OK, I will reconsider the design... One question: could we drop support for |
Yes, absolutely. We can also consider dropping support for < 6. |
zuo
force-pushed
the
jk/ssl-improvements-related-to-stomp-and-n6
branch
3 times, most recently
from
2e64fc1
to
a5d0922
Compare
zuo
force-pushed
the
jk/ssl-improvements-related-to-stomp-and-n6
branch
2 times, most recently
from
43a4b8c
to
717bbbf
Compare
There was a problem hiding this comment.
The more important review comes from @kamil-certat as he is a user/operator of the bot and knows more details, also from a user- and admin perspective.
|
|
||
| # Used if `auth_by_ssl_client_certificate` is false (otherwise ignored): | ||
| username: str = 'guest' # (STOMP auth *login*) | ||
| password: str = 'guest' # (STOMP auth *passcode*) |
There was a problem hiding this comment.
Yes, re-defining them is not necessary
|
OK, this is a redesigned version + with a few additional minor improvements/fixes/updates -- in particular, minimum |
|
@sebix @aaronkaplan Could you please merge this change? It's quite important for changes in n6 (using default CAs). |
|
@zuo We have merged the new docs system, may you merge |
OK, I will. |
…+ doc updates/improvements) doc: a few fixes/improvements ad STOMP bots and *n6* feed lib, bots, doc: only cosmetic/very minor tweaks and comments lib, bots, pkg, doc: drop support for `stomp.py` older than 4.1.12 The affected bots are: *STOMP collector* (`StompCollectorBot` defined in `intelmq.bots.collectors.stomp.collector`) and *STOMP output* (`StompOutputBot` defined in `intelmq.bots.outputs.stomp.output`). Also, in `debian/control`, the `python3-stomp` package name has been fixed (by removing the `.py` suffix). The changelog has been updated appropriately. lib, bots, doc: STOMP/*n6*-related fixes/enhancements, also ad security SSL-related changes -- regarding `intelmq.lib.mixins.StompMixin` and, therefore, also the *STOMP collector* bot (`StompCollectorBot` defined in `intelmq.bots.collectors.stomp.collector`) and the *STOMP output* bot (`StompOutputBot` defined in `intelmq.bots.outputs.stomp.output`) -- have been made: * *Security*-focused: fixed certain security problems which were caused by the fact that certain versions of the `stomp.py` library we need to be compatible with use the `ssl` module's tools in such ways that suffer from certain *security weaknesses*. In particular, `stomp.py` in versions `>=8.0, <8.1` mistakenly creates an `SSLContext` instance with the `check_hostname` flag unset -- an important negative effect of that is that the hostname of the STOMP server is *not* checked during the TLS handshake (making all STOMP communication vulnerable to certain kinds of attacks...). Also, there are weaknesses (caused either by `stomp.py` or by older, yet still supported by IntelMQ, Python versions) of using too old versions of the TLS protocol (namely: 1.0 and 1.1 -- nowadays considered insecure). * *Admin convenience*-focused: from now on, for each of the STOMP bots, you can set the `ssl_ca_certificate` config param to an empty string -- dictating that the SSL tools employed by the `stomp.py`'s machinery will attempt to load the system’s default CA certificates. Thanks to that, administrators of the given IntelMQ instance can be relieved of of the fuss with manual updates of the CA certificate(s) file -- *if* the certificate of the STOMP server can be verified using some of the publicly available CA certificates which are part of nearly all mainstream operating system distributions (this will be the case with the server certificate of the new variant of the *n6* Stream API, that is, the variant with STOMP-login-and-passcode-based authentication). An important part of the implementation of the aforementioned changes is a non-public class, `intelmq.lib.mixins.stomp._StompPyDedicatedSSLProxy` -- which implements a kind of transparent proxy object that wraps the `ssl` attribute of the `stomp.transport` module (originaly set to the `ssl` module object), replacing some of the `ssl` module's tools with their patched variants (note that the `ssl` module itself and all its members are left untouched). The parts of the IntelMQ's documentation related to those STOMP bots + integration with *n6* (including the CERT.PL's "N6 Stomp Stream" feed description) have been updated and improved; also, the changelog has been updated. bots: fix import logic in STOMP collector's module The logic regarding importing of the `stomp.py`'s stuff has been fixed: now the condition of the absence of the `stomp` module (and thus, of the entire library) would not be confused with the condition of the absence of only the `stomp.exception` module (which would mean the presence of a version of that library lacking just the `exception` submodule).
sebix
force-pushed
the
jk/ssl-improvements-related-to-stomp-and-n6
branch
from
717bbbf
to
735f415
Compare
|
I rebased the PR on the branch 'develop' and moved all docs to the new files. As the documentation changes were spread over five commits, I squashed them all together, because keeping the changes apart in separate commits, and moving every single change manually to the new format would be a heck of a lot of effort. As @kamil-certat approved the PR, I will merge it after the CI checks pass. |
|
@sebix @kamil-certat Oh, thank you for the rebase and merge! And I am sorry I haven't managed to do that promptly! |
|
@zuo Thank you for improving, maintaining and contributing to IntelMQ! |
The main purpose of this PR is to fix the following two problems -- both related to how the
stomp.pylibrary deals with SSL:(1) [ad security] Certain versions of
stomp.py, that IntelMQ needs to be compatible with, use thesslmodule's tools in ways that suffers from certain security weaknesses. In particular,stomp.py < 4.1.12uses a deprecated helper:ssl.wrap_socket()stomp.py >=8.0, <8.1mistakenly creates anSSLContextinstance with thecheck_hostnameflag unset; an important negative result is that the hostname of the STOMP server is not checked during the TLS handshake (!). Also, there are weaknesses (caused either bystomp.pyor by older Python versions) of using too old versions of TLS (namely: 1.0 and 1.1 -- today considered insecure).(2) [ad admin convenience] No version of
stomp.pymakes it possible to load the system's default CA certificates -- condemning administrators to bother with manual updates of the CA certificate(s) file, even if the certificate of the STOMP server the bots connect to could be verified using some of the publicly available CA certificates which are part of nearly all mainstream operating system distributions... Note: this is the case with the upcoming new n6 Stream API server's certificate.An important part of the changes to fix these problems is a non-public helper class,
intelmq.lib.mixins.stomp._StompPyDedicatedSSLProxy, which implements a kind of transparent proxy object: it wraps thesslattribute of thestomp.transportmodule (originaly set to thesslmodule object) replacing some of thesslmodule's tools with their patched variants. Note: thesslmodule itself and all its members are left untouched -- only thesslmember of thestomp.transportmodule is replaced with an instance of that class.For more details -- see the docstrings and comments in the code. [See the 4th commit...]
Apart from that, versions of
stomp.py >=4.1.8, <4.1.12are no longer supported + some details indebian/controlhave been fixed. [See the 3rd commit...]Together with the aforementioned changes, relevant parts of the documentation (including also the feeds spec and the changelog) have been appropriately updated + also improved by including some extra details concerning the upcoming switch to the new variant of n6 Stream API (the one with username+password-based authentication). [Also, see the 3rd and 4th commit...]
Also, an additional minor fix has been applied to the imports part of the STOMP collector's module, concerning the
stomp.py-imports-related logic. [See the 5th commit...]Plus, apart from all that, several additional (also STOMP-or-n6-integration-related) doc updates/improvements as well as very minor/cosmetic code tweaks have been made. [See the 1st, 2nd and 6th commit...]