-
Notifications
You must be signed in to change notification settings - Fork 438
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
Notify syscall 180709 #457
Notify syscall 180709 #457
Conversation
@@ -135,6 +135,8 @@ def setify(cp): return set((section, option) for section in cp.sections() | |||
parser.read(user) | |||
|
|||
for item in setify(parser).difference(a): | |||
if item[0]=="syscall": |
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.
spaces around operator
@@ -0,0 +1,114 @@ | |||
|
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.
Could you add this in the docs
folder and link it so it appears on the website when @posativ will have time to update stuffs.
if comment["email"]: | ||
author += " <%s>" % comment["email"] | ||
|
||
rv.write(author + " wrote:\n") |
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.
Use string formatting instead of concatenation. (it applies for all the function below :) )
Also calling only once rv.write
for subsequent calls would be better.
|
||
def _delete_comment(self, id): | ||
logger.info('comment %i deleted', id) | ||
logger.info("comments.delete %i ", id) |
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.
Remove the trailing space in the string
|
||
def _activate_comment(self, id): | ||
logger.info("comment %s activated" % id) | ||
logger.info("comments.activate %s" % id) |
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.
Do not use string formatting here, let logging do it. It will compute strings only if needed according to the log level.
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 saw there's more above of this too to correct
msgbody=_format(thread,comment,self.general_host,key) | ||
subject = "..." + thread['uri'][-15:] + " :: " + comment['text'][:15] + "..." | ||
cmdlist = [ a.replace('{{SUBJECT}}',subject,1) for a in self.conf.getiter('call') ] | ||
logger.info(cmdlist); |
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.
useless semicolon
key=self.isso.sign(comment["id"]) | ||
msgbody=_format(thread,comment,self.general_host,key) | ||
subject = "..." + thread['uri'][-15:] + " :: " + comment['text'][:15] + "..." | ||
cmdlist = [ a.replace('{{SUBJECT}}',subject,1) for a in self.conf.getiter('call') ] |
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.
No space after opening bracket.
No space before opening bracket.
Spaces after comma.
subject = "..." + thread['uri'][-15:] + " :: " + comment['text'][:15] + "..." | ||
cmdlist = [ a.replace('{{SUBJECT}}',subject,1) for a in self.conf.getiter('call') ] | ||
logger.info(cmdlist); | ||
p=subprocess.run(cmdlist, |
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.
spaces around assignement operator
+ "stdout:\n" \ | ||
+ str(p.stdout) + "\n" \ | ||
+ "stderr:\n" \ | ||
+ str(p.stderr) + "\n" |
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.
Use string formatting. And maybe multiline string using triple quotes?
input=str(msgbody).encode(), | ||
stdout=subprocess.PIPE, | ||
stderr=subprocess.PIPE) | ||
s = "Syscall: return code " + str(p.returncode) + "\n" \ |
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.
Name it logged_string
or something more significant.
Ok for now I checked the code and it seems ok. Some minor changes (even though there's a few). Important stuff to check:
I still need to test the PR too :) |
Stale and unlikely to ever be merged without a massive rebase. |
I should have closed this pull long ago - apologies. There really isn't any great need for the suggested feature - what exists is perfectly satisfactory. |
Send Notifications by arbitrary program via python systemcall
The [isso] repository was forked by github user craigphicks. A branch
notify-syscall-180709
was created. The branch is described herein.The main uncertainly I have as author is the use of
subprocess.run
which appeared in Python3.5. Which versions of Python must be supported? Must Python 2 be supported? (See Limitations at the end of this document).This comment is copied from the file ./README-notify-syscall-180709 in hte main directory.
Motivation
Security
When isso uses the SMTP notification backend, it also sends the username and password. The are various perspectives from which this could be problematic.
Password in clear: In the worst case TLS (Transport Layer Security) is not established before passing the password and so the password is transmitted in the clear.
Key better than password: Even with TLS, the password is still in the clear at either end. While this is usually OK, security can be improved by using a key which can be periodically refreshed. The point of the key is that is that it is only authorizes a narrow range of actions. Especially, it does not allow logging in as mail account owner, which could be very damaging.
Dependence on Python SMTP implementation
Isso is tied to the Python SMTP implementation. Even if SMTP is the desired protocol, the may be other software besides the Python SMTP implementation which gives a better result, e.g. in terms of enabling TLS or working with tokens.
Some examples of alternate software are the multitude of generic sendmail programs, e.g. sendmail, postfix, exim, nullmailer, all of which all share the linux sendmail application interface as a standard.
Also there utilities such as curl and swaks which are very flexible with many options. In case something is not working it is almost always possible to work through it with these programs (or change to another program). Although they are not designed for production use, they are sufficient on the usage scale expected by a typical isso installation.
Solution
Outline
The isso administrator specifies the program and it's arguments in the isso configuration file. When a comment notification event occurs the specified program and arguments are executed via a system call. The message is passed to the standard input of that program.
Administrator interface
Add
syscall
to the list of notify backends:Create a section
[syscall]
and therein specify the program and its arguments. The following example showscurl
being used to transmit an email request through a (free) Mailgun account:The key
call
appears on the first line followed by equal. The value is split between lines with exactly one argument per line. Each successive value line must have an empty space at the beginning.Often in a shell command line quotation mark must be used to prevent a parser from interpreting spaces as being argument delimiters. E.g., for the subject
a bash script would require quotes placed like
"subject=isso {{SUBJECT}}"
orsubject="isso {{SUBJECT}}"
to keep the argument together. The shell would interpret the quotes for their syntactic meaning, and then remove them before passing each argument phrase to the program as, e.g. argv[11] or equivalent.However, for the isso configuration
call
value above, quotes are not required because the grouping is done by lines. In fact quotes for the purpose of syntactic grouping must NOT be used. The quotes will not be removed before passing to the program, and the input will likely be invalid. Quotes can still be used as part of the content of any argument, as required.Some (but not all) scenarios require the mail subject to be specified as part of the arguments, and not in the standard input data. The above
curl
example shows such a case. In order to enable variable subject line content a template parameter%SUBJECT%
is provided. The isso software will render this template %SUBJECT% as the fixed formula:The last part of the last
curl
argument: `<-' , is a curl specific instruction to accept data from standard input. Because the message body is variable it must be transmitted via standard input to the process. So any program used must be configured (or already be hard coded) to accept standard input.Be aware that protocol issues depend on the called program and the site receiving the data. It is not handled by the isso code. For example, although the above example appears to transact via HTTPS, there is some possibility it might downgrading to HTTP after authentication for the message transaction part. (The log file is still under interpretation.)
Read the current limitations section at the end of this document.
Implementation
The notify-syscall-180708 branch adds a new module
Syscall
in theisso/ext/notifications.py
file.It formats the mail message body in the same way as the existing
SMTP
module, but without the SMTP header and quopri / base64 encoding.The command and arguments as specified in the configuration are supplied as a list which is the first parameter of
subprocess.run
.The message body is encoded to binary and passed as standard input to the invoked process.
When complete,
subprocess.run
returns an object with membersreturncode
,stdout
, and 'stderr'. All are logged via the global logger (usually to/var/log/isso.log
).Isso expects that success is indicated by a
returncode
value of zero, and anything else is error. However, an error does not result in an isso program error. The only difference in isso program behavior is that for a non-zero result isso will log viaand for a zero result via
Other changes
The notification backend
Stdout
module was rewritten to fix a bug caused by trying to log a bloom filter as text. Also, the relation between function names, log content, and events was cleaned up.IMPORTANT: Current Limitations
Implementation
subprocess.run
is not available before Python 3.5. Certainly not in Python2. In environments where the code will not run it should not be enabled in the configuration file, (if it even compiles).