Opened 8 months ago

Closed 5 months ago

Last modified 5 months ago

#5526 closed enhancement (complete)

Radius: Implement Radius communication

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: hook-radius Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 10
Total Hours: 0 Internal?: no

Description

The RadiusDesign assumes the hook library will conduct communication with the Radius server for two purposes: authentication and accounting.

Many operations will be shared for those two types of communication (connect to the server, prepare Radius packet, send data, parse answers, close connection, etc.)

Note that authorization is expected to be done synchronously, while the accounting will by async. Furthermore, authorization will likely one day (but not in its initial implementation) be taking advantage of the park packets feature that was recently implemented.

This ticket covers implementing the core Radius communication code.

Subtickets

Change History (10)

comment:1 Changed 7 months ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 Changed 7 months ago by fdupont

  • Add Hours to Ticket changed from 0 to 10

Reached a point where it becomes reviewable even not fully finished:

  • support for synchronous and asynchronous communication.
  • unit tests

Todo:

  • system tests
  • documentation (in particular for the setup of system tests)
  • cleanup to make further work easier.

comment:3 Changed 6 months ago by fdupont

Ready for review. Note that it is based on trac5525 but new things should be fully in new files. This means that until #5525 is taken I'll update it.

comment:4 Changed 6 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 5 months ago by fdupont

  • Owner changed from UnAssigned to fdupont
  • Status changed from reviewing to accepted

Take it the time to update after #5525 merge.

comment:6 Changed 5 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Update done.

comment:7 Changed 5 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:8 follow-up: Changed 5 months ago by tomek

  • Owner changed from tomek to fdupont

