sshguard-users

[SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin B. <kev...@gm...>]

Hi there,

having noticed that someone added an entry, into a whitelist file, that was
already covered by a range further up the file, it struck me that it might
be useful if there could be comments after the to-be-whitelisted IP address
or range, eg

127.0.0.0/8		# Loopback range
nnn.nnn.nnn.nn/20	# Some Org's range

and so on, rather than having commented lines above each entries.

Looking into the code I've  the following patch would achieve the desired
result (although strsep() isn't as portable as a much more convoluted strtok()
based "soultion") but there may be some test cases you have that I can't check
against.

Any clues/pointers there and/or would there be any interest in developing this
idea so that it does? (Or just accepting it as is, if it passes your tests!)

Furthermore, would there be any interest in having an extra flag, in the blocker,
that would "turn on" some logging of successful parsing of the whitelist, that
could then be used in testing (some people here still aren't convinced) and,
whilst I'm at that, I could even expand the usage() function to spit out the
option flags and a brief description, rather than just, well, you know what it
does at present.

No probs if this doesn't "float your boat", and thanks for SSHGuard,
Kevin Buckley

That patch in the body of the email for perusal (also attached as a file)

diff -ur sshguard-2.4.0-dist/src/blocker/sshguard_whitelist.c sshguard-2.4.0/src/blocker/sshguard_whitelist.c
--- sshguard-2.4.0-dist/src/blocker/sshguard_whitelist.c        2018-12-16 10:41:51.000000000 +0800
+++ sshguard-2.4.0/src/blocker/sshguard_whitelist.c     2020-05-13 15:34:14.857159691 +0800
@@ -137,6 +137,7 @@
      char line[WHITELIST_SRCLINE_LEN];
      int lineno = 0;
      size_t len;
+    char* pos;
  
  
      if (filename == NULL) return -1;
@@ -155,6 +156,9 @@
          len = strlen(line);
          if (len == 0) continue;
          if (line[len-1] == '\n') line[len-1] = '\0';
+        /* handle EOL spaces, TABs and comments, */
+        pos = line;
+        strsep(&pos, " \t#");
          /* handling line */
          if (whitelist_add(line) != 0) {
              sshguard_log(LOG_ERR, "whitelist: Unable to handle line %d from whitelist file \"%s\".", lineno, filename);

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin Z. <kev...@gm...>]

Hi Kevin,

On 5/13/20 2:33 AM, Kevin Buckley wrote:
> having noticed that someone added an entry, into a whitelist file,
> that was already covered by a range further up the file, it struck me
> that it might be useful if there could be comments after the
> to-be-whitelisted IP address or range, eg

That would be a welcome change.

> Looking into the code I've  the following patch would achieve the
> desired result (although strsep() isn't as portable as a much more
> convoluted strtok() based "soultion") but there may be some test
> cases you have that I can't check against.
> 
> Any clues/pointers there and/or would there be any interest in 
> developing this idea so that it does? (Or just accepting it as is, if
> it passes your tests!)

I will take a closer look soon, but I'm inclined to accept this patch
"as is". Without checking too closely, it seems like strsep() is
available on all of the platforms listed on SSHGuard's download page.

> Furthermore, would there be any interest in having an extra flag, in
> the blocker, that would "turn on" some logging of successful parsing
> of the whitelist, that could then be used in testing (some people
> here still aren't convinced) and, whilst I'm at that, I could even
> expand the usage() function to spit out the option flags and a brief
> description, rather than just, well, you know what it does at
> present.

What about a separate test program like 'sshg-whitelist' that takes IP
addresses on stdin and filters whitelisted addresses away from stdout?

And some documentation, of course, suggesting that it's there.

Regards,
Kevin

-- 
Kevin Zheng
kev...@gm... | ke...@be...
XMPP: ke...@ee...

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin Z. <kev...@gm...>]

On 5/13/20 2:33 AM, Kevin Buckley wrote:
> Furthermore, would there be any interest in having an extra flag, in
> the blocker, that would "turn on" some logging of successful parsing
> of the whitelist, that could then be used in testing (some people
> here still aren't convinced) and, whilst I'm at that, I could even
> expand the usage() function to spit out the option flags and a brief
> description, rather than just, well, you know what it does at
> present.

We can also add an sshguard_log() line right after whitelist_conf_fin()
on src/blocker/blocker.c:132 that says how many address or address
ranges are on the whitelist.

A patch to do that is attached; what do you think?

-- 
Kevin Zheng
kev...@gm... | ke...@be...
XMPP: ke...@ee...

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin B. <kev...@gm...>]

On 2020/05/14 00:58, Kevin Zheng wrote:
> On 5/13/20 2:33 AM, Kevin Buckley wrote:
>> Furthermore, would there be any interest in having an extra flag, in
>> the blocker, that would "turn on" some logging of successful parsing
>> of the whitelist, that could then be used in testing (some people
>> here still aren't convinced) and, whilst I'm at that, I could even
>> expand the usage() function to spit out the option flags and a brief
>> description, rather than just, well, you know what it does at
>> present.
> 
> We can also add an sshguard_log() line right after whitelist_conf_fin()
> on src/blocker/blocker.c:132 that says how many address or address
> ranges are on the whitelist.
> 
> A patch to do that is attached; what do you think?
> 

Other than neededing to bump up the log level to LOG_ERR to see this
working on my test system, that would clearly be a useful addition,
however, our testing would suggest that, when the whitelist parser
"gets it wrong", then it really gets it wrong, and just segfaults*.

My "extra flag for debuging" idea would have just seen everything
dumped into the log stream by adding in some

if( debug_flag_used) then
   sshguard_log(LOG_ERR, "stuff")
fi

guards, on the understanding that, if you invoked SSHGuard with the
debug flag, you would want to see some output without needing to alter
anything else on the system outside of SSHGuard.

Then again, maybe all of SSHGuard's sshguard_log() lines could be wrapped
in such guards, thereby affording a fuller control of log output at
invocation, along the lines of a "multiple debug flags increase verbosity"
approach.

Clearly an idea that needs a bit more fleshing out,
Kevin

* What we originally found, after adding EOL comments to a whitelist file
was that SSHGuard "appeared" to work OK, except that it wasnt, and it was
only when we added a comment along the lines of:


nnn.nnn.nnn.nnn   # Host at some org but covered by range nnn.nnn.nnn.0/30 above


that we realised that the whitelist parser (whitelist_add() function)
was seeing the "/" in the comment as indication of a range and then
"not quite getting there" when parsing the full line.

Deep sorrow!

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin Z. <kev...@gm...>]

On 5/13/20 6:45 PM, Kevin Buckley wrote:
> Other than neededing to bump up the log level to LOG_ERR to see this
> working on my test system, that would clearly be a useful addition,
> however, our testing would suggest that, when the whitelist parser
> "gets it wrong", then it really gets it wrong, and just segfaults*.
> 
> My "extra flag for debuging" idea would have just seen everything
> dumped into the log stream by adding in some
> 
> if( debug_flag_used) then
>   sshguard_log(LOG_ERR, "stuff")
> fi
> 
> guards, on the understanding that, if you invoked SSHGuard with the
> debug flag, you would want to see some output without needing to alter
> anything else on the system outside of SSHGuard.

Do you mean to say that you're running in an environment where setting
environment variables is not possible?

Currently, LOG_DEBUG lines are only displayed when SSHGUARD_DEBUG is set
in the environment, see sshguard(8).

> * What we originally found, after adding EOL comments to a whitelist file
> was that SSHGuard "appeared" to work OK, except that it wasnt, and it was
> only when we added a comment along the lines of:
> 
> 
> nnn.nnn.nnn.nnn   # Host at some org but covered by range
> nnn.nnn.nnn.0/30 above
> 
> that we realised that the whitelist parser (whitelist_add() function)
> was seeing the "/" in the comment as indication of a range and then
> "not quite getting there" when parsing the full line.

Sounds like what we should really have is a better-written parser.
Perhaps we should write a better IP list parser and combine the
whitelist and blacklist parsers to support comments?

Regards,
Kevin

-- 
Kevin Zheng
kev...@gm... | ke...@be...
XMPP: ke...@ee...

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin B. <kev...@gm...>]

