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

Set Calling-Station-Id in accounting by User-Name attribute if it is undefine #1756

Merged
merged 3 commits into from
Nov 1, 2016

Conversation

fdurand
Copy link
Member

@fdurand fdurand commented Oct 12, 2016

Description

Set Calling-Station-Id if it doesn't exist by the User-Name attribute if it represent a mac address.

Impacts

RADIUS Accounting

Issue

Fix empty Calling-Station-Id in radius accounting

Delete branch after merge

YES

NEWS file entries

Bug Fixes

  • Fix empty Calling-Station-Id in radius accounting

@fdurand fdurand added this to the PacketFence-6.4 milestone Oct 12, 2016
@@ -257,9 +260,8 @@ accounting {
&control:PacketFence-RPC-User = ${rpc_user}
&control:PacketFence-RPC-Pass = ${rpc_pass}
&control:PacketFence-RPC-Proto = ${rpc_proto}
&request:Calling-Station-Id = "%{User-Name}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I don't understand something from this logic but it seems like its going to set the Calling-Station-Id to the User-Name in the request whether it looks like a MAC or not, meaning if its a 802.1x connection with username 'lzammit', its going to put 'lzammit' in the Calling-Station-Id.

Only god knows what will happen if 'lzammit' becomes the value of the Calling-Station-Id

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found a picture of the best case:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact it will set the Calling-Station-Id attribute to the user-name if only Calling-Station-Id is undef.
Btw you are right, i will change the policy to something like that:
rewrite_user_name {
if (&User-Name && (&User-Name =~ /^${policy.mac-addr-regexp}$/i)) {
update request {
&User-Name := "%{tolower:%{1}:%{2}:%{3}:%{4}:%{5}:%{6}}"
&Calling-Station-Id = "%{tolower:%{1}:%{2}:%{3}:%{4}:%{5}:%{6}}"
}
updated
}
else {
noop
}
}
And remove &request:Calling-Station-Id = "%{User-Name}"

@@ -111,3 +111,16 @@ rewrite_calling_station_id {
noop
}
}

set_calling_station_id {
if (&User-Name && (&User-Name =~ /^${policy.mac-addr-regexp}$/i)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be careful here.
policy.mac-addr-regexp would also match "aafacceebb01" which could be a user.
You could overwrite a valid username.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, btw this is only use in accounting.
OK so let's start the name challenge, we have to find a username with a or b or c or d or e or f with 12 characters long !

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one of Zayme's cousin is named Abdea Befafa
Which could translate to username abdeabefafa1

Jokes aside, I tend to agree with @fdurand on that point where it is extremely unlikely that will happen and since a few controllers tend to send «vidange requests» then we will fix 99.999999% of the cases and break the remaining.

That seems like a good trade-off and likely the only one we can make as I don't see another way to address this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right then.
Don't come to me when Mr. Befafa complains though 😉

@extrafu
Copy link
Member

extrafu commented Nov 1, 2016

Status?

@louismunro louismunro merged commit 8457d49 into devel Nov 1, 2016
@fdurand fdurand deleted the fix/empty_calling_station_id_accounting branch November 11, 2016 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants