-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
STOMP-and-n6-related updates, fixes and enhancements, especially adding login-based authentication #2408
STOMP-and-n6-related updates, fixes and enhancements, especially adding login-based authentication #2408
Conversation
docs/user/bots.rst
Outdated
* `ssl_ca_certificate`: path to CA file | ||
* `ssl_client_certificate`: path to client cert file | ||
* `ssl_client_certificate_key`: path to client cert key file | ||
auth_by_ssl_client_certificate`: Boolean, default: true (note: set to false for new *n6* auth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth_by_ssl_client_certificate`: Boolean, default: true (note: set to false for new *n6* auth) | |
* `auth_by_ssl_client_certificate`: Boolean, default: true (note: set to false for new *n6* auth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: now I added also CHANGELOG.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now: a few corrections in CHANGELOG.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now (among others): more corrections/additions in CHANGELOG.md
.
docs/user/bots.rst
Outdated
* `stomp_login`: STOMP login (e.g., *n6* user login), used only if `auth_by_ssl_client_certificate` is false | ||
* `stomp_passcode`: STOMP passcode (e.g., *n6* user API key), used only if `auth_by_ssl_client_certificate` is false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about calling them username
and password
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used these names because login
and passcode
are STOMP-(protocol)-specific terms/keywords.
But if you think username
and password
will be better here, I will change them; it's not a problem. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what is your final decision, @sebix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamil-certat @gethvi what do you think?
username/password would be consistent with the parameters of other bots and independent of the underlying protocol.
login/passcode is in line with the specific protocol
I tend towards consistency and username/password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, done. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use usernam/password or login/passcode (stomp prefix doesn't look like necessary when we are speaking about Stomp bot :))
25ca767
to
d4fee3e
Compare
@sebix Hm, the only failing test does not seem to be related to this PR:
Maybe re-run could help?... |
Yes, it did |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very well, just a small suggestion. Do you have any date when the transition from certs to credentials occurs?
|
||
# check if certificates exist | ||
for f in [self.ssl_ca_cert, self.ssl_cl_cert, self.ssl_cl_cert_key]: | ||
for f in ssl_params.values(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the ssl_cs_certtificate
is still important, maybe let's add the check if the file exists in the check
method as well? It is used during verification if IntelMQ is properly configured, see e.g.:
intelmq/intelmq/bots/outputs/file/output.py
Lines 92 to 104 in 85c215d
@staticmethod | |
def check(parameters): | |
if 'file' not in parameters: | |
return [["error", "Parameter 'file' not given."]] | |
dirname = os.path.dirname(parameters['file']) | |
if not os.path.exists(dirname) and '{ev' not in dirname: | |
path = Path(dirname) | |
try: | |
path.mkdir(mode=0o755, parents=True, exist_ok=True) | |
except OSError: | |
return [["error", "Directory (%r) of parameter 'file' does not exist and could not be created." % dirname]] | |
else: | |
return [["info", "Directory (%r) of parameter 'file' did not exist, but has now been created." % dirname]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will add such checks, together with some refactoring (to reduce code duplication...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I just added (among others) those checks.
docs/user/bots.rst
Outdated
* `stomp_login`: STOMP login (e.g., *n6* user login), used only if `auth_by_ssl_client_certificate` is false | ||
* `stomp_passcode`: STOMP passcode (e.g., *n6* user API key), used only if `auth_by_ssl_client_certificate` is false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use usernam/password or login/passcode (stomp prefix doesn't look like necessary when we are speaking about Stomp bot :))
Soon -- in a few days -- CERT.pl will contact all interested parties (i.e., organizations that connect to the n6 Stream API), in particular (obviously) CERT.at, conveying necessary information on the upcoming auth mechanism change. As far as I know, it is planned to turn off the old mechanism (client-certificate-based auth) around mid-November. |
Thanks for the info! |
d4fee3e
to
a247811
Compare
The latest version is a refactored one + with additional changes. In particular, now both bots implement the |
a247811
to
bfc6d4f
Compare
A few tweaks more... Now, it should be ready. :-) (I hope...) |
e752d58
to
93ab34b
Compare
@kamil-certat @sebix Sorry for the fuss with the recent forced pushes... Now the stuff is ready. |
The updates, fixes and improvements regard the *STOMP collector* and *STOMP output* bots. Important changes are described below... From now on, newer versions of the `stomp.py` package are supported -- including the latest (8.1.0). Now both STOMP bots coerce the `port` configuration parameter to int -- so that a string representing an integer number is also acceptable (even if not recommended) as a value of that parameter. In the *STOMP output* bot, a bug has been fixed: `AttributeError` caused by attempts to get unset attributes (`ssl_ca_cert` and companions...). The *STOMP collector*'s reconnection mechanism has been fixed: from now on, no reconnection attempts are made after `shutdown()`. Apart from that, reconnection is not attempted at all for versions of `stomp.py` older than 4.1.21 (as it did not work properly anyway). Also regarding the *STOMP collector* bot, the following (undocumented and unused) attributes of `StompCollectorBot` instances are no longer set in `init()`: `ssl_ca_cert`, `ssl_cl_cert`, `ssl_cl_cert_key`. Various checks have been improved/enhanced. Now, for example, both STOMP bot classes implement the `check()` static/class method -- whose role is to check ("statically", without the need to run the bot) configuration parameters; in particular, it checks whether necessary certificate files are accessible. When it comes to runtime (on-initialization) checks, one notable improvement is that now also the *STOMP output* bot will raise a `MissingDependencyError` if the `stomp.py` version is older than 4.1.8 (an analogous check has already been implemented by *STOMP collector*). The code of those bot classes have also been significantly refactored -- in particular, several common operations have been factored out and placed in a new mix-in class: `intelmq.lib.mixins.StompMixin`; its definition resides in a new module: `intelmq.lib.mixins.stomp`.
Each of the *STOMP collector* and *STOMP output* bots obtained the following new configuration parameters: * `auth_by_ssl_client_certificate` (a Boolean flag; it is `True` by default -- to keep backward compatibility); * `username` and `password` -- to be used as STOMP authentication credentials (login and passcode), but *only* if the aforementioned parameter `auth_by_ssl_client_certificate` is `False`. If `auth_by_ssl_client_certificate` is `False`, then the (supported also previously...) `ssl_client_certificate` and `ssl_client_certificate_key` parameters are ignored (i.e., not only left unused, but also there are *no checks* whether the files they refer to actually exist).
The changes include also those regarding *feeds* (values of certain properties of the CERT.PL's "N6 Stomp Stream" feed entry have been updated/improved) and the *changelog*.
eb8e0b4
to
7c59d49
Compare
Now just rebased on the latest |
This is a bunch of updates, fixes and enhancements related to integration with STOMP-based services, especially with the n6's one.
The most important change is related to the upcoming switch of authentication to the n6 Stream API: from client certificate-based to login-and-passcode-based. Other changes focus on compatibility with newer versions of the
stomp.py
library as well as on minor fixes/cleanups/improvements (mostly consistency-related).The key changes concern the STOMP collector bot and the STOMP output bot:
auth_by_ssl_client_certificate
: if true (the default), the legacy certificate-based authentication will be attempted (sossl_client_certificate
andssl_client_certificate_key
need to be specified); if false, the new login-based authentication will be attempted (so the config parameters described below need to be specified);stomp_login
username
: a STOMP login (in the case of n6 it is just the user's login, the same which you use to log in to n6 Portal);stomp_passcode
password
: a STOMP passcode (in the case of n6 it is the user's API key which can be obtained via n6 Portal).stomp.py
library are supported, including the latest (8.1.0).See also: the commit messages.