On 2020/05/15 02:36, Kevin Zheng wrote:
>
> Do you mean to say that you're running in an environment where setting
> environment variables is not possible?

No.

> Currently, LOG_DEBUG lines are only displayed when SSHGUARD_DEBUG is set
> in the environment, see sshguard(8).

So that's the missing piece for that then.

>> * What we originally found, after adding EOL comments to a whitelist file
>> was that SSHGuard "appeared" to work OK, except that it wasnt, and it was
>> only when we added a comment along the lines of:
>> 
>> 
>> nnn.nnn.nnn.nnn   # Host at some org but covered by range
>> nnn.nnn.nnn.0/30 above
>> 
>> that we realised that the whitelist parser (whitelist_add() function)
>> was seeing the "/" in the comment as indication of a range and then
>> "not quite getting there" when parsing the full line.
> 
> Sounds like what we should really have is a better-written parser.
> Perhaps we should write a better IP list parser ...


In all fairness, if one is happy to ONLY have comments on lines of
their own, so above or below individual entries, then the existing
parser is fine.

Furthermore, none of the example files show End-of-line comments.

However, if you do not wish to have your IP addresses and ranges
surrounded by comments, and it could make the file a bit unwiedly,
and/or the IP addresses and ranges a little harder to discern, then
the existing parser doesn't always do the right thing.

