Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Radius erp #380

Draft
wants to merge 141 commits into
base: main
Choose a base branch
from
Draft

Radius erp #380

wants to merge 141 commits into from

Conversation

mereacre
Copy link
Contributor

@mereacre mereacre commented Jan 9, 2023

This PR incorporates the ERP server into Radius and refactors the VLAN attribute generation nmechanims.

Changes made:

  • Refactored the radius server.
  • Added the ERP server into the RADIUS server
  • Added the ERP-TLS library from hostapd
  • Refactored the VLAN and Tunnel password attributes generation. The RADIUS attributes are stored in a hashmap with the key as the MD5 hash of the RADIUS identity.
  • Added the RADIUS identity processor:
    -- Added the implementation to distinguish between three identities: MAC address, x509 certificate serial number and generic
    -- Added the boiler plate for the access engine for each identity.

Note for reviewers: This PR only compiles for the Linux preset. For other presets we need to add the ability to disable the RADIUS server by means of using the flag USE_RADIUS_SERVER.

@mereacre mereacre marked this pull request as ready for review January 20, 2023 14:00
sys_strlcpy(mbuf, inet_ntoa(cli->mask), sizeof(mbuf));
}

ret = os_snprintf(

Check failure

Code scanning / CodeQL

Potentially overflowing call to snprintf

The [size argument](1) of this snprintf call is derived from its return value, which may exceed the size of the buffer and overflow.
@mereacre mereacre requested a review from aloisklink January 24, 2023 16:47
@aloisklink aloisklink self-assigned this Feb 3, 2023
@aloisklink
Copy link
Contributor

aloisklink commented Feb 3, 2023

I'm currently trying to rebase this branch onto main without the changes from #376.

Current progress: (2023-02-03)

This is going to be a bit of struggle to git rebase, especially since there's 96 commits to go through (+ some commits delete, then re-add files, which git isn't good with).

The command that I'm using for testing currently is:

shopt -s extglob && rm -rf build/!(dl)
git rebase --reschedule-failed-exec --onto main 1da297dd3157ca8670a7a5e06174ebba7075d4d7 --exec '( for PRESET in "linux" "linux-with-crypt" "linux-with-protobuf" "linux-with-example-middlewares" "openwrt" "openwrt-with-header" "openwrt-19.07.10/ath79/generic" "openwrt-21.02.1/bcm27xx/bcm2710"; do cmake --preset "$PRESET" && nice -n19 cmake --build --preset "$PRESET" -j=$(nproc) && { if ctest --list-presets | grep "\"$PRESET\""; then ctest --preset "$PRESET" --output-on-failure; fi } && cmake --install "./build/$PRESET" --prefix "./tmp/$PRESET" || exit 1; done ) && echo "all presets passed"'

Edit: Test compiling with more presets (cross-compile) and use rm -rf build/!(dl) to delete all configuration files. This is slower, but seems to cause less issues with cache invalidation.

Edit 2: Enable --reschedule-failed-exec

Commit 61bbc66 is right before a merge commit, so it's probably worth refactoring everything before that first.

Edit 3: Move the shopt -s extglob && rm -rf build/!(dl) before the git rebase command.
It seems to cause issues, and tbh, we should only rarely need to clear out the build/ dir for issues with the PATCH_COMMAND in eloop.

Notes:

File history:

  • src/radius/radius_server.c and src/radius/radius_server.h:
    • Commit 5493357 deletes these files, and commit 7ec9a06 recreates it.
  • src/radius/radius_server.c.old and src/radius/radius_server.h.old:
    • Added in commit 7ec9a06
    • Renamed to src/radius/radius_server.old.c and src/radius/radius_server.old.h in 4f6e3e5
    • Deleted in 7af32b0

Current progress: (2023-02-06)

I'm still struggling to rebase this PR. My current progress is at https://github.com/nqminds/edgesec/tree/radius-erp-alois-fix (currently pointing at 3efa30c).

