Implement Asynchrounous Requests via UDP/TCP by mriemensberger · Pull Request #93 · radcli/radcli

mriemensberger · 2023-02-10T01:03:19Z

Radius clients, for example, splash portal applications, frequently perform
lots of networking-related tasks like serving HTTP. Those applications are
often written using an event loop (based on select, poll, etc.) and non-blocking
async processing. In order to integrate radcli into such an event loop
architecture it also needs to support non-blocking async radius exchanges.

Implements async requests by splitting the original send_server_ctx()
function and its wrappers into multiple pieces that are driven by a
simple state machine and events on the associated socket. The
rc_async_handle represents on async request, holds the state
and the result when the request is done.
Stores the state for each request in a separate handle.
Adds async handling functions in sendserver_async.c
Implements parallel handling of multiple async requests in multihandle.c
with direct support for select and poll event loop structures.

Caveats:

Only UDP and TCP are supported (no TLD/DTLS)
Automatic retries are not supported; Users can retry by creating a new
handle and retry the request using the new handle.
Only one configured server is supported. rc_async_prepare_handle can
be used to specify different servers for a different handle with
otherwise the same request data.

Moves code needed by both sendserver.c and sendserver_async.c to a separate file. Adds an associated header.

Radius clients, for example, splash portal applications, frequently perform lots of networking-related tasks like serving HTTP. Those applications are often written using an event loop (based on select, poll, etc.) and non-blocking async processing. In order to integrate radcli into such an event loop architecture it also needs to support non-blocking async radius exchanges. Implements async requests by splitting the original send_server_ctx() function and its wrappers into multiple pieces that are driven by a simple state machine and events on the associated socket. The rc_async_handle represents on async request, holds the state and the result when the request is done. Caveats: - Only UDP and TCP are supported (no TLD/DTLS) - Automatic retries are not supported; Users can retry by creating a new handle and retry the request using the new handle. - Only one configured server is supported. rc_async_prepare_handle can be used to specify different servers for a different handle with otherwise the same request data.

…quests Implements parallel handling of multiple async requests in multihandle.c with direct support for select and poll event loop structures. This change was co-authored by Philipp Stanner.

mriemensberger · 2023-03-13T08:20:26Z

lib/sendserver_util.c

+		switch (vp->attribute) {
+		case PW_USER_PASSWORD:
+
+			/* Encrypt the password */
+
+			/* Chop off password at AUTH_PASS_LEN */
+			length = vp->lvalue;
+			if (length > AUTH_PASS_LEN)
+				length = AUTH_PASS_LEN;
+
+			/* Calculate the padded length */
+			padded_length =
+			    (length +
+			     (AUTH_VECTOR_LEN - 1)) & ~(AUTH_VECTOR_LEN - 1);
+
+			/* Record the attribute length */
+			*buf++ = padded_length + 2;
+			if (vsa_length_ptr != NULL)
+				*vsa_length_ptr += padded_length + 2;
+
+			/* Pad the password with zeros */
+			memset((char *)passbuf, '\0', AUTH_PASS_LEN);
+			memcpy((char *)passbuf, vp->strvalue, (size_t) length);
+
+			secretlen = strlen(secret);
+			vector = (unsigned char *)auth->vector;
+			for (i = 0; i < padded_length; i += AUTH_VECTOR_LEN) {
+				/* Calculate the MD5 digest */
+				strcpy((char *)md5buf, secret);
+				memcpy((char *)md5buf + secretlen, vector,
+				       AUTH_VECTOR_LEN);
+				rc_md5_calc(buf, md5buf,
+					    secretlen + AUTH_VECTOR_LEN);
+
+				/* Remeber the start of the digest */
+				vector = buf;
+
+				/* Xor the password into the MD5 digest */
+				for (pc = i; pc < (i + AUTH_VECTOR_LEN); pc++) {
+					*buf++ ^= passbuf[pc];
+				}
+			}
+
+			total_length += padded_length + 2;
+
+			break;
+		default:
+			switch (vp->type) {
+			case PW_TYPE_STRING:
+				length = vp->lvalue;
+				*buf++ = length + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += length + 2;
+				memcpy(buf, vp->strvalue, (size_t) length);
+				buf += length;
+				total_length += length + 2;
+				break;
+
+			case PW_TYPE_IPV6ADDR:
+				length = 16;
+				*buf++ = length + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += length + 2;
+				memcpy(buf, vp->strvalue, (size_t) length);
+				buf += length;
+				total_length += length + 2;
+				break;
+
+			case PW_TYPE_IPV6PREFIX:
+				length = vp->lvalue;
+				*buf++ = length + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += length + 2;
+				memcpy(buf, vp->strvalue, (size_t) length);
+				buf += length;
+				total_length += length + 2;
+				break;
+
+			case PW_TYPE_INTEGER:
+			case PW_TYPE_IPADDR:
+			case PW_TYPE_DATE:
+				*buf++ = sizeof(uint32_t) + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += sizeof(uint32_t) + 2;
+				lvalue = htonl(vp->lvalue);
+				memcpy(buf, (char *)&lvalue, sizeof(uint32_t));
+				buf += sizeof(uint32_t);
+				total_length += sizeof(uint32_t) + 2;
+				break;
+
+			default:
+				break;
+			}
+			break;
+		}


