feat(inputs.tacacs): Add tacacs plugin for simple tacacs auth response time monitoring #12747

Hr0bar · 2023-02-27T13:42:06Z

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format

Opening PR after discussion in this Issue: #12573 and separating the tacacs plugin into a separate PR as discussed int #12736

srebhan

Thanks for contributing this plugin @Hr0bar! I have some comments, mostly the same as for the radius one. Can you please also add a unit-test?!

plugins/inputs/tacacs/sample.conf

plugins/inputs/tacacs/tacacs.go

Hr0bar · 2023-03-13T09:25:20Z

Ill try to check on this likely over the coming weeks, likely most stuff that was done for the radius plugin will be applicable here as well. Good thing the tacacs GO package seem to have some basic testing server implementation for tests and https://hub.docker.com/r/dchidell/docker-tacacs seem to be hopefully usable for integration tests.

powersj · 2023-04-04T14:16:10Z

Hi @Hr0bar,

Any further progress on this PR recently?

Thanks!

Hr0bar · 2023-04-04T16:35:10Z

Not yet, I shall have more time after April 22 (getting married, so not much free time because of preparations)

…

On Tue, Apr 4, 2023 at 4:16 PM Joshua Powers ***@***.***> wrote: Hi @Hr0bar <https://github.com/Hr0bar>, Any further progress on this PR recently? Thanks! — Reply to this email directly, view it on GitHub <#12747 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTNGZQTLMJ62UBTPX75XEDW7QULLANCNFSM6AAAAAAVJNNOIE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

powersj · 2023-04-04T17:00:42Z

Not yet, I shall have more time after April 22

Thanks

getting married, so not much free time because of preparations

Congrats! I know how busy that time can be :)

edit: I am going to mark this as a draft until you do another push and ping us to review again.

plugins/inputs/tacacs/README.md

plugins/inputs/tacacs/tacacs.go

Hr0bar · 2023-05-02T18:52:11Z

Applied most of the comments. However:

Tried using the status code returned, but it was very useless as the status depends on the current tacacs auth sequence. On initial start packet, return code X may mean something completely different than return code X for send username packet, then X may mean something else on send password packet etc. So without the secondary info at what stage the status code was returned, its useless.
In the radius plugin, the status code is always from the same place - its just one request to do the auth, not multiple, so its nicely descriptive with exact string representation of the status. Here its not the case, you get an integer code that may mean something else for every auth stage.

Anyway, its meant to monitor SUCCESSFUL authentications response time, so decided to simply return an error in all other cases. Let me know if that makes sense ?

Deleted responsetime, kept only responsetime_ms, same as in the radius plugin.

"Maybe also check for other empty required fields?" - mandatory fields are uncommented in the sample conf. If not set, I think these should catch it:

	username, err := t.Username.Get()
	if err != nil {
		return fmt.Errorf("getting username failed: %v", err)
	}

The other non mandatory are now defaults:
return &Tacacs{RemAddr: "127.0.0.1", ResponseTimeout: config.Duration(time.Second * 5)}

	if len(t.Servers) == 0 {
		t.Servers = []string{"127.0.0.1:49"}
	}

I could not move the client initialization to Init(), as the client itself contains the secret, it would be unnecessary stored in memory without defer releasesecret, or it gets cleaned from memory and secret is then broken for connections. Maybe there is a solution, but seems like unnecessary struggle (tried once, definitely can be avoided, just did not spent time on it, let me know if its a must to have it in Init).

Hr0bar · 2023-05-02T19:00:07Z

BTW the "Git repo dirty. Please run "make docs" and push the updated README. Failing CI." in "ci/circleci: test-go-linux" is likely caused by me changing README.MD -> README.md filename in GIT some time ago in this PR. Ran make docs in the past after that, but still pops up

plugins/inputs/tacacs/tacacs.go

srebhan · 2023-05-09T18:40:48Z

Hey @Hr0bar,

regarding

BTW the "Git repo dirty. Please run "make docs" and push the updated README. Failing CI." in "ci/circleci: test-go-linux" is likely caused by me changing README.MD -> README.md filename in GIT some time ago in this PR. Ran make docs in the past after that, but still pops up

running make docs should solve the issue. It's likely due to the sample.conf content doesn't match the README.md Configuration section content...

plugins/inputs/tacacs/tacacs.go

srebhan

@Hr0bar thanks for the update. Just a few minor comments...

plugins/inputs/tacacs/README.md

plugins/inputs/tacacs/tacacs.go

plugins/inputs/tacacs/tacacs_test.go

plugins/inputs/tacacs/tacacs.go

Hr0bar · 2023-06-01T09:30:06Z

Thanks, Ill check these comments when Ill be back from travelling ~mid June. Or feel free to adjust it until then.

…

On Wed, May 17, 2023 at 3:44 PM Sven Rebhan ***@***.***> wrote: ***@***.**** commented on this pull request. @Hr0bar <https://github.com/Hr0bar> thanks for the update. Just a few minor comments... ------------------------------ In plugins/inputs/tacacs/README.md <#12747 (comment)> : > @@ -0,0 +1,49 @@ +# Tacacs Input Plugin + +The Tacacs plugin collects successful tacacs authentication response times. Is there an implementation/software we can or should reference? ------------------------------ In plugins/inputs/tacacs/tacacs.go <#12747 (comment)> : > + "sync" + "time" + + "github.com/nwaples/tacplus" + + "github.com/influxdata/telegraf" + "github.com/influxdata/telegraf/config" + "github.com/influxdata/telegraf/plugins/inputs" +) + +type Tacacs struct { + Servers []string `toml:"servers"` + Username config.Secret `toml:"username"` + Password config.Secret `toml:"password"` + Secret config.Secret `toml:"secret"` + RemAddr string `toml:"request_ip"` Can we please use a more speaking name for the variable!? ⬇️ Suggested change - RemAddr string `toml:"request_ip"` + RequestAddr string `toml:"request_ip"` ------------------------------ In plugins/inputs/tacacs/tacacs.go <#12747 (comment)> : > + + "github.com/influxdata/telegraf" + "github.com/influxdata/telegraf/config" + "github.com/influxdata/telegraf/plugins/inputs" +) + +type Tacacs struct { + Servers []string `toml:"servers"` + Username config.Secret `toml:"username"` + Password config.Secret `toml:"password"` + Secret config.Secret `toml:"secret"` + RemAddr string `toml:"request_ip"` + ResponseTimeout config.Duration `toml:"response_timeout"` + Log telegraf.Logger `toml:"-"` + clients []tacplus.Client + testAuthStart tacplus.AuthenStart Why does it have a test prefix? Can we simply name it authStart or auth or similar? ------------------------------ In plugins/inputs/tacacs/tacacs.go <#12747 (comment)> : > + if net.ParseIP(t.RemAddr) == nil { + return fmt.Errorf("Invalid ip address provided for request_ip: %s", t.RemAddr) + } Your sample.conf says the default is 127.0.0.1 and I think that should be set here (instead of in init()) if empty. Furthermore, errors should start with lowercase letters in Golang... ------------------------------ In plugins/inputs/tacacs/tacacs.go <#12747 (comment)> : > + for index, serverValue := range t.Servers { + t.clients = append(t.clients, tacplus.Client{}) + t.clients[index].Addr = serverValue + t.clients[index].ConnConfig = tacplus.ConnConfig{} + } Maybe ⬇️ Suggested change - for index, serverValue := range t.Servers { - t.clients = append(t.clients, tacplus.Client{}) - t.clients[index].Addr = serverValue - t.clients[index].ConnConfig = tacplus.ConnConfig{} - } + t.clients = make([]tacplug.Client, 0, len(t.Servers)) + for _, server := range t.Servers { + t.clients = append(t.clients, tacplus.Client{ + Addr: server, + ConnConfig: tacplus.ConnConfig{}, + }) + } ------------------------------ In plugins/inputs/tacacs/tacacs.go <#12747 (comment)> : > + // Check the returned status + if reply.Status != tacplus.AuthenStatusGetUser { + return fmt.Errorf("error on new tacacs authentication start request to %s : Unexpected response code %d", client.Addr, reply.Status) + } I don't remember what we did on radius, but it might make sense to have this in the metric to make it discoverable to monitoring.... ------------------------------ In plugins/inputs/tacacs/tacacs_test.go <#12747 (comment)> : > + "github.com/influxdata/telegraf/config" + "github.com/influxdata/telegraf/testutil" + "github.com/nwaples/tacplus" + "github.com/stretchr/testify/require" + "github.com/testcontainers/testcontainers-go/wait" ⬇️ Suggested change - "github.com/influxdata/telegraf/config" - "github.com/influxdata/telegraf/testutil" - "github.com/nwaples/tacplus" - "github.com/stretchr/testify/require" - "github.com/testcontainers/testcontainers-go/wait" + "github.com/nwaples/tacplus" + "github.com/stretchr/testify/require" + "github.com/testcontainers/testcontainers-go/wait" + + "github.com/influxdata/telegraf/config" + "github.com/influxdata/telegraf/testutil" ------------------------------ In plugins/inputs/tacacs/tacacs.go <#12747 (comment)> : > + ResponseTimeout config.Duration `toml:"response_timeout"` + Log telegraf.Logger `toml:"-"` +} + +//go:embed sample.conf +var sampleConfig string + +func (t *Tacacs) SampleConfig() string { + return sampleConfig +} + +func (t *Tacacs) Init() error { + if len(t.Servers) == 0 { + t.Servers = []string{"127.0.0.1"} + } + return nil I do agree, it might be worth checking the options in Init() to fail early and loudly! — Reply to this email directly, view it on GitHub <#12747 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTNGZU6KUCQPZGW4YF7TYTXGTI5JANCNFSM6AAAAAAVJNNOIE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

