simplesamlphp / simplesamlphp/simplesamlphp-module-radius

RADIUS authentication is retried at secondary server even if first server returned Auth Failure

Open
#35 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
2
Forks
2
PR merge metrics
No merged PRs in 30d

Description

We have configured two RADIUS servers for failover. Recently, I noticed that failed authentications from the primary are immediately re-asked at the secondary server (which still generates and Auth failure, so the end result is consistent and no harm done).

But there's really no point in asking the failover server if the primary is sure that the auth failed.

Looking at the code, I found a logic error here:

https://github.com/simplesamlphp/simplesamlphp-module-radius/blob/27d7591603dd2665a147857a4f8adcd33eb6d6cf/src/Auth/Source/Radius.php#L167

The code considers the RADIUS query successful only if it returns not-false.

The underlying library returns sth not-false only in case the authentication succeeded. Notably, a failed authentication is as "false" as a protocol error. See the return paths of its function: they are either outright "false" or compare whether the authentication was a success:

https://github.com/dapphp/radius/blob/master/src/Radius.php#L1752

I.e. error conditions and a negative outcome both have the same result; and the calling module in SSP will loop over all configured servers in both cases. Only a positive result breaks out of the loop.

Ideally, a confirmed negative result from the primary authentication server should be taken as-is.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/Auth/Source/Radius.php around line 167 and inspect the referenced return paths in dapphp/radius's src/Radius.php around line 1752. Confirm how authentication failure and protocol errors are represented, then add coverage showing a confirmed primary denial is not retried while an error can still fail over.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
authentication, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.