This is code that was just moved without changes to enable reuse in the async implementation.

mriemensberger · 2023-03-13T08:28:26Z

src/radembedded_async.c

+/*
+	VALUE_PAIR	*vp = NULL;
+	DICT_VALUE	*dval = NULL;
+*/


mriemensberger · 2023-03-13T08:20:44Z

lib/sendserver_util.c

+		switch (vp->attribute) {
+		case PW_USER_PASSWORD:
+
+			/* Encrypt the password */
+
+			/* Chop off password at AUTH_PASS_LEN */
+			length = vp->lvalue;
+			if (length > AUTH_PASS_LEN)
+				length = AUTH_PASS_LEN;
+
+			/* Calculate the padded length */
+			padded_length =
+			    (length +
+			     (AUTH_VECTOR_LEN - 1)) & ~(AUTH_VECTOR_LEN - 1);
+
+			/* Record the attribute length */
+			*buf++ = padded_length + 2;
+			if (vsa_length_ptr != NULL)
+				*vsa_length_ptr += padded_length + 2;
+
+			/* Pad the password with zeros */
+			memset((char *)passbuf, '\0', AUTH_PASS_LEN);
+			memcpy((char *)passbuf, vp->strvalue, (size_t) length);
+
+			secretlen = strlen(secret);
+			vector = (unsigned char *)auth->vector;
+			for (i = 0; i < padded_length; i += AUTH_VECTOR_LEN) {
+				/* Calculate the MD5 digest */
+				strcpy((char *)md5buf, secret);
+				memcpy((char *)md5buf + secretlen, vector,
+				       AUTH_VECTOR_LEN);
+				rc_md5_calc(buf, md5buf,
+					    secretlen + AUTH_VECTOR_LEN);
+
+				/* Remeber the start of the digest */
+				vector = buf;
+
+				/* Xor the password into the MD5 digest */
+				for (pc = i; pc < (i + AUTH_VECTOR_LEN); pc++) {
+					*buf++ ^= passbuf[pc];
+				}
+			}
+
+			total_length += padded_length + 2;
+
+			break;
+		default:
+			switch (vp->type) {
+			case PW_TYPE_STRING:
+				length = vp->lvalue;
+				*buf++ = length + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += length + 2;
+				memcpy(buf, vp->strvalue, (size_t) length);
+				buf += length;
+				total_length += length + 2;
+				break;
+
+			case PW_TYPE_IPV6ADDR:
+				length = 16;
+				*buf++ = length + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += length + 2;
+				memcpy(buf, vp->strvalue, (size_t) length);
+				buf += length;
+				total_length += length + 2;
+				break;
+
+			case PW_TYPE_IPV6PREFIX:
+				length = vp->lvalue;
+				*buf++ = length + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += length + 2;
+				memcpy(buf, vp->strvalue, (size_t) length);
+				buf += length;
+				total_length += length + 2;
+				break;
+
+			case PW_TYPE_INTEGER:
+			case PW_TYPE_IPADDR:
+			case PW_TYPE_DATE:
+				*buf++ = sizeof(uint32_t) + 2;
+				if (vsa_length_ptr != NULL)
+					*vsa_length_ptr += sizeof(uint32_t) + 2;
+				lvalue = htonl(vp->lvalue);
+				memcpy(buf, (char *)&lvalue, sizeof(uint32_t));
+				buf += sizeof(uint32_t);
+				total_length += sizeof(uint32_t) + 2;
+				break;
+
+			default:
+				break;
+			}
+			break;
+		}


This is code that was just moved without changes to enable reuse in the async implementation.

This change was co-authored by Philipp Stanner.

nmav · 2024-01-08T19:47:10Z

COPYRIGHT

 University of Michigan and Merit Network, Inc. shall not be liable for any
 special, indirect, incidental or consequential damages with respect to any
 claim by Licensee or any third party arising from use of the software.
+------------------------------------------------------------------------------


Please no new license. Just add the Copyright at the beginning of the file.

Just to clarify: After line 8 in this file, right? Will do.

nmav · 2024-01-08T19:52:03Z

Sorry for getting to it with such a big delay. 2023 was a crazy year for me. I like the idea, and I would like to review. At the same time there are not any functional tests and for introducing a new API, tests are necessary to ensure that it works as expected, and that future changes do not modify this functionality. If I get to review are you still interested to enhance it with functional tests?

mriemensberger · 2024-01-09T08:00:04Z

Sorry for getting to it with such a big delay. 2023 was a crazy year for me. I like the idea, and I would like to review. At the same time there are not any functional tests and for introducing a new API, tests are necessary to ensure that it works as expected, and that future changes do not modify this functionality. If I get to review are you still interested to enhance it with functional tests?

No worries. I have a small amount of time for upstreaming the changes.

Regarding the functional tests. There is the radembedded_async.c, which exercises the proposed async api similar to radembedded.c. Does it just need an additional wrapper in tests? Or are you looking for more tests beyond what is in radembedded_async.c?

mriemensberger · 2024-01-09T08:02:01Z

src/radembedded_async.c

+/*
+ * Copyright (C) 2022 Cadami GmbH [email protected]
+ *
+ * radembedded.c - a sample c program showing how to embed the configuration of a radius


s/rademedded.c/radembedded_async.c

nmav · 2024-01-09T08:26:42Z

Sorry for getting to it with such a big delay. 2023 was a crazy year for me. I like the idea, and I would like to review. At the same time there are not any functional tests and for introducing a new API, tests are necessary to ensure that it works as expected, and that future changes do not modify this functionality. If I get to review are you still interested to enhance it with functional tests?

No worries. I have a small amount of time for upstreaming the changes.

Regarding the functional tests. There is the radembedded_async.c, which exercises the proposed async api similar to radembedded.c. Does it just need an additional wrapper in tests? Or are you looking for more tests beyond what is in radembedded_async.c?

To run the new code during CI and validate the expected output, for example as in tests/radembedded-tests.sh

nmav · 2024-01-09T08:28:46Z

I see that no github workflows were run in this PR.

mriemensberger · 2024-01-09T08:34:15Z

Regarding the functional tests. There is the radembedded_async.c, which exercises the proposed async api similar to radembedded.c. Does it just need an additional wrapper in tests? Or are you looking for more tests beyond what is in radembedded_async.c?

To run the new code during CI and validate the expected output, for example as in tests/radembedded-tests.sh

Sure. I'll add a similar wrapper for the async version of those tests.

nmav · 2024-01-09T08:48:44Z

I see that no github workflows were run in this PR.

That is a configuration issue of github actions. I've tried to fix it in master, please try rebasing.

nmav · 2024-01-09T08:58:46Z

lib/sendserver_async.c

+ * Copyright 1992,1993, 1994,1995 The Regents of the University of Michigan
+ * and Merit Network, Inc. All Rights Reserved
+ *
+ * Copyright (C) 2022 Cadami GmbH, [email protected]


This seem to be on a strange position. I'd expect it to be on the top together with the BSD2 boilerplate.

nmav · 2024-01-09T09:00:50Z

src/radembedded_async.c

+		exit(EXIT_FAILURE);
+	}
+
+/* ======================== Prepare New Request ============================= */