srebhan · 2023-06-02T10:34:42Z

Ok. It's a pitty as we will likely miss v1.27.0 which is planned for June 12th... :-(

srebhan · 2023-07-04T13:30:43Z

@Hr0bar any chance you work on this PR?

Hr0bar · 2023-07-10T14:37:59Z

I added all the changes, BUT the one regarding returning the return status in case of failures likely needs more thought. I added the response_code tag, and returning timeout value for response time, same as in the other plugin. That is all OK, but it works for wrong password, wrong username etc, but it doesnt cover for example wrong tacacs secret. In that case client.SendAuthenStart returns hard error, and no response code is returned. I think it should be consistent for the user, so as I commented on that suggestion already, what about creating the response codes completely custom and human readable for telegraf purposes? Instead of the hard errors there were previously.

Hr0bar · 2023-07-10T14:58:19Z

So either:

restore parts of code 4387d9f from before today for cases where wrong password etc is used, that return hard error on everything that is not success
keep the current weird mix where we return the response code in tag, but it doesnt apply to wrong secret and possibly other cases, because in that case tacacs servers dont return any response and there is hard error on connection instead.
Implement custom response codes, that provide all info to the user (including real response codes if provided by tacacs server), that cover also wrong tacacs secret and other cases. Use timeout time for response time in case of those failures (same as in the other plugin)

srebhan

Thanks @Hr0bar for the update! The code looks good to me and to me the current response-code strategy also seems good. IMO we error out if the sending reports an error. If the sending succeeds but the server reports some unexpected state we pass that information on to the user...

The only comment I have is that currently we do not report the (context/connection) timeout to the user. I think you should make an exception there when checking the error codes and also generate a metric for this case. This way an operator can see that Telegraf cannot reach the server and check what is going on...

plugins/inputs/tacacs/sample.conf

plugins/inputs/tacacs/tacacs_test.go

plugins/inputs/tacacs/README.md

Hr0bar · 2023-07-25T05:52:59Z

Let me know, if the tests are readable/by telegraf standards enough, or if additional refactoring is needed

