OpenVPN, handle basically radius Calling-Station-Id attribute to iden… #7565
Conversation
…tify remote IP for authentication
| @@ -410,6 +410,8 @@ public function updateAccounting($username, $sessionid, $session_time, $bytes_in | |||
| */ | |||
| public function authenticate($username, $password) | |||
| { | |||
| global $remote_address; | |||
There was a problem hiding this comment.
globals are not (ever) allowed in modern code, if the problem is you need to pass properties to the authenticator, you need a clean way to pass items in there (likely needs modifications in the authentication scheme explained in our documentation).
If you have a desire to get features merged eventually, please try to be explicit what you want and why you think it should work the way your proposing.
Just opening tons of tickets and PR's are not likely to help you reach your goals....
There was a problem hiding this comment.
Thats only my patch work of last months ... had a bigger cold last weeks.
I try to give my input to the community back for functionalities which are basically needed.
Checkout your preferedd Oauth communicator like MS Auth, LastPass, OpenOTP and so on and you find on nearly every authenticator (or minimum in the logs) the IP based gelocation to validate /check later for security braches. But for this you need the IP, or not? 😃
It's clear for me too that a global variable isn't the best but I am "admin only" and no php "expert" ...
This is a small fix, easy to read and check out and works since months fine for me/us.
And I have no clue in your MVC template world howto takeover the additional content without rewriting dozens of files by vim ...
There was a problem hiding this comment.
BTW for same reason I haven't yet patched / tried to patch these bulk of OPNsense/Auth authenticators for correct OTP usage passthrough since OPNsense has misimplemented OTP order.
So far I see the dedicated password + otp_token usage over the Auth system needs also changes in every file.
Therefore I created only a local quick hack patch for our dual system in f4a24ea which didn't belong to the "tons of issues" - in total 8 inclusive PRs each.
Clearly i could do it in big 3 ones but by me known developers would then complain about "too complex patches" .
"There is always something" 😃
There was a problem hiding this comment.
opening a lot of PR's like this also doesn't work.... I'm not unwilling to work with you to add a feature, but you're not making it easy either.
My advise would be to close the PR's and start with one (this one for example), take the time to learn how this works with opensource projects. Communicate about the details of the proposed change, alternatives considered and possible (negative) impact to others (if any). Usually standards have references, try to locate them and point to them. This also helps future documentation updates (which helps others as well).
…tify remote IP for authentication