feat(inputs.radius): Add radius plugin for simple radius auth response time monitoring #12736

Hr0bar · 2023-02-24T12:40:27Z

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

Summary: In our org we build custom telegraf binary, with added tacacs&radius simple auth response time monitoring plugins, this PR explores discussed possibility of merging it upstream.

srebhan

Thanks @Hr0bar for this awesome contribution!!! I have a few comments in the code. Besides from this, I'd like to ask you to split this into two PRs, one for each plugin. Furthermore, we need some testing...
Focusing on the RADIUS plugin for now, is there a way to test this against a docker container like FreeRadius? If so you could use code similar to this...

plugins/inputs/radius/radius.go

plugins/inputs/radius/README.MD

plugins/inputs/radius/radius.go

Hr0bar · 2023-02-25T14:10:57Z

Added the mentioned changes.

Should I close this PR to open new ones for each plugin separately?

Seems both upstream tacplus and radius go packages have some sort of testing servers implementations or examples in them, could be perhaps used for the tests (gotta check whats allowed by telegraf test environment - if I can temporarily listen on some ports etc, or if the container approach is required)

And a sidenote, im not the original author in our org, so no need to thank me, Im just trying to get it merged :)

srebhan · 2023-02-27T11:48:29Z

@Hr0bar the best is to keep the Radius part in this PR (change description, title etc) and open a new PR for the tacacs plugin. Furthermore, I take the liberty to thank you anyway for contributing those missing pieces and like to ask you to also thank your co-worker for the implementation! :-)

srebhan

Very nice update @Hr0bar! For the Radius plugin part there are only a few small comments and the request to add test(s). Best would be to base them on the server example of the library and, if possible, add an integration test against a docker container. If you need help, please let me know!

plugins/inputs/radius/radius.go

plugins/inputs/radius/sample.conf

plugins/inputs/radius/radius.go

Hr0bar · 2023-02-27T15:11:30Z

Ill have a look at creating the tests hopefully in near future. Meanwhile the tacacs part is split to separate PR now. Latest comments reflected in the code.

srebhan

Very close. Some more small comments. Looking forward to the test. Hope to get this in for v1.26...

plugins/inputs/radius/radius.go

Hr0bar · 2023-02-28T15:51:41Z

Added the response_code as a tag.

btw thrown together some tests just now as a skeleton to start, doesnt work at all yet, im not sure if its even possible to listen on port during testing in circleci? Or is local temporary file socket needed instead? Gotta have a look at the testing packages/methods provided by telegraf I guess. The container needs config via mountpoint, the mariadb example should be helpful there. Is the container found&downloaded automagically if its present on docker hub or it needs to be enabled somewhere?


import (
	"context"
	_ "embed"
	"testing"
	"time"

	"github.com/docker/go-connections/nat"
	"github.com/influxdata/telegraf/config"
	"github.com/influxdata/telegraf/testutil"
	"github.com/stretchr/testify/require"
	"github.com/testcontainers/testcontainers-go/wait"
	"layeh.com/radius"
	"layeh.com/radius/rfc2865"
)

func TestRadiusLocal(t *testing.T) {

	handler := func(w radius.ResponseWriter, r *radius.Request) {
		username := rfc2865.UserName_GetString(r.Packet)
		password := rfc2865.UserPassword_GetString(r.Packet)

		var code radius.Code
		if username == "testusername" && password == "testpassword" {
			code = radius.CodeAccessAccept
		} else {
			code = radius.CodeAccessReject
		}
		w.Write(r.Response(code))
	}

	server := radius.PacketServer{
		Handler:      radius.HandlerFunc(handler),
		SecretSource: radius.StaticSecretSource([]byte(`testsecret`)),
	}

	if err := server.ListenAndServe(); err != nil {
		require.NoError(t, err, "failed to start local radius server")
	}

	plugin := &Radius{
		Servers:  []string{"localhost:1812"},
		Username: config.NewSecret([]byte(`testusername`)),
		Password: config.NewSecret([]byte(`testpassword`)),
		Secret:   config.NewSecret([]byte(`testsecret`)),
	}
	var acc testutil.Accumulator

	require.NoError(t, plugin.Init())
	require.NoError(t, plugin.Gather(&acc))
	require.Len(t, acc.Errors, 0)
	if !acc.HasMeasurement("radius") {
		t.Errorf("acc.HasMeasurement: expected radius")
	}
	require.Equal(t, acc.HasTag("radius", "source"), true)
	require.Equal(t, acc.HasTag("radius", "source_port"), true)
	require.Equal(t, acc.HasTag("radius", "response_code"), true)
	require.Equal(t, acc.TagValue("radius", "source"), "localhost")
	require.Equal(t, acc.TagValue("radius", "source_port"), "1812")
	require.Equal(t, acc.TagValue("radius", "response_code"), radius.CodeAccessAccept.String())
	require.Equal(t, acc.HasInt64Field("radius", "responsetime_ms"), true)

	server.Shutdown(context.Background())
}

