SSL Certificate Verification For collectd-nut Plugin #2145

Geraden07 · 2017-01-22T11:49:27Z

See issue #1760 for full write up of changes and dev/implementation decisions, as well as link to tested scenarios using the new code.

Geraden07 · 2017-01-23T09:10:38Z

The first CI checks run on cf5de52 failed due to the use of strcpy which was poisoned, so I changed the code to use strncpy instead, and re-ran some spot checks to verify everything works well.

However, after the CI trigger-aggregation check failed for the latest version 7d8088a I am completely stumped. The failure has only happened on some of the systems (epel6, precise and wheezy) while building successfully on other systems (epel7, jessie, trusty, and xenial).

The errors from the failed builds are all the same:

cc1: warnings being treated as errors
src/nut.c: In function 'nut_read_one':
src/nut.c:201: error: implicit declaration of function 'upscli_init'
src/nut.c:206: error: implicit declaration of function 'upscli_cleanup'
src/nut.c:212: error: 'UPSCLI_CONN_CERTVERIF' undeclared (first use in this function)
src/nut.c:212: error: (Each undeclared identifier is reported only once
src/nut.c:212: error: for each function it appears in.)
make[1]: *** [src/nut_la-nut.lo] Error 1

The three symbols in error, upscli_init, upscli_cleanup and UPSCLI_CONN_CERTVERIF are all new references to the same file that was already being used in this code before any of my changes. In fact, the other constants used: UPSCLI_CONN_TRYSSL (existing) and UPSCLI_CONN_REQSSL (also new in my code) are defined literally lines away from UPSCLI_CONN_CERTVERIF in upsclient.h but only one of those is showing in the error as undeclared.

More confusing is the question of why this is happening on only some systems but not others. I imagine it's an issue with the linking but haven't the foggiest where to go next to continue debugging.

I did do a search and found that in the configure.ac file the following check happens:

AC_CHECK_LIB([upsclient], [upscli_connect],
     [with_libupsclient="yes"],
     [with_libupsclient="no (symbol upscli_connect not found)"]
)

I'm wondering if (even if this isn't the root cause) it's a good idea to add additional checks there for the new symbols I've added. However, I'm very uncertain that that would actually fix this issue (again I don't understand why it would happen on epel6 but not epel7 for example, considering I would guess they are somewhat similar environments).

I've checked the nut project to see when the code for those three symbols was added to the project, thinking it might be a version dependency thing, but I don't think that's the case because (again for example) the UPSCLI_CONN_CERTVERIF constant was part of the initial commit and defined at the same time as the already used constant UPSCLI_CONN_TRYSSL. So if one is present in a version of the libupsclient, then they both will be.

I may need some help from the project authors to get past this road block.

rubenk · 2017-01-23T10:47:12Z

You'll notice that the build only fails on older distros.
epel6 has nut 2.6, whereas epel7 has nut 2.7. The former doesn't define upscli_init in upsclient.h. The same goes for the other symbols.

Geraden07 · 2017-01-24T10:14:42Z

Oh you're right!

I was thrown because I looked at git blame of the upsclient.h file from the nut project. I saw the last commit to touch UPSCLI_CONN_TRYSSL and UPSCLI_CONN_REQSSL also touched UPSCLI_CONN_CERTVERIF and I assumed that meant all three were introduced at the same time. Obviously, if I actually go back and look at the commit I see the TRYSSL and REQSSL constants predated that commit even though they were moved by that commit.

Anyway, onto actually fixing the issue... I thought of three approaches:

Release a new version of collectd that adds a dependency on the 2.7 version of the library
Use code to detect if the upscli_init(), upscli_cleanup() and UPSCLI_CONN_CERTVERIF have been defined and define them as empty if not. Also set a flag to then avoid these calls since they wouldn't do anything.
Add a compile time check for the upscli_init() function and set a macro that can be used by compiler directives to define differing code based on whether upscli_init() was found.

Option number 1 seems heavy handed just for changes to a small plugin, so I assumed that was off the table.

Option number 2 would work, but seemed a little less clean than option 3, so I've implemented the fix using option 3's approach.