Hipska

Yup, looks good. Just a few minor things..

plugins/inputs/tacacs/tacacs.go

plugins/inputs/tacacs/tacacs_test.go

srebhan

Looks good to me. Thanks for contributing this plugin @Hr0bar!

powersj

One question inline to help me understand the reported value for response time.

powersj · 2023-08-01T13:57:37Z

plugins/inputs/tacacs/tacacs.go

+		if !errors.Is(err, context.DeadlineExceeded) {
+			return fmt.Errorf("error on tacacs authentication continue username request to %s : %w", client.Addr, err)
+		}
+		fields["responsetime_ms"] = time.Duration(t.ResponseTimeout).Milliseconds()


I realize this code path means there was a timeout, but why is the response time set to the timeout value and not the real time?

Same question below, where it would be even more important to see how long the cumulative request time took.

I think that is a valid point, especially for users who would set the timeout to zero. In other cases, it will be close to the configured timeout value anyway. With zero timeout configured it can theoretically also timeout from other sources than the configured context deadline (from some network functions called down the line). Maybe in addition to context.DealineExceeded error we should also allow errors.Is(err, os.ErrDeadlineExceeded) for these cases.

While the used tacacs package seems to be correctly setting the timeout to the deeper used network calls, I could not deterministically say that it will always return only context.DeadlineExceeded error on timeout.

It seemd os.IsTimeout() would be better here, as it returns True for both context.DeadlineExceeded and os.ErrDeadlineExceeded and possibly other timeout time errors, but after testing it it was too lenient/broad and it was returning True even for immediate dial tcp or dns errors etc so will keep
if !errors.Is(err, context.DeadlineExceeded) && !errors.Is(err, os.ErrDeadlineExceeded)

The timeout time returned is now real value.

telegraf-tiger · 2023-08-07T17:02:11Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

powersj

Thank you for the effort and work on this!

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 27, 2023

srebhan requested changes Mar 7, 2023

View reviewed changes

srebhan self-assigned this Mar 7, 2023

srebhan added the new plugin label Mar 7, 2023

powersj added the waiting for response waiting for response from contributor label Apr 4, 2023

telegraf-tiger bot removed the waiting for response waiting for response from contributor label Apr 4, 2023

powersj marked this pull request as draft April 4, 2023 17:01

Hipska suggested changes Apr 5, 2023

View reviewed changes

Hr0bar marked this pull request as ready for review May 2, 2023 20:31

Hipska suggested changes May 4, 2023

View reviewed changes

plugins/inputs/tacacs/tacacs.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs.go Show resolved Hide resolved

plugins/inputs/tacacs/tacacs.go Show resolved Hide resolved

Hipska reviewed May 11, 2023

View reviewed changes

plugins/inputs/tacacs/tacacs.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs.go Show resolved Hide resolved

srebhan reviewed May 17, 2023

View reviewed changes

Hipska added the waiting for response waiting for response from contributor label Jul 4, 2023

telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jul 10, 2023

srebhan closed this Jul 12, 2023

srebhan reopened this Jul 12, 2023

srebhan reviewed Jul 12, 2023

View reviewed changes

plugins/inputs/tacacs/sample.conf Outdated Show resolved Hide resolved

Hr0bar added 7 commits July 12, 2023 20:47

Only pass references to clients

aadf57f

PR comments changes

0a0a2db

Remove trailing whitespace

5d08da0

Correct tests for new response codes

debaff4

PR comments changes

2fb6ee8

Update tacacs_test.go

bc824c1

Remove unused condition

daac089

Hipska added the waiting for response waiting for response from contributor label Jul 12, 2023

Separate Init() tests

8da4827

Hipska removed the waiting for response waiting for response from contributor label Jul 13, 2023

Hipska suggested changes Jul 13, 2023

View reviewed changes

plugins/inputs/tacacs/tacacs_test.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs_test.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs_test.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs_test.go Outdated Show resolved Hide resolved

Hr0bar added 2 commits July 13, 2023 12:42