It seems the function is too long. Should we split into different functions for easier readbility?

nmav · 2024-01-09T09:02:47Z

lib/multihandle.c

+};
+
+
+struct rc_multihandle {


It is not clear to me what is the intention of the multihandle. Some documentation is needed here to explain why this exists and what it does.

nmav · 2024-01-09T09:05:14Z

src/radembedded_async.c

+
+	for (; received_responses < count;) {
+		/* Get all the currently enqueued pollable file descriptors. */
+		nr_of_poll_fds = rc_multi_get_pollfds(mhdl, polli, count);


What is my concern is that the API is very poll-system-call-centric. Today common ways to do it is also with epoll, kqueue or libev. How does the API accomodate these?

There is also rc_multi_get_fd_set and rc_multi_process_fd_set to be used with select.

Also, the entire multihandle API is a thin layer on top of the rc_async_* API to provide an easy way to handle multiple radius requests with poll or select. Those syscalls always report the entire set and multihandle helps with walking and processing the pollfds array or select fdset. It's loosely inspired by libcurl but much simpler.

rc_async_* can also directly be used with and event multiplexer syscall as it exposes the associated fd and the expected events in any state.

Extending the multihandle API to more complex event multiplexer syscalls like epoll and kqueue might be possible although I'm not sure how useful that would be given those syscalls report events not for the entire set of descriptors but only for descriptors that saw events.

Indeed, we should not provide wrappers for all possibilities, just make sure that it is possible to use the API with them. Documentation seem to be important towards that, so that the async API is not perceived as poll-only.

nmav · 2024-01-24T10:37:12Z

Taking a note for me of what remains for final review:

Rebase to master
Documentation that covers usage with epoll / libev
Documentation for structures such as multihandle
Tests that run on CI

mriemensberger · 2024-01-24T10:40:11Z

Thanks @nmav!

It will however take some time until I find the time to go through those points and address them.

Philipp Stanner and others added 4 commits February 10, 2023 02:01

lib: Code move: sendserver.c -> sendserver_util.c

237ac3c

Moves code needed by both sendserver.c and sendserver_async.c to a separate file. Adds an associated header.

lib: Split connect and sendto operations for TCP sockets

dfc36a0

lib: Add a multihandle for handling multiple parallel async radius re…

68dddd9

…quests Implements parallel handling of multiple async requests in multihandle.c with direct support for select and poll event loop structures. This change was co-authored by Philipp Stanner.

mriemensberger force-pushed the implement-async-requests branch 2 times, most recently from 44bc5c5 to 7acb752 Compare February 10, 2023 08:02

nmav self-assigned this Feb 19, 2023

github-advanced-security bot found potential problems Feb 19, 2023

View reviewed changes

mriemensberger added 2 commits March 13, 2023 09:23

src: Add an example using rc_async_* and rc_multi_* functions

31c346c

This change was co-authored by Philipp Stanner.

ABI: update abi dump

27fd8ba

mriemensberger force-pushed the implement-async-requests branch from 7acb752 to 27fd8ba Compare March 13, 2023 08:24

nmav reviewed Jan 8, 2024

View reviewed changes

nmav mentioned this pull request Jan 8, 2024

Add option to not wait for accounting acknowledge/reply #99

Open

mriemensberger commented Jan 9, 2024

View reviewed changes

nmav reviewed Jan 9, 2024

View reviewed changes

nmav added the enhancement label Jan 15, 2024

		};


		struct rc_multihandle {

Conversation

mriemensberger commented Feb 10, 2023

Uh oh!

Check notice

Choose a reason for hiding this comment

Uh oh!

Check notice

Choose a reason for hiding this comment

Uh oh!

Check notice

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!

nmav commented Jan 8, 2024

Uh oh!

mriemensberger commented Jan 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmav commented Jan 9, 2024

Uh oh!

nmav commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mriemensberger commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmav commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nmav commented Jan 24, 2024

Uh oh!

mriemensberger commented Jan 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nmav commented Jan 9, 2024 •

edited

Loading

mriemensberger commented Jan 9, 2024 •

edited

Loading

nmav commented Jan 9, 2024 •

edited

Loading