I believe the builds will all pass now. Will check back in an hour or two to verify.

Geraden07 · 2017-01-24T11:24:07Z

Okay all checks are passing. To summarize:

ForceSSL support has been added.
VerifyPeer (and CAPath) support has been added if libupsclient provides upscli_init() (ie: libupsclient version >= 2.7).
VerifyHost cannot be implemented at this time.
Documentation was updated (src/collectd.conf.in and src/collectd.conf.pod)

I believe everything is all set for reviewing and merging if there are no concerns about the code.

rubenk · 2017-02-14T10:24:00Z

configure.ac

@@ -5186,6 +5186,11 @@ if test "x$with_libupsclient" = "xyes"; then
    [with_libupsclient="no (symbol upscli_connect not found)"]
  )

+  AC_CHECK_LIB([upsclient], [upscli_init],
+    [AC_DEFINE(WITH_UPSCLIENT_27, 1, [At least version 2-7])],


Please quote all the arguments.

I looked at a number of other AC_DEFINE calls within the same file and none of them have their arguments quoted. Most had square brackets around them though. Is this what you meant?

I'll add square brackets but not quotes to keep with the style of the rest of the file.

rubenk · 2017-02-14T10:25:09Z

src/collectd.conf.in

@@ -942,6 +942,9 @@

 #<Plugin nut>
 #	UPS "upsname@hostname:port"
+#   ForceSSL true


Please use tabs here.

rubenk · 2017-02-14T10:25:40Z

src/collectd.conf.pod

@@ -4082,11 +4082,11 @@ If enabled, the plugin sends a notification if the replication slave I/O and /
 or SQL threads are not running. Defaults to B<false>.

 =item B<WsrepStats> I<true|false>
- 
+


Unrelated change.

All these "unrelated changes" are simply a result of my editor removing trailing whitespace from lines on save.

I will be rebasing to tidy up this pull request and will make sure to remove these changes from my change set.

rubenk · 2017-02-14T10:25:49Z

src/collectd.conf.pod

 Enable the collection of wsrep plugin statistics, used in Master-Master
 replication setups like in MySQL Galera/Percona XtraDB Cluster.
 User needs only privileges to execute 'SHOW GLOBAL STATUS'
-


rubenk · 2017-02-14T10:27:11Z

src/collectd.conf.pod

@@ -7984,7 +8013,7 @@ will be collected.
 =item B<BlockDeviceFormat> B<target>|B<source>

 If I<BlockDeviceFormat> is set to B<target>, the default, then the device name
-seen by the guest will be used for reporting metrics. 


unrelated change.

rubenk · 2017-02-14T10:27:50Z

src/nut.c

 static int config_keys_num = STATIC_ARRAY_SIZE(config_keys);
+static int force_ssl = 0; // Initialized to default of 0 (false)


there's no need to zero-initialize static variables.

This isn't about initializing to 0 to ensure the value is zero from a programmatic standpoint. The default behavior if the config does not specify a value should be false (0). Setting it to 0 makes it clear that if it doesn't get changed by the config file, it will be 0 and thus equivalent to "false".

rubenk · 2017-02-14T10:30:57Z

src/nut.c

@@ -123,6 +168,121 @@ static void nut_submit(nut_ups_t *ups, const char *type,
  plugin_dispatch_values(&vl);
 } /* void nut_submit */

+static int nut_connect(nut_ups_t *ups){
+#ifdef WITH_UPSCLIENT_27


this function is getting overly long. Any chance you could split is out into something like nut_connect_tls?

The reason it's so long is because it's actually the same function twice but with differing logic, but between compiler #if directives.

If the compiler detects that the UPSCLIENT version is 2.7 or greater it can compile using the first version of the function which allows for calls to upscli_init (and a few other symbols). This is what enables the certificate verification to happen.

If however the compiler detects that the UPSCLIENT version is 2.6 or less, before the upscli_init (and other) symbols were available, we use a different definition that will connect without verification and give warning if the user tries to enforce VERIFYPEER because it is unsupported with that library version.

The two definitions separately themselves are not actually too long (40 and 60 lines of code respectively, give or take). I thought about refactoring them out into their own functions with differing names and then nut_connect will call whichever one based on the compiler flag, but we would still need to hide the function with the cert verification logic behind the compiler flags too.

This would result in a differing set of function symbols being available in our code, based on which version of the upsclient library is present. I dislike this approach, so I think ultimately the code should remain as it is now. I think this is the simplest, most straight forward way that will result in the least confusion about whats going on.

rubenk · 2017-02-14T10:32:59Z

src/nut.c

+    force_ssl = 1;
+  else if (strcasecmp(value, "false") == 0)
+    force_ssl = 0; // Should already be set to 0 from initialization
+  else{


space before the brace. If you format the code with clang-format -style=LLVM it should do this for you. See also https://collectd.org/wiki/index.php/Repository#Automatic_formatting

I'll be rebasing to tidy things up and will make sure to do this.

rubenk · 2017-02-14T10:36:44Z

Thank you @Geraden07. I left some minor comments inline.

…r information.

Geraden07 · 2017-02-19T07:16:46Z

As per my comments on the review notes, I have:

run clang-format -style=LLVM -i on nut.c to ensure it's formatted as desired.
converted to using tabs in the documentation file where the reviewer asked
added square brackets around arguments in the configure.ac file to keep the style of the existing code
rebased so the pull request now only has two commits. They have been split up by feature (ForceSSL and certificate checking) and include all the fixes mentioned above, as well as the commits fixing the build issues.

Cheers

…lient library version >= 2.7. Otherwise will show warnings if this feature is used.

rubenk · 2017-02-19T11:55:11Z

Thanks again. For the next time, please don't rebase unless the reviewer asks for it, otherwise Github looses the review comments and the reviewer has to start all over again.

For now I trust you addressed them all ;)

rubenk · 2017-02-20T09:54:12Z

On 19 Feb 2017, at 05:48, Steven B ***@***.***> wrote: @Geraden07 commented on this pull request. In configure.ac: > @@ -5186,6 +5186,11 @@ if test "x$with_libupsclient" = "xyes"; then [with_libupsclient="no (symbol upscli_connect not found)"] ) + AC_CHECK_LIB([upsclient], [upscli_init], + [AC_DEFINE(WITH_UPSCLIENT_27, 1, [At least version 2-7])], I looked at a number of other AC_DEFINE calls within the same file and none of them have their arguments quoted. Most had square brackets around them though. Is this what you meant? I'll add square brackets but not quotes to keep with the style of the rest of the file.

Ah I should have been more specific. Square brackets are quotes in m4, the language these macros are written in. So that's exactly what I meant. Thanks!

rubenk reviewed Feb 14, 2017

View reviewed changes

rubenk reviewed Feb 14, 2017

View review 10000 ed changes

rubenk reviewed Feb 14, 2017

View reviewed changes

Added ForceSSL feature. Updated documentation and added my contributo…

7fe1468

…r information.

Geraden07 force-pushed the master branch from b0fd7b1 to cdd089c Compare February 19, 2017 06:02

Implemented certificate checking conditionally if compiler finds upsc…

1c1b3f9

…lient library version >= 2.7. Otherwise will show warnings if this feature is used.

Geraden07 force-pushed the master branch from cdd089c to 1c1b3f9 Compare February 19, 2017 10:43

rubenk merged commit 4138e6c into collectd:master Feb 19, 2017

octo added this to the 5.8 milestone Oct 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SSL Certificate Verification For collectd-nut Plugin #2145

SSL Certificate Verification For collectd-nut Plugin #2145

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -4082,11 +4082,11 @@ If enabled, the plugin sends a notification if the replication slave I/O and /
		or SQL threads are not running. Defaults to B<false>.

		=item B<WsrepStats> I<true\|false>

		static int config_keys_num = STATIC_ARRAY_SIZE(config_keys);
		static int force_ssl = 0; // Initialized to default of 0 (false)

SSL Certificate Verification For collectd-nut Plugin #2145

SSL Certificate Verification For collectd-nut Plugin #2145

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!