response_code convert to field, separate Init() tests

3f5c533

Return human readable string representation of a return code

d30aa38

Hipska suggested changes Jul 13, 2023

View reviewed changes

plugins/inputs/tacacs/README.md Outdated Show resolved Hide resolved

plugins/inputs/tacacs/README.md Outdated Show resolved Hide resolved

plugins/inputs/tacacs/README.md Outdated Show resolved Hide resolved

Make tests more readable & misc PR suggestions

45d7de2

Hipska suggested changes Jul 31, 2023

View reviewed changes

plugins/inputs/tacacs/tacacs.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs.go Outdated Show resolved Hide resolved

plugins/inputs/tacacs/tacacs_test.go Outdated Show resolved Hide resolved

latest PR comments

c3daaea

Hipska approved these changes Aug 1, 2023

View reviewed changes

Hipska added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 1, 2023

srebhan approved these changes Aug 1, 2023

View reviewed changes

srebhan assigned powersj and unassigned srebhan Aug 1, 2023

powersj reviewed Aug 1, 2023

View reviewed changes

Change timeout errors/behaviour to use real values

2fdb9f9

Hipska approved these changes Aug 7, 2023

View reviewed changes

powersj approved these changes Aug 7, 2023

View reviewed changes

powersj merged commit dae1158 into influxdata:master Aug 7, 2023

github-actions bot added this to the v1.28.0 milestone Aug 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.tacacs): Add tacacs plugin for simple tacacs auth response time monitoring #12747

feat(inputs.tacacs): Add tacacs plugin for simple tacacs auth response time monitoring #12747

Hr0bar commented Feb 27, 2023 •

edited

Loading

srebhan left a comment

Hr0bar commented Mar 13, 2023

powersj commented Apr 4, 2023

Hr0bar commented Apr 4, 2023 via email

powersj commented Apr 4, 2023 •

edited

Loading

Hr0bar commented May 2, 2023

Hr0bar commented May 2, 2023

srebhan commented May 9, 2023

srebhan left a comment

Hr0bar commented Jun 1, 2023 via email

srebhan commented Jun 2, 2023

srebhan commented Jul 4, 2023

Hr0bar commented Jul 10, 2023

Hr0bar commented Jul 10, 2023

srebhan left a comment

Hr0bar commented Jul 25, 2023

Hipska left a comment

srebhan left a comment

powersj left a comment

powersj Aug 1, 2023

Hr0bar Aug 1, 2023

Hr0bar Aug 7, 2023 •

edited

Loading

telegraf-tiger bot commented Aug 7, 2023

Artifact URLs

powersj left a comment

feat(inputs.tacacs): Add tacacs plugin for simple tacacs auth response time monitoring #12747

feat(inputs.tacacs): Add tacacs plugin for simple tacacs auth response time monitoring #12747

Conversation

Hr0bar commented Feb 27, 2023 • edited Loading

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Mar 13, 2023

powersj commented Apr 4, 2023

Hr0bar commented Apr 4, 2023 via email

powersj commented Apr 4, 2023 • edited Loading

Hr0bar commented May 2, 2023

Hr0bar commented May 2, 2023

srebhan commented May 9, 2023

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Jun 1, 2023 via email

srebhan commented Jun 2, 2023

srebhan commented Jul 4, 2023

Hr0bar commented Jul 10, 2023

Hr0bar commented Jul 10, 2023

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Jul 25, 2023

Hipska left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

powersj left a comment

Choose a reason for hiding this comment

powersj Aug 1, 2023

Choose a reason for hiding this comment

Hr0bar Aug 1, 2023

Choose a reason for hiding this comment

Hr0bar Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

telegraf-tiger bot commented Aug 7, 2023

Artifact URLs

powersj left a comment

Choose a reason for hiding this comment

Hr0bar commented Feb 27, 2023 •

edited

Loading

powersj commented Apr 4, 2023 •

edited

Loading

Hr0bar Aug 7, 2023 •

edited

Loading