I have reviewed the code. It initially didn't compile due to problems with boost 1.65, but Francis made some workarounds and it now compiles. With the fixes committed on April 19th, I'm now able to build RADIUS hook on mac os 10.12.6 with boost 1.65 and the
freeradius-client from Francis repo (https://github.com/fxdupont/freeradius-client/, branch iscdev).

The code builds and unit-tests all pass. Good work!


With the git diff master...trac5526, the changes included changes done in 5525. As a result, I spent time reviewing code that was already reviewed. We need to avoid such situation in the future.

radius_utils.h
The method printable() should be called toText as similar methods in many other places in the code.

hexa should be called toHex, as existing code are has similar naming convention.

radius_tests.dox
The anchor name ("testServerHowToIntroduction") should be filed in. For now it can be a short list (install this server from repository X, branch Y, use this config, run this command in that directory, etc.)

radius_request.h
rc2txt should be renamed to rcToText()

RadiusRequest? description must be extended. In particular, the usage model has
to be described.

The description of start() should explain what the derived classes are expected
to do in that method. If it's synchronous, are the control expect to return
only after Radius server response has been processed?

Why do we need both send_attrs_ and send_? Aren't the AttributeCollection?
a better type to hold the info? If we really have to used a pointer to send_,
can we at least make sure it's a smart pointer?

Who is going to free the send_ pair? Perhaps the class should have
a virtual destructor?

RadiusSyncAcct::getResult() description must describe the returned status
code. What are the allowed values? Are they following some constants?
The same applies to getResult() in other classes.

The state machine in RadiusAsync? should have been implemented using
src/lib/util/state_model.h. But given the time constraints, there is no
time to rewrite it, so please add a @todo in the code to use the same
model as any other FSM in Kea code.

next() method in line 286 has a comment "received something". it should
be more precise. Is there any possibility to receive something else
than RADIUS servers' response?

Why retry closes/reopens socket? why do we need to open the socket every
time there's something (presumably an accounting request) to be sent?
Couldn't the code open the socket once and use it for all communications?

If not, there should be a comment explaining why.

startTimer(), line 331. Why does it return if there's no timer? Shouldn't
the timer be created in such case?

invokeCallback() should have a comment that explains what the callback will do.

SEND_CONTEXT should be a smart pointer.

Why do we need separate RadiusAsyncAuthImpl?? Since this class is in the
same header as RadiusAsyncAuth?, what's the point of having Impl class?
Couldn't they be merged into one? The same questions applies to RadiusAsyncAcctImpl?.

radius_request.cc
The READBLOCK_RC is only available in freeradius-client with the async patch.

RadiusAsync?<...>::retry() in line 181. What are the cases when ctx_ is
not set? Isn't it mandatory? Does it make sense to move between states
if there's no context?

radius_parsers.h
For some reason when I did git diff master...trac5526 I see the changes
we did for trac5525. This is annoying and the repeated review of a
code that was already reviewed a waste of time. This is one of the
reasons why we should change our practices to have new tickets
always rebased. Some environments make that easy to do.

radius_attribute.h
Attribute(VALUE_PAIR *) should check tbe argument is not null
and throw if it is.

AttributeCollection? should use shared_ptr rather than raw pointers.

attribute_unittest.cc
compare(), line 43 should have the return value documented.

AttributeTest?.fromAVPipv6prefix checks 3 prefixes. The first
is /64. Why does prefix[] contain 16 bytes, instead of only
the first 8 bytes?

There's fillToElement test. It is disabled. The reasons why it
is disabled should be documented (@todo? or maybe pointer to
a ticket?). But in general, I think fillToElement is way too
big. It should be split into smaller unit-tests and disable only
those sections that really have to be disabled.

Line 775 please add comment to explain that 0xc0000201 is 192.0.2.1.

Test AtttributeTest?.fill should be split into smaller tests.

The ifdefed code around 1110 should be either removed or
turned into disabled test.

What's the point of having sync_request_unittests.cc? There
is no code in that file.

request_unittests.h
RequestTest::poll() this should have a better comment. Explain
what happens when and why poll() is called.

RequestTest::open() - the server_port is TCP or UDP?

The comment for RequestTest?.noAuthServer seems wrong. It
says it tests what happends in there is no authentication
at all, but the CONFIGS[0] used have both access and acct
servers configured. The same is true for noAcctServer test.

RequestTest?.noAuthResponse - I don't understand what exactly
is going on in this test. The comment says "when no response
is sent". I interpreted that as "the server does not sent
any response", but the code seems to contradict that.

It looks like the response is received (EXPECT_TRUE(received_)),
a timeout is set to false and there is response processing.
Looks like the test comment and test name are incorrect or
at least misleading.

Overall, your tests are very thorough. Thanks a lot for
writing them.


Passing back to you. Please note that we are working on
a tight schedule. If fixing the issues I mentioned in this
review take a long time, please consider adding @todo instead
and we will deal with them at a later time.


This change requires a changelog entry. Make sure to put it
into the ChangeLog? for premium. Here's my proposal:

24.	[func]		fdupont
	Radius communication library has been implemented. The code
	is now able to communicate with the Radius server.
	(Trac #5526, git tbd)

Final note: I did this review on a plane. Apologies if some of my comments don't make sense.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Replying to tomek:

radius_utils.h
The method printable() should be called toText as similar methods in many other places in the code.

=> I disagree as it emits only printable text. IMHO toPrintable is the right name.

hexa should be called toHex, as existing code are has similar naming convention.

=> done.

radius_tests.dox
The anchor name ("testServerHowToIntroduction") should be filed in. For now it can be a short list (install this server from repository X, branch Y, use this config, run this command in that directory, etc.)

=> it is on my plan but:

  • it is more for next tickets
  • a solution for boost on CentOS is required before.

radius_request.h
rc2txt should be renamed to rcToText()

=> done.

RadiusRequest? description must be extended. In particular, the usage model has
to be described.

=> done.

The description of start() should explain what the derived classes are expected
to do in that method. If it's synchronous, are the control expect to return
only after Radius server response has been processed?

=> yes of course. I updated the description.

Why do we need both send_attrs_ and send_? Aren't the AttributeCollection?
a better type to hold the info? If we really have to used a pointer to send_,
can we at least make sure it's a smart pointer?

=> AttributeCollection? is good for the calling interface but the AVP head is
needed for the C interface. And we already had the discussion about the pointer:
it is C code so no smart pointer.

Who is going to free the send_ pair? Perhaps the class should have
a virtual destructor?

=> The class has already a virtual and not empty destructor which BTW
free the send_ C structure... So I don't understand the question.

RadiusSyncAcct::getResult() description must describe the returned status
code. What are the allowed values? Are they following some constants?
The same applies to getResult() in other classes.

=> the return result is from C code. There is no guarantee it is one of defined
constants defined by C macros.

The state machine in RadiusAsync? should have been implemented using
src/lib/util/state_model.h. But given the time constraints, there is no
time to rewrite it, so please add a @todo in the code to use the same
model as any other FSM in Kea code.

=> I added the @todo but before doing this we have to check if the state model
is usable for this (a priori it should be).

next() method in line 286 has a comment "received something". it should
be more precise. Is there any possibility to receive something else
than RADIUS servers' response?

=> according to the async API PR documentation no. Note the
"resume returned something" in the head comment is more accurate.

Why retry closes/reopens socket? why do we need to open the socket every
time there's something (presumably an accounting request) to be sent?
Couldn't the code open the socket once and use it for all communications?

=> it is how the async API PR is done.

If not, there should be a comment explaining why.

=> added.

startTimer(), line 331. Why does it return if there's no timer? Shouldn't
the timer be created in such case?

=> no because a timer is created when setIOService is called.
The check is to avoid crash if it is not called (I add a comment about sanity check).

invokeCallback() should have a comment that explains what the callback will do.

=> I added comments where the termination callback types are defined.

SEND_CONTEXT should be a smart pointer.

=> No, it is a C structure managed by C code.

Why do we need separate RadiusAsyncAuthImpl?? Since this class is in the
same header as RadiusAsyncAuth?, what's the point of having Impl class?

=> can't bind a pointer to an object in its constructor just because it does not
yet exist. So an intermediate class is required and as I want a shared pointer
I use the special template (without it you finish by distinct shared pointers to
the same object, the first one frees it and the second crashes).
BTW I didn't invent this code: it is in asioxxx test codes in core Kea.

Couldn't they be merged into one? The same questions applies to RadiusAsyncAcctImpl?.

=> same problem so same conclusion. BTW it could be possible to win one class
by integrating request into 5527 access and 5530 accounting but it is not
how the code is supposed to be organized (or written).

radius_request.cc
The READBLOCK_RC is only available in freeradius-client with the async patch.

=> I remembered this and it is already fixed (i.e. before reading this comment).

RadiusAsync?<...>::retry() in line 181. What are the cases when ctx_ is
not set? Isn't it mandatory? Does it make sense to move between states
if there's no context?

=> if you read the rc_aaa_async() code there are some error cases where
the send context (ctx_) is not created. So it is required to check it.
And there are error cases (even it is not expected to get one in
production code).

radius_parsers.h

=> I checked again master (full diff) and there is no difference.

radius_attribute.h

=> same: no difference.

attribute_unittest.cc
compare(), line 43 should have the return value documented.

=> same: no difference but as there was a spelling error I fixed
compare documentation. And I removed the #if 0 I left.
BTW the fill test should not be split because it is used to
check the list too (unfortunately not the easy way, cf the
DISABLED).

What's the point of having sync_request_unittests.cc? There
is no code in that file.

=> the code is in the .h. sync and async files only set some macros
and include it.

request_unittests.h
RequestTest::poll() this should have a better comment. Explain
what happens when and why poll() is called.

=> I copied the boost asio documentation explaining what it does.

RequestTest::open() - the server_port is TCP or UDP?

=> added UDP.

The comment for RequestTest?.noAuthServer seems wrong. It
says it tests what happends in there is no authentication
at all, but the CONFIGS[0] used have both access and acct
servers configured. The same is true for noAcctServer test.

=> hum, seems a "listening" word is needed to clarify what
means "no server" even it is not very correct for UDP
(no listen system call but the behavior can be different if
there is something bound on the server side or not).

RequestTest?.noAuthResponse - I don't understand what exactly
is going on in this test. The comment says "when no response
is sent". I interpreted that as "the server does not sent
any response", but the code seems to contradict that.

It looks like the response is received (EXPECT_TRUE(received_)),
a timeout is set to false and there is response processing.
Looks like the test comment and test name are incorrect or
at least misleading.

=> what is misleading is the client is in the run method and
the code in the test is the pseudo-server.
I am adding a comment explaining this.

This change requires a changelog entry. Make sure to put it
into the ChangeLog? for premium. Here's my proposal:

24.	[func]		fdupont
	Radius communication library has been implemented. The code
	is now able to communicate with the Radius server.
	(Trac #5526, git tbd)

Merged using this ChangeLog.

comment:10 in reply to: ↑ 9 Changed 5 months ago by tomek

Replying to fdupont:

radius_tests.dox
The anchor name ("testServerHowToIntroduction") should be filed in. For now it can be a short list (install this server from repository X, branch Y, use this config, run this command in that directory, etc.)

=> it is on my plan but:

  • it is more for next tickets
  • a solution for boost on CentOS is required before.

I have installed boost 1.65.1 on CentOS 7 and it seems to be working.
I'm working on a description of the installation procedure as part of #5538.

Thanks for this work.

Note: See TracTickets for help on using tickets.