func TestRadiusIntegration(t *testing.T) {
	if testing.Short() {
		t.Skip("Skipping integration test in short mode")
	}

	port := "1812"

	// TODO mount filesystem with testing freeradius config with test user

	container := testutil.Container{
		Image:        "freeradius/freeradius-server",
		ExposedPorts: []string{port},
		WaitingFor: wait.ForAll(
			wait.ForListeningPort(nat.Port(port)),
		),
	}
	err := container.Start()
	require.NoError(t, err, "failed to start container")
	defer container.Terminate()

	// Define the testset
	var testset = []struct {
		name                 string
		testing_timeout      config.Duration
		expected_source      string
		expected_source_port string
	}{
		{
			name:                 "timeout_5s",
			testing_timeout:      config.Duration(time.Second * 5),
			expected_source:      container.Address,
			expected_source_port: port,
		},
		{
			name:                 "timeout_0s",
			testing_timeout:      config.Duration(0),
			expected_source:      container.Address,
			expected_source_port: port,
		},
	}

	for _, tt := range testset {
		t.Run(tt.name, func(t *testing.T) {
			// Setup the plugin-under-test
			plugin := &Radius{
				ResponseTimeout: tt.testing_timeout,
				Servers:         []string{"localhost:1812", "unreachable.unreachable.com:1812"},
				Username:        config.NewSecret([]byte(`testusername`)),
				Password:        config.NewSecret([]byte(`testpassword`)),
				Secret:          config.NewSecret([]byte(`testsecret`)),
			}
			var acc testutil.Accumulator

			// Startup the plugin
			require.NoError(t, plugin.Init())

			// Gather
			require.NoError(t, plugin.Gather(&acc))
			require.Len(t, acc.Errors, 0)

			// Do the tests that are in simple local test
			// TODO check HasInt64Field  for responsetime_ms
			// TODO check timeout 0 behavior
			// TODO check that responsetime_ms is under 5 sec for reachable test
			// TODO check that responsetime_ms is equal to timeout for unreachable test
			// TODO create third check with wrong user password and check correct response code
			if !acc.HasMeasurement("radius") {
				t.Errorf("acc.HasMeasurement: expected radius")
			}
		})
	}
}

srebhan · 2023-02-28T16:18:57Z

@Hr0bar you can listen on a port and the tests look good at a first glance. One request would be to use a dynamic port so prevent later (parallel) tests to interfere because they are using the same port...

srebhan · 2023-03-03T09:15:32Z

@Hr0bar can you please add the test above to your PR!?! For the TODO in the test (regarding mounting) you can take a look at the sql input plugin's test it does this...

Hr0bar · 2023-03-06T13:06:35Z

@Hr0bar can you please add the test above to your PR!?! For the TODO in the test (regarding mounting) you can take a look at the sql input plugin's test it does this...

Added draft version of the tests, I could not run them locally due to problems/bad setup on my Windows machine, so I expect them needing more work

srebhan

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

This reverts commit c483b4b.

powersj · 2023-03-06T19:08:52Z

There were conflicts, so I have rebased you on master. I will wait for tests.

powersj · 2023-03-06T21:03:11Z

The radius integration test is the thing that is failing currently:

container failed to start: /bin/sh command not executable: failed to start container

This usually means that the container is missing something to start up or some option was omitted.

@Hr0bar are you able to reproduce locally?

Hr0bar · 2023-03-06T21:22:08Z

@powersj I dont have setup for testing the containers right now.

There was one run with what I think was the same config as right now, that started OK:
https://app.circleci.com/pipelines/github/influxdata/telegraf/15339/workflows/100f9c88-62d4-4457-a81a-cbf391d3e199/jobs/241223