What I've done so far is to move some of the commits around using git rebase 1da297dd3157ca8670a7a5e06174ebba7075d4d7 --rebase-merges -i, and move all of the complicated/broken commits to the end of the commit chain, to lesson some of the cognitive load.

Current progress: (2023-02-07)

ad261f0 is giving me constant issues, so I'm ignoring it for now on my https://github.com/nqminds/edgesec/tree/radius-erp-alois-fix branch.

I'll need to manually run git cherry-pick ad261f0bc3c89c0b4a0f360e171f830a682e9991..666a6093f0dff4fce25c59bbd66de984d44d48fe to manually re-add these commits back in later.

My current progress is at https://github.com/nqminds/edgesec/tree/radius-erp-alois-fix (currently pointing at 8043aa8).

I've added ⚠️ to a bunch of commits and commit messages for commits I or @mereacre still need to look into.

Current progress: (2023-02-10)

I've split up dff384c into a bunch of different commits.

My current progress is at https://github.com/nqminds/edgesec/tree/radius-erp-alois-fix (currently pointing at 534c57e).

Current progress: (2023-02-17)

My current progress is at https://github.com/nqminds/edgesec/tree/radius-erp-alois-fix (currently pointing at 9891d15).

I've rebase-ed 1da297dd3157ca8670a7a5e06174ebba7075d4d7..8ccb583f6fff5fdad4e70cafb483c1edd9e18bfe onto main and put them into their own PRs:

I've rebase-ed 8ccb583f6fff5fdad4e70cafb483c1edd9e18bfe..1ae437aac70102c440bb1b68d0ea5f0f5708f2e5 onto main and put them into their own PRs:

Current progress: (2023-02-20)

I'm currently working on git rebase-ing 1ae437aac70102c440bb1b68d0ea5f0f5708f2e5..9d1d1b6cc48a753d8b83019f9988775c7f773a7c.

The command I'm using is:

git rebase --interactive --rerere-autoupdate --autosquash --rebase-merges 1ae437aac70102c440bb1b68d0ea5f0f5708f2e5

Current progress: (2023-02-21)

I'm currently working on git rebase-ing 1ae437aac70102c440bb1b68d0ea5f0f5708f2e5..dc582aba987e10e1544f2e495b2f711b854e26fc onto main using the command:

git rebase --reschedule-failed-exec --onto main 1ae437aac70102c440bb1b68d0ea5f0f5708f2e5 --exec '( for PRESET in "linux-with-crypt" "linux-with-protobuf" "linux-with-example-middlewares" "openwrt" "openwrt-with-header" "openwrt-19.07.10/ath79/generic" "openwrt-21.02.1/bcm27xx/bcm2710"; do cmake --preset "$PRESET" && nice -n19 cmake --build --preset "$PRESET" -j=$(nproc) && { if ctest --list-presets | grep "\"$PRESET\""; then ctest --preset "$PRESET" -j=$(nproc) --output-on-failure; fi } && cmake --install "./build/$PRESET" --prefix "./tmp/$PRESET" || exit 1; done ) && echo "all presets passed"'

Current progress (2023-02-27)

I'm currently working on git rebase-ing 5156aa6f03643eb567e06e0b669e21e71f3d6e4b..a2cd5787c7094b8f5a88b1e57b8246a14c7c8795.

The command I'm using is:

git rebase --interactive --rerere-autoupdate --autosquash --rebase-merges 5156aa6f03643eb567e06e0b669e21e71f3d6e4b

@aloisklink aloisklink marked this pull request as draft February 3, 2023 13:13
@mereacre
Copy link
Contributor Author

mereacre commented Feb 3, 2023

It will not compile on all presets unless we solve the eloop naming issue.

@mereacre
Copy link
Contributor Author

mereacre commented Feb 3, 2023

Also, we need to add an option to disable the compilation of the RADIUS server for the recap preset.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #380 (9006f31) into main (14135a9) will decrease coverage by 0.69%.
The diff coverage is n/a.

