Set standard RADIUS attributes ouside of the RADIUS class #534
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Augustin-FL
changed the title
Radius Attributes are currently not used in a common/standard way by pfSense Authentication components (currently OpenVPN, IPsec xAuth and CaptivePortal) when performing an Authentication. In an attempt to clarify RADIUS PHP code, i suggested in pfsense/pfsense#3640 to standardize the way attributes are used, but outside of the RADIUS class, in auth.inc. This would make normalization easier, and also make the code more clear (i am not sure that using functions like getNasIp() to retrieve arguments is a best practice. Shouldn't arguments be used instead)?
rbgarga
requested changes
Jul 17, 2018
| - $this->putAttribute(RADIUS_SERVICE_TYPE, RADIUS_FRAMED); | ||
| - $this->putAttribute(RADIUS_FRAMED_PROTOCOL, RADIUS_PPP); | ||
| - $this->putAttribute(RADIUS_CALLING_STATION_ID, isset($var['REMOTE_HOST']) ? $var['REMOTE_HOST'] : '127.0.0.1'); | ||
| + $this->putAttribute(RADIUS_SERVICE_TYPE, RADIUS_LOGIN); |
There was a problem hiding this comment.
Why are you removing attributes that are assigned by the original port? Less changes we need to carry on it are better.
There was a problem hiding this comment.
- For
Service-TypeandFramed-Protocolattributes : because these 2 attributes are wrong : they indicate that pfSense is querying Radius server in order to make a PPP connection(which is false...). These two attributes were also not assigned in oldRADIUS.inc. - For
Calling-Station-ID: Because I suggested in Redmine # 5112: Captive Portal - Use User Manager as Authentication source pfsense#3640 to set this attribute here in auth.inc (for user manager authentication) and here in captiveportal.inc (for captiveportal accounting), so i am removing it from the RADIUS class. Also, the new RADIUS port broke this attribute : it is currently unconditionally set to127.0.0.1....
|
@rbgarga I think it should be more clear in a packet capture : This is why i removed attributes assigned by the original port. Please let me know if i you want me to modify something in this pull request |
netgate-git-updates
pushed a commit
that referenced
this pull request
Aug 6, 2018
netgate-git-updates
pushed a commit
that referenced
this pull request
Feb 4, 2023
Changes since 1.1.0: v1.3.0 This release contains some features and enhancements + upgrades all dependencies. - feat: Allow to set reporter on issue create by @ankitpokhrel in #539 - feat: Use single char ellipsis instead of triple dot by @ankitpokhrel in #540 - ehc: Make assignee operation atomic on create by @ankitpokhrel in #531 - ehc: Auto fallback to plain output on notty by @ankitpokhrel in #538 - ehc: Add warning for invalid custom field by @ankitpokhrel in #528 (Original work by @martinpovolny on #525) - fix(build): Invalid commit hash in docker image by @ankitpokhrel in #535 - dep: Upgrade all packages by @ankitpokhrel in #532 - dep: Upgrade golang to v1.19 by @ankitpokhrel in #534 - ci: Upgrade golangci-lint to v1.50.1 by @ankitpokhrel in #536 Full Changelog: ankitpokhrel/jira-cli@v1.2.0...v1.3.0 v1.2.0 This release adds support for Jira v9, a serverinfo command to quickly check your Jira server build info, lets you set resolution, assignee and comment on issue move, and many more. - feat: Add serverinfo command by @ankitpokhrel in #440 - feat: Support for Jira v9 by @ankitpokhrel in #447 - feat: Allow to set start datetime on worklog add by @ankitpokhrel in #453 - feat: Make date time input in worklog flexible by @ankitpokhrel in #465 - feat: Add support for project datatype in custom fields by @oveaurs in #482 - feat: Add weblink to issue (#446) by @Syd7 in #483 - feat: Resolution, assignee & comment on issue move by @ankitpokhrel in #492 - feat: Filter issues by the absence of label(s) by @martinpovolny in #505 - feat: Add labels to the issue listing by @martinpovolny in #506 - feat: Allow setting of fixed columns in the list of issues, epics and sprints by @martinpovolny in #509 - fix: Option to show issues from all projects in sprint list by @ankitpokhrel in #475 - fix: Discrepancy in --insecure flag by @ankitpokhrel in #507 - fix: Make board selection optional by @ankitpokhrel in #502 - fix: Improve support for pager by @ankitpokhrel in #503 - fix: Respect editor env vars in Windows by @ankitpokhrel in #524 - ci: Multi-arch docker image by @ankitpokhrel in #508 - doc: Add link to project in help by @ankitpokhrel in #456 - doc: Add Nix package by @bryanasdev000 in #458 - doc: Update help for completion cmd by @ankitpokhrel in #491 - doc: Add scoop installation process by @alkuzad in #497 - @bryanasdev000 made their first contribution in #458 - @oveaurs made their first contribution in #482 - @Syd7 made their first contribution in #483 - @alkuzad made their first contribution in #497 - @martinpovolny made their first contribution in #505 Full Changelog: ankitpokhrel/jira-cli@v1.1.0...v1.2.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Radius Attributes are currently not used in a common/standard way by pfSense Authentication components (currently OpenVPN, IPsec xAuth and CaptivePortal) when performing an Authentication.
In an attempt to clarify RADIUS PHP code, i suggested in pfsense/pfsense#3640 to standardize the way attributes are used, but outside of the RADIUS class, in
auth.inc. This would make normalization easier, and also make the code more clear (i am not sure that using functions likegetNasIp()to retrieve variables is a best practice. Shouldn't arguments be used instead)?