2023/03/06 16:23:36 Starting container id: 5a745b38fb8f image: freeradius/freeradius-server
2023/03/06 16:23:36 Waiting for container id 5a745b38fb8f image: freeradius/freeradius-server
2023/03/06 16:23:36 Container is ready id: 5a745b38fb8f image: freeradius/freeradius-server
mapped container port '1812' to host port '32821'

So seems non-deterministic. First guess is that some additional wait condition is needed to wait for certain container log output before we issue the Start(), Ill likely need to spin up local docker to see whats going on inside.

Hr0bar · 2023-03-07T11:15:28Z

Im afraid Im lost, tried it locally after setting powerful enough test env for Integration tests, tried with different setups but it seems to be failing because of poor support for UDP by the testcontainer tool.

Looking at the testcontainer project repo it has too many open PRs / Issues regarding UDP exposing + dynamic mapping waiting to be resolved for some years now.

Unless I find a working example how to achieve fully functioning UDP testcontainer with dynamic port mapping I think we will need to stick to the simple test with local Radius server, and not use full container Integration test.

powersj · 2023-03-07T18:18:58Z

Unless I find a working example how to achieve fully functioning UDP testcontainer with dynamic port mapping I think we will need to stick to the simple test with local Radius server, and not use full container Integration test.

I think I'm good with the switching to a generic server. @srebhan thoughts?

Hr0bar · 2023-03-07T18:43:07Z

Its possible I may be missing something obvious though, that I couldnt get to the container with UDP.

powersj · 2023-03-07T19:04:42Z

Its possible I may be missing something obvious though, that I couldnt get to the container with UDP.

I don't think you are :( I was reproducing this locally as well yesterday. After playing with the container itself and config I was also running into issues.

srebhan · 2023-03-08T21:10:37Z

@Hr0bar I do have massive issues to setup the container with the configs by hand (without Telegraf being involved) to make a radtest succeed... Does this work for you? If so, can you please provide an example to me (maybe on Slack) to teach me how to setup the container?

Hr0bar · 2023-03-08T21:17:38Z

@srebhan I just tried something 1 minute ago and seems I got successful tests with the container. Im going to sleep now but I expect to commit the changes tomorrow morning.

In short, the issue seems to be in here in telegraf wrapper around testcontainers:

If I comment those lines I get the UDP working fine (but I already have some other changes as well so no idea if it works with the current PR) and just received all tests passing on my test box. But I need to confirm tomorrow.

Hr0bar · 2023-03-08T22:21:54Z

Seems everything is passing now, except telegraf internal tests regarding the port mapping behavior. Nothing else seems impacted. The extra wrapper that is removing the proto part from ports needs to be checked why it was done like that and the below test adjusted likely.

--- Container Logs End ---
2023/03/08 22:11:57 Starting container id: d2334d6225ec image: nginx:stable-alpine
2023/03/08 22:11:57 Container is ready id: d2334d6225ec image: nginx:stable-alpine
mapped container port "80/tcp" to host port "80"
--- FAIL: TestMappedPortLookupIntegration (2.84s)
    container_test.go:53: 
        	Error Trace:	/home/circleci/project/testutil/container_test.go:53
        	Error:      	Not equal: 
        	            	expected: "80"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-80
        	            	+
        	Test:       	TestMappedPortLookupIntegration
FAIL

The logline:
fmt.Printf("mapped container port %q to host port %q\n", port, p.Port())

could likely include the protocol part - p.Proto() so its clear in the logs whats going on

srebhan · 2023-03-09T11:01:16Z

@Hr0bar I took the liberty to fix the test-container issue in your branch. Hope that is ok for you!?

srebhan

Just one more suggestion... :-)

plugins/inputs/radius/radius.go

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

telegraf-tiger · 2023-03-09T15:40:52Z

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 taking the time to put up this PR and work through the integration tests!

srebhan

This is awesome! Thanks for this great contribution @Hr0bar!

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Feb 24, 2023

srebhan requested changes Feb 24, 2023

View reviewed changes

srebhan added new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 24, 2023

srebhan self-assigned this Feb 24, 2023

srebhan requested changes Feb 27, 2023

View reviewed changes

Hr0bar changed the title ~~feat: add input plugins for simple tacacs and radius auth response time monitoring~~ feat(inputs.radius): Add radius plugin for simple radius auth response time monitoring Feb 27, 2023

Hr0bar mentioned this pull request Feb 27, 2023

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

Merged

3 tasks

srebhan reviewed Feb 27, 2023

View reviewed changes

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

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

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

srebhan reviewed Feb 28, 2023

View reviewed changes

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

srebhan approved these changes Mar 6, 2023

View reviewed changes

srebhan assigned powersj and unassigned srebhan Mar 6, 2023

srebhan 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 Mar 6, 2023

Hr0bar and others added 9 commits March 6, 2023 12:07

Initial add of radius and tacacs plugin

1eb268a

Fix whitespace

33478a4

Compliancy with telegraf repo

78d738e

Update LICENSE_OF_DEPENDENCIES.md

2c404ca

Update README.MD

dec6085

Revert "Update README.MD"

a0a58cf

This reverts commit c483b4b.

Fix leftover

c330fa5

Clarify examples

f4be2c7

Changes according to PR comments

a29a8a9

Hr0bar added 5 commits March 6, 2023 12:07

Fix ListenAndServe is blocking and nil Logger panic

c565cd0

Fix lint warns/errs

07c2d57

Dont report ErrServerShutdown as test error

c6f4448

Dont report ErrServerShutdown as test error

5ec2544

Fix container port, fix order of expected/actual results

a21cbdd

go mod tidy

bb4a51b

Explicitly specify UDP for test container expose

141f8e9

Attempt at fixing UDP connectivity in integration tests

9d20f19

srebhan added 3 commits March 9, 2023 11:36

Fix testcontainer issue

aedfdfa

Fix testcase for testcontainer expectations

f4da475

Fix testcase for testcontainer expectations

792a10e

srebhan reviewed Mar 9, 2023

View reviewed changes

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

powersj and others added 2 commits March 9, 2023 07:42

Update plugins/inputs/radius/radius.go

3742e64

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

import errors

1cf9520

powersj approved these changes Mar 9, 2023

View reviewed changes

srebhan approved these changes Mar 9, 2023

View reviewed changes

powersj merged commit 97fd189 into influxdata:master Mar 9, 2023

srebhan added this to the v1.26.0 milestone Jun 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.radius): Add radius plugin for simple radius auth response time monitoring #12736

feat(inputs.radius): Add radius plugin for simple radius auth response time monitoring #12736

Hr0bar commented Feb 24, 2023

srebhan left a comment

Hr0bar commented Feb 25, 2023

srebhan commented Feb 27, 2023

srebhan left a comment

Hr0bar commented Feb 27, 2023

srebhan left a comment

Hr0bar commented Feb 28, 2023

srebhan commented Feb 28, 2023

srebhan commented Mar 3, 2023

Hr0bar commented Mar 6, 2023

srebhan left a comment

powersj commented Mar 6, 2023

powersj commented Mar 6, 2023

Hr0bar commented Mar 6, 2023 •

edited

Hr0bar commented Mar 7, 2023

powersj commented Mar 7, 2023

Hr0bar commented Mar 7, 2023

powersj commented Mar 7, 2023

srebhan commented Mar 8, 2023

Hr0bar commented Mar 8, 2023

Hr0bar commented Mar 8, 2023

srebhan commented Mar 9, 2023

srebhan left a comment

telegraf-tiger bot commented Mar 9, 2023

Artifact URLs

powersj left a comment

srebhan left a comment

feat(inputs.radius): Add radius plugin for simple radius auth response time monitoring #12736

feat(inputs.radius): Add radius plugin for simple radius auth response time monitoring #12736

Conversation

Hr0bar commented Feb 24, 2023

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Feb 25, 2023

srebhan commented Feb 27, 2023

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Feb 27, 2023

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Feb 28, 2023

srebhan commented Feb 28, 2023

srebhan commented Mar 3, 2023

Hr0bar commented Mar 6, 2023

srebhan left a comment

Choose a reason for hiding this comment

powersj commented Mar 6, 2023

powersj commented Mar 6, 2023

Hr0bar commented Mar 6, 2023 • edited

Hr0bar commented Mar 7, 2023

powersj commented Mar 7, 2023

Hr0bar commented Mar 7, 2023

powersj commented Mar 7, 2023

srebhan commented Mar 8, 2023

Hr0bar commented Mar 8, 2023

Hr0bar commented Mar 8, 2023

srebhan commented Mar 9, 2023

srebhan left a comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Mar 9, 2023

Artifact URLs

powersj left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

Hr0bar commented Mar 6, 2023 •

edited