❗ Current head 9006f31 differs from pull request most recent head 666a609. Consider uploading reports for the commit 666a609 to get more accurate results

@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
- Coverage   52.48%   51.80%   -0.69%     
==========================================
  Files         142      139       -3     
  Lines       19687    19316     -371     
==========================================
- Hits        10333    10006     -327     
+ Misses       9354     9310      -44     
Impacted Files Coverage Δ
src/radius/radius.c 28.23% <ø> (+0.19%) ⬆️
src/radius/radius_server.c 53.80% <ø> (ø)
src/utils/os.c 50.37% <0.00%> (-2.00%) ⬇️
src/utils/log.c 59.55% <0.00%> (-0.45%) ⬇️
tests/utils/test_os.c 100.00% <0.00%> (ø)
tests/radius/eap_test_server.c
tests/radius/eap_test_peer.c
tests/radius/test_libeap.c

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aloisklink
Copy link
Contributor

aloisklink commented May 26, 2023

@nallott asked me to make a report on what's on the main branch that's causing
merge conflicts with this radius-erp branch.

Warning: This report is not 100% done, I'll need to continue this on Monday.

I'll be honest, the current WIP state of this PR means I'm still struggling
to understand what's in this PR, since the current PR description is pretty
vague, and the code/git history is a mess. But, I can at least say what's in
the main branch and what we've already merged.

Running a git diff --compact-summary main shows the following output:
191 files changed, 6276 insertions(+), 5012 deletions(-).

The head commit from this PR is commit e7c5540d8accf2121d87033d77de855c885ef843
from 2022-12-13 (more than 6 months ago).

Commands like git merge-base will
give you the wrong impression, since there's a bunch of merges from main into
the radius-erp branch, like:

These merge commits make life quite a bit more complicated, since they contain
non-trivial merge conflict resolutions, and because they're built on top of
WIP/broken code, it's very difficult to say whether they're actually good.

What's on the main branch

PRs

We can use GitHub's UI to search for PRs that were merged since 2022-12-13:

See: https://github.com/nqminds/edgesec/pulls?q=is:pr is:merged updated:>=2022-12-13

Currently, there are 172 PRs (7 pages!) that have been merged since 2022-12-13.

PR categories

I can't really summarize these PRs, but from the labels:

Please be aware that some PRs aren't labelled or have milestones associated with
them. For example, a code-change that fixed a bug in a test, wouldn't normally
be labeled as a "bug", since it was only in test code.

Commits

If we take the head commit from the PR, the radius-erp branch is 500 commits
behind the main branch:

alois@pc:~/edgesec (main)$ git log e7c5540d8accf2121d87033d77de855c885ef843..main --pretty=oneline | wc -l
500

However, due to merge commits (which I doubt are valid), most git tools would
say that the radius-erp branch is only 459 commits behind the main branch.

Commit categories

We do not currently have any commit standards for the edgesec project.

However, at @nqminds, we generally use Conventional Commits 1.0.0,
and so many (but not all!) of the edgesec commits follow the same standard.

pie showData
    title Commits in main not in radius-erp branch
    "refactor": 88
    "test": 48
    "fix": 38
    "build": 28
    "ci": 13
    "docs": 25
    "style": 9
    "feat": 12
    "perf": 6
Loading
Python script used to generate the above diagram
#!/usr/bin/env python3
import collections
import subprocess

res = subprocess.run([
    "git", "log", "e7c5540d8accf2121d87033d77de855c885ef843..radius-erp",
    r"--pretty=format:%s"], capture_output=True, check=True, text=True)

counter = collections.Counter()

for commit in res.stdout.split("\n"):
    for commit_type in {"feat", "fix", "test", "perf", "build", "ci", "refactor", "docs", "style"}:
        if commit.startswith(commit_type):
            counter[commit_type] += 1

for commit_type in counter:
    print(f'"{commit_type}": {counter[commit_type]}')

What's on the radius-erp branch

I don't really understand what's in this PR, so lease see the
PR description
by @mereacre.

Commits

A quick check shows that there are 182 commits on the radius-erp branch that
aren't on the main branch. However, this number isn't fully accurate, since
there are quite a few commits that have been git rebase-d and put onto main.

alois@pc:~/edgesec (radius-erp)$ git log e7c5540d8accf2121d87033d77de855c885ef843..radius-erp --pretty=oneline | wc -l
182

Commit categories

I tried running a similar script to the one above on the radius-erp branch,
but since the commit titles are pretty all over of the place (this is a WIP),
I don't think this is very accurate:

pie showData
  title Commits in radius-erp branch
  "test": 18
  "feat": 43
  "build": 8
  "ci": 3
  "fix": 8
  "refactor": 7
  "perf": 1
  "docs": 4
  "style": 1
  "other": 89
Loading

What's already been done (radius-erp-alois-fix branch)

Some progress has already been made in rewriting the radius-erp branch, see
commit #380 (comment).

I've got some notes there, but it's been months since I last looked into it,
I'm sure most of the weeks I spent on this earlier have been forgotten.

The rewrite is happening in the
radius-erp-alois-fix branch.

Please be aware that the radius-erp-alois-fix branch DOES NOT FIX MERGE CONFLICTS.
Instead, it's rewriting the radius-erp branch. By splitting up the commits,
we can more easily test/fix merge conflicts when we do merge changes into main.

Already merged to main

Commits
1da297dd3157ca8670a7a5e06174ebba7075d4d7..dc582aba987e10e1544f2e495b2f711b854e26fc have already had merge-conflcits fixed,
bugs fixed, and been merged into main.

These have been git rebase-ed onto main and merged as the following PRs:

These consist of 53 commits.

alois@pc:~/edgesec (radius-erp-alois-fix)$ git log 1da297dd3157ca8670a7a5e06174ebba7075d4d7..dc582aba987e10e1544f2e495b2f711b854e26fc --pretty=oneline | wc -l
53

Changes from PR #376

The 69 commits e7c5540d8accf2121d87033d77de855c885ef843..1da297dd3157ca8670a7a5e06174ebba7075d4d7
were part of the old PR #376.

These have mostly already been merged as part of
PR #379 and
PR #381, and so can be mostly ignored.

What is left to merge

The 78 commits from dc582aba987e10e1544f2e495b2f711b854e26fc..6e03c82bdedfe7b94b6f23be3d6618c84632f4dc
are still left to cleanup, fix, and hopefully get into a mergeable state.

However, these are some of the more complex commits, so I wouldn't be surprised
if it gets much harder to try to fix them.

Commits

Currently, there are 200 commits on the radius-erp-alois-fix branch
(some of commits in radius-erp have been split in sub-commits)

pie showData
  title Commits in radius-erp-alois-fix branch
  "refactor": 37
  "feat": 53
  "test": 19
  "build": 12
  "fix": 10
  "docs": 12
  "style": 3
  "ci": 1
Loading
pie showData
  title Merge status of commits in radius-erp-alois-fix branch
  "Merged": 53
  "Not needed (see PR #376)": 69
  "Leftover": 78
Loading

Updates

2023-05-30:

Updated the radius-erp-alois-fix branch to 085aca7.

For some reason, on my PC, I had a different version of the radius-erp-alois-fix
branch (commit 6e03c82).

Running a
git range-diff e7c5540d8accf2121d87033d77de855c885ef843..origin/radius-erp-alois-fix e7c5540d8accf2121d87033d77de855c885ef843..origin/radius-erp-alois-fix-elementaldriftwood
seems to show that the primary differences are:

  • The radius-erp-alois-fix-elementaldriftwood branch fixes a few invalid C
    issues (i.e. features that are only supported in C++ or C23).
  • 122: f59e35d5 ! 121: 0ab9964b refactor(radius_server): revert to hostapd v2.10 ⚠️
    The radius-erp-alois-fix-elementaldriftwood branch has some invalid merge
    statements. I've fixed this locally, and pushed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants