Add input plugin for OpenLDAP #2612

acobaugh · 2017-04-03T14:40:17Z

This is a plugin to pull metrics out of cn=Monitor from a local or remote OpenLDAP server.

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

danielnelson · 2017-04-03T18:49:47Z

Can you add some unit tests?

…wn function that we can use to mock out a search result in testing

…sts()

acobaugh · 2017-04-07T15:26:27Z

Added unit tests. There is a very simple test function that mocks up a SearchRequest and feeds that in, and the integration tests connect to a super simple openldap container.

phemmer · 2017-04-12T12:37:25Z

plugins/inputs/openldap/README.md

+```
+$ telegraf -config telegraf.conf -input-filter openldap -test --debug
+* Plugin: inputs.openldap, Collection 1
+> openldap,port=389,host=localhost,server=localhost abandon_operations_initiated=4,extended_operations_completed=125963,bytes_statistics=595939321,pdu_statistics=17028251,modify_operations_initiated=0,delete_operations_completed=0,compare_operations_completed=0,max_file_descriptors_connections=4096,unbind_operations_completed=7981688,extended_operations_initiated=125963,referrals_statistics=0,modify_operations_completed=0,delete_operations_initiated=0,bind_operations_completed=8115329,search_operations_completed=4385841,add_operations_completed=0,abandon_operations_completed=4,write_waiters=0,bind_operations_initiated=8115329,modrdn_operations_initiated=0,compare_operations_initiated=0,entries_statistics=4401128,read_waiters=1,current_connections=3,search_operations_initiated=4385842,modrdn_operations_completed=0,add_operations_initiated=0,total_connections=8147531,unbind_operations_initiated=7981688 1491189665000000000


Why are all the values being added as floats? None of them look to be floatish (have a decimal in them).

You're right, the syntax for the monitorCounter attribute specifies a 32-bit integer. I'll go ahead and make that change.

phemmer · 2017-04-12T12:40:13Z

plugins/inputs/openldap/openldap.go

+	TlsSkipverify bool
+	BindDn        string
+	BindPassword  string
+}


We should probably support SSL as well (meaning LDAP over SSL on port 636).

Also, what about providing a path to a custom CA cert, so that users running their own cert aren't forced to use tls_skipverify?

I didn't implement ldapS because it was never formally standardized, LDAPv3 doesn't specify anything regarding ldaps, only starttls, and go-ldap itself doesn't directly support ldaps.

I'll have to dig into the tls and x509 packages to see about adding a ca cert option.

phemmer · 2017-04-12T12:42:12Z

plugins/inputs/openldap/openldap.go

+	return nil
+}
+
+func gatherSearchResult(sr *ldap.SearchResult, o *Openldap, acc telegraf.Accumulator) error {


This function has an error in the return signature, but it doesn't ever return an error, nor is the caller checking the error.

phemmer · 2017-04-12T12:52:37Z

plugins/inputs/openldap/openldap.go

+	return nil
+}
+
+// Convert a DN to metric name, eg cn=Read,cn=Waiters,cn=Monitor to read_waiters


What are your thoughts on reversing this? So that it's waiters_read? LDAP DNs are backwards, in that they go from most specific to least specific. The advantage of reversing this is that it allows grouping when sorted.
Meaning if you have cn=Write,cn=Waiters,..., cn=Read,cn=Waiters,..., cn=Write,cn=foo,..., and cn=Read,cn=foo,..., you end up with foo_read,foo_write,waiters_read,waiters_write instead of read_foo,read_waiters,write_foo,write_waiters.

I debated having a map of DNs to field names instead. It does bother my OCD just a bit having read_waiters instead of waiters_read, but it also reads a little easier? If other plugins name their fields so they sort more naturally I can change this one to match that pattern.

…urn type altogether

…uct, and fix the sampleConfig

acobaugh · 2017-04-24T23:02:39Z

Added a tls_cacertificate option - not sure how best to do the integration test for that.
Field values are stored as ints now.
gatherSearchResults() returns nil.

I'm still a bit hesitant to support ldapS, but I will if there is consensus around it.

danielnelson · 2017-04-24T23:50:13Z

@cobaugh Do you think the config can be made to look more like some the existing tls options? Here is an example:

telegraf/plugins/inputs/mongodb/mongodb.go

Line 24 in 06baf7c

// Path to CA file

danielnelson · 2017-05-31T18:10:06Z

Is this a server setting?

phemmer · 2017-05-31T18:11:01Z

Client and server. The documentation I copied was the server side. The client side documentation just says "see server side documentation".

danielnelson · 2017-05-31T18:53:22Z

Sounds good, lets just do it like OpenLDAP.

phemmer · 2017-05-31T18:56:29Z

Do we want to do LDAPS on the first release, or just make sure that we would be able to add support in the future without breaking compatibility?

danielnelson · 2017-05-31T19:17:38Z

So long as we can add it in the future I think we are good.

acobaugh · 2017-06-25T17:42:13Z

Again, sorry to drag this out, but I'm still confused what the consensus is on this. TLS_REQCERT has to do with verifying the server certificate in the client context, and nothing to do with whether we are doing LDAPS or LDAP+StartTLS. Normally LDAPS is requested with a URI of ldaps://, and StartTLS is requested with the -ZZ option. To me, that means having a boolean called 'starttls'.

If we want to take this conversation off of this issue to avoid cluttering it up I'm fine with that too.

Fwiw, grafana has two booleans for its ldap authentication, use_ssl and start_tls:
grafana/grafana#2441

use_ssl enables ssl, and if start_tls is True, then it does a StartTLS instead of DialTLS.

danielnelson · 2017-06-26T18:50:11Z

What happens if the TLS_REQCERT option is set to demand, but the -ZZ option is not given? I think we will want to behave as closely as possible to the openldap client.

One common issue with using multiple booleans like grafana is that you could specify an invalid state combination, such as setting both start_tls and use_ssl to be true, or they could conflict with the url scheme which I think we will want to match openldap.

Don't worry having too much discussion here, this is the right place. Best to sort this out beforehand than get stuck with a config file we don't like.

acobaugh · 2017-06-26T20:10:39Z

If TLS_REQCERT is set to demand (the default), and neither -ZZ nor a URI of ldaps:// is specified, it has no effect. The only way you can request StartTLS with the cli tools with -Z(Z), and the only way you can request ldaps is with the ldaps uri. If you specify -ZZ and ldaps://, you get an error along the lines of "TLS already started". With grafana setting both se_ssl = true and start_tls = true is how you specify that you want to do StartTLS. There is no combination of those two booleans that doesn't make sense. Grafana also doesn't allow one to specify a connection url, only a hostname or IP. go-ldap also doesn't accept URLs, so that's something we would need to handle ourselves.

…

On Mon, Jun 26, 2017 at 2:50 PM, Daniel Nelson ***@***.***> wrote: What happens if the TLS_REQCERT option is set to demand, but the -ZZ option is not given? I think we will want to behave as closely as possible to the openldap client. One common issue with using multiple booleans like grafana is that you could specify an invalid state combination, such as setting both start_tls and use_ssl to be true, or they could conflict with the url scheme which I think we will want to match openldap. Don't worry having too much discussion here, this is the right place. Best to sort this out beforehand than get stuck with a config file we don't like. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG57OaNHscnibq4Kbr_fkNqjWKStyBRks5sH_1ogaJpZM4Mxof9> .

-- andy

danielnelson · 2017-06-26T21:27:00Z

The problem is that the options are mutually exclusive, since you would never do a tls connection and then issue a start_tls command. So Grafana must be giving more precedence to one of them, I assume use_ssl.

One issue with using ldap:// urls, if we do this I'm sure people will want to use all features of this url, and not understand that it is only meant to be an host + port. ldap://host:port/DN?attributes?scope?filter?extensions.

Also, what about unix sockets, are these commonly used?

How do you feel about a single option that holds either ssl/tls or start_tls?

acobaugh · 2017-06-27T00:44:07Z

Grafana's options are not mutually exclusive: https://github.com/grafana/grafana/blob/master/pkg/login/ldap.go#L65 I also don't think we should support URLs unless we're willing to parse any valid ldap url, like you suggested. Sockets (ldapi://) are used sometimes in the wild. go-ldap doesn't seem to support those either. I'm fine with a single option that takes a string."ssl" or "starttls".

…

On Mon, Jun 26, 2017 at 5:27 PM, Daniel Nelson ***@***.***> wrote: The problem is that the options are mutually exclusive, since you would never do a tls connection and then issue a start_tls command. So Grafana must be giving more precedence to one of them, I assume use_ssl. One issue with using ldap:// urls, if we do this I'm sure people will want to use all features of this url, and not understand that it is only meant to be an host + port. ldap://host:port/DN?attributes?scope?filter? extensions. Also, what about unix sockets, are these commonly used? How do you feel about a single option that holds either ssl/tls or start_tls? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG57I7ZA9i6MC1RvIUVPmOTznn4m_0Wks5sICIpgaJpZM4Mxof9> .

-- andy

danielnelson · 2017-06-27T00:58:13Z

So in Grafana, StartTLS doesn't do anything unless UseSSL is set, this seems a bit unexpected to me... the single option should resolve any confusion around this.

…xdata#2813

acobaugh · 2017-06-28T05:04:40Z

Made ssl option a string, accepting "starttls" or "ldaps". Empty string disables encryption entirely. Any other value is an error. We also support ldapS now.

Also synced up with master and switched from HasIntField to HasInt64Field due to #2813

danielnelson · 2017-07-11T22:13:09Z

Can you update the example output now that we are using integers?

acobaugh · 2017-07-14T14:32:17Z

Example output and example config updated, as well as a couple of other minor issues fixed.

danielnelson · 2017-07-21T22:50:09Z

This has been merged for 1.4, github just doesn't realize it.

acobaugh added 3 commits April 3, 2017 10:30

Add input plugin for OpenLDAP

0c2f633

Run go fmt for the openldap plugin

2106ed0

Forgot to run gdm restore/save.

0728537

acobaugh added 11 commits April 4, 2017 22:13

openldap: cannot close a connection that isn't open

0a2ff9d

openldap: initial stab at some simple unit tests

8c95bb0

openldap: get TestOpenldapNoConnection() working

507e338

openldap: add ldif file for the openldap docker container

9f58b37

openldap: split out the code that parses the search result into its o…

069be71

…wn function that we can use to mock out a search result in testing

openldap: add TestOpenldapMockResult(), put our common tests in runTe…

3f78c5c

…sts()

openldap: get the rest of the test functions working

61a38f3

openldap: go fmt

8965523

openldap: monitor backend is now part of the image

5c812b8

Makefile: add openldap docker commands

b778d8f

Add openldap container to docker-run-circle make target

de0692f

danielnelson added the triage label Apr 12, 2017

phemmer reviewed Apr 12, 2017

View reviewed changes

acobaugh added 3 commits April 12, 2017 23:13

Merge branch 'master' of https://github.com/influxdata/telegraf

5c30a1e

openldap: gatherSearchResults() always returns nil, so remove the ret…

845a0c1

…urn type altogether

openldap: all field values should be ints, not floats

feb8ff3

timhallinflux added the pm/extensibility label Apr 13, 2017

danielnelson removed the triage label Apr 13, 2017

acobaugh added 4 commits April 23, 2017 21:55

openldap: add tls_cacertificate option

de99556

openlda: set tls.Config{ServerName} to Host, add toml tags to our str…

df187bf

…uct, and fix the sampleConfig

openldap: merge local openldap branch

fc5eb36

openldap: go fmt

bb4340b

acobaugh added 7 commits June 27, 2017 23:41

openldap: expose port 636 for the openldap container

4a47c60

openldap: make ssl option a string, support ldaps

1a29eef

openldap: update README.md per recent changes to the config

a9fb95f

Merge branch 'master' of https://github.com/influxdata/telegraf

1451064

openldap: re-run gdm save

80759b0

openldap: add messages to the common tests

0e6230c

openldap: need to switch to HasInt64Field because of changes in influ…

80dd38b

…xdata#2813

acobaugh added 3 commits July 13, 2017 20:08

openldap: fix capitalization of InsecureSkipVerify and SslCa

49a27d1

openldap: update sample config

acb4392

openldap: update README sample config and sample output

a31e5f6

danielnelson pushed a commit that referenced this pull request Jul 21, 2017

Add input plugin for OpenLDAP (#2612)

1ff6e92

danielnelson closed this Jul 21, 2017

vlamug pushed a commit to vlamug/telegraf that referenced this pull request Jul 24, 2017

Add input plugin for OpenLDAP (influxdata#2612)

1390878

jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017

Add input plugin for OpenLDAP (influxdata#2612)

73e5c15

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add input plugin for OpenLDAP (#2612)

9fb8cf6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add input plugin for OpenLDAP #2612

Add input plugin for OpenLDAP #2612

acobaugh commented Apr 3, 2017

danielnelson commented Apr 3, 2017

acobaugh commented Apr 7, 2017

phemmer Apr 12, 2017

acobaugh Apr 13, 2017

phemmer Apr 12, 2017

phemmer Apr 12, 2017

acobaugh Apr 13, 2017

phemmer Apr 12, 2017

phemmer Apr 12, 2017

acobaugh Apr 13, 2017

acobaugh commented Apr 24, 2017

danielnelson commented Apr 24, 2017

danielnelson commented May 31, 2017

phemmer commented May 31, 2017

danielnelson commented May 31, 2017

phemmer commented May 31, 2017

danielnelson commented May 31, 2017

acobaugh commented Jun 25, 2017

danielnelson commented Jun 26, 2017

acobaugh commented Jun 26, 2017 via email

danielnelson commented Jun 26, 2017

acobaugh commented Jun 27, 2017 via email

danielnelson commented Jun 27, 2017

acobaugh commented Jun 28, 2017

danielnelson commented Jul 11, 2017

acobaugh commented Jul 14, 2017

danielnelson commented Jul 21, 2017

Add input plugin for OpenLDAP #2612

Add input plugin for OpenLDAP #2612

Conversation

acobaugh commented Apr 3, 2017

Required for all PRs:

danielnelson commented Apr 3, 2017

acobaugh commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acobaugh commented Apr 24, 2017

danielnelson commented Apr 24, 2017

danielnelson commented May 31, 2017

phemmer commented May 31, 2017

danielnelson commented May 31, 2017

phemmer commented May 31, 2017

danielnelson commented May 31, 2017

acobaugh commented Jun 25, 2017

danielnelson commented Jun 26, 2017

acobaugh commented Jun 26, 2017 via email

danielnelson commented Jun 26, 2017

acobaugh commented Jun 27, 2017 via email

danielnelson commented Jun 27, 2017

acobaugh commented Jun 28, 2017

danielnelson commented Jul 11, 2017

acobaugh commented Jul 14, 2017

danielnelson commented Jul 21, 2017