Making the latter work is what the addition of the strsep() proposal
addresses, not any percieved issue with the existing parser when
used against whitelists that dont have EOL comments.


> ... and combine the
> whitelist and blacklist parsers to support comments?

The ability to annotate a blacklist with EOL comments might add
some operational benefit, as well as providing consistency in
the way the black and white lists are handled, which then reduces
any potential for confusion and errors in the input files.

Kevin

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin B. <kev...@gm...>]

On 2020/05/15 02:36, Kevin Zheng wrote:
> 
> Currently, LOG_DEBUG lines are only displayed when SSHGUARD_DEBUG is set
> in the environment, see sshguard(8).

Another observation on that (now that I'm aware of it, that is!).

The various log messages already in SSHGuard go out via syslog, vis:

$ cat sshguard_log.h
...
#define sshguard_log syslog
$

which is a nod in the direction of SSHGuard being run as a daemon.


In the kind of debug message that I was thinkng about when suggesting
"adding a debug flag", I was thinking about the case where one runs
the blocker from the command line and so might want to see messages
directly on the console. (The typical run daemon in non-detached mode)

I had found myself sprinkling a few "fprintf(stderr, ...)" across
the codebase and so was more thinking of the "debug flag" as a way
to wrap those.

Hoping that serves to inform the discussion a little more,
Kevin

Re: [SSHGuard-users] [Proposal] Handle End-of-line comments in Whitelists

[Kevin Z. <kev...@gm...>]

On 5/14/20 9:14 PM, Kevin Buckley wrote:
> The various log messages already in SSHGuard go out via syslog, vis:
> 
> $ cat sshguard_log.h
> ...
> #define sshguard_log syslog
> $
> 
> which is a nod in the direction of SSHGuard being run as a daemon.
> 
> 
> In the kind of debug message that I was thinkng about when suggesting
> "adding a debug flag", I was thinking about the case where one runs
> the blocker from the command line and so might want to see messages
> directly on the console. (The typical run daemon in non-detached mode)

Yes, you read that part correctly.

However, the blocker also passes the SSHGUARD_DEBUG environment variable
to init_log, which calls openlog() with LOG_PERROR. If your syslog
supports it, then:

LOG_PERROR    Write the message to standard error output as well to the
              system log.

I believe running sshg-blocker directly from the terminal does correctly
log to standard error.

-- 
Kevin Zheng
kev...@gm... | ke...@be...
XMPP: ke...@ee...