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

phpCAS is not handling logout requests when behind a load balancer #311

Open
austin48 opened this issue Jun 25, 2019 · 9 comments
Open

phpCAS is not handling logout requests when behind a load balancer #311

austin48 opened this issue Jun 25, 2019 · 9 comments

Comments

@austin48
Copy link

  1. place your app behind a load balancer.
  2. configure config.php
    $cas_real_hosts = array('cas-real-1.example.com', 'cas-real-2.example.com');
  3. login to your app
  4. in a separate browser tab visit: https://your.cas-server.com/cas/logout
  5. go back to your app and refresh the page
  6. The app is still logged in... should not be

I suspect this is due to Client.php
$client_ip = $_SERVER['REMOTE_ADDR'];
which if your app is behind a load balancer, will return the load balancer's IP instead of the end user's ip. Instead, Client.php should check all the x-forwarded-for, etc. headers and try to find the real IP address. for example: https://stackoverflow.com/questions/44085102/php-most-accurate-safe-way-to-get-real-user-ip-address-in-2017

see also, https://groups.google.com/a/apereo.org/forum/#!topic/cas-user/Wte7lwmYkkc

@phy25
Copy link
Member

phy25 commented Jun 25, 2019

I guess this can also be done on the web server end? Of course it would be nice if a extendable method of getting IP address can be added, or add a new config of "whitelist IP ranges" (while this will make the whole library a bit complicated).

@jfritschi
Copy link
Contributor

jfritschi commented Jun 25, 2019

The challenge with logout is that there is that phpcas is lightweight and has no persistent "storage" to map a logout request to a phpcas session. Phpcas cheats by having a direct mapping from SessionTicket -> php session (Technically a hash + secret).

Having this logout work depends on a user and logout call (not from user directly but originating from the CAS server) ending up on the same web farm instance. This is usually the issue since all loadbalancer magic around sticky sessions does not help due to the different origins of the requests. Unless you have a shared php session storage between the nodes you have to use one of our very rarely used phpcas features that is called "Rebroadcast". I have never used it myself and not sure what the quality is.
Basically you tell all nodes where to rebroadcast the logout call to. If the logout call reaches the wrong node #1 it rebroadcasts the call to all other nodes in your web farm. This way every logout call reaches all nodes and successfully kills the session.

You can see it in the examples and source doc:
https://apereo.github.io/phpCAS/
https://github.com/apereo/phpCAS/blob/master/docs/examples/example_proxy_rebroadcast.php

GitHub
Jasig PHP CAS Client. Contribute to apereo/phpCAS development by creating an account on GitHub.

@austin48
Copy link
Author

On a basic level, couldn't you do:

$client_ip = getenv("HTTP_CLIENT_IP")?: getenv("HTTP_X_FORWARDED_FOR")?: getenv("HTTP_X_FORWARDED")?: getenv("HTTP_FORWARDED_FOR")?: getenv("HTTP_FORWARDED")?: getenv("REMOTE_ADDR");

As far as the "Rebroadcast" if the request comes into a different node than the end user is on, which jfritschi mentioned above, I feel that that's a separate issue. But yes, that's a good point. Thanks for the link to the proxy example

@austin48
Copy link
Author

Hello @jfritschi

I tried testing the code I mentioned in my last comment and that seems to work for getting the actual client_ip instead of the load balancer's IP. Should I create a PR?

Also, as far as the CAS logout request not going to the same node in the cluster, yes, I confirmed that that is an issue. Then I tried adding the rebroadcast call as you suggested,

phpCAS::addRebroadcastNode($rebroadcast_node_1)
phpCAS::addRebroadcastNode($rebroadcast_node_2)

but that caused some errors:

PHP Warning: Invalid callback CAS_Request_CurlRequest::_curlReadHeaders, cannot access private method CAS_Request_CurlRequest::_curlReadHeaders() in /home/myapp/phpCAS-1.3.7/source/CAS/Request/CurlMultiRequest.php on line 134

HP Warning: curl_multi_exec(): Could not call the CURLOPT_HEADERFUNCTION in /home/myapp/phpCAS-1.3.7/source/CAS/Request/CurlMultiRequest.php on line 134

HP Fatal error: Call to private method CAS_Request_CurlRequest::_storeResponseBody() from context 'CAS_Request_CurlMultiRequest' in /home/myapp/phpCAS-1.3.7/source/CAS/Request/CurlMultiRequest.php on line 140

PHP Warning: (null)(): 37 is not a valid cURL handle resource in Unknown on line 0

PHP Warning: (null)(): 37 is not a valid cURL handle resource in Unknown on line 0

Any ideas about that?

@austin48
Copy link
Author

Here's another question... would it be possible for the CAS Single Logout POST request to add additional info about the 'canonical' hostname of the CAS server? e.g the main CAS hostname? Either as post data or in the SAML message? That way if the 'real' cas hosts (behind the cas load balancer) change, we won't need to update $cas_real_hosts in our config.php... the phpCAS logout request handler would then check the validity of the request against the main hostname (instead of the 'real' hosts?

@phy25
Copy link
Member

phy25 commented Feb 24, 2020

@austin48 Sorry for the late reply, not sure if this is still relevant.

I tried testing the code I mentioned in my last comment and that seems to work for getting the actual client_ip instead of the load balancer's IP. Should I create a PR?

I would prefer having a single function to get IP address, and extending it if needed, OR having a configuration to control the behavior. If an app is not behind a LB, making this as a default will make it not secure.

PHP Warning: Invalid callback CAS_Request_CurlRequest::_curlReadHeaders, cannot access private method CAS_Request_CurlRequest::_curlReadHeaders() in /home/myapp/phpCAS-1.3.7/source/CAS/Request/CurlMultiRequest.php on line 134

Probably a bug? Will take a look later. I think _storeResponseBody() should be replaced by storeResponseBody(), and _curlReadHeaders() may need to be protected or public?

Here's another question... would it be possible for the CAS Single Logout POST request to add additional info about the 'canonical' hostname of the CAS server? e.g the main CAS hostname? Either as post data or in the SAML message? That way if the 'real' cas hosts (behind the cas load balancer) change, we won't need to update $cas_real_hosts in our config.php... the phpCAS logout request handler would then check the validity of the request against the main hostname (instead of the 'real' hosts?

Is there a specification about this in SAML or CAS protocal specification?

However I think it may be worth moving the validation part out of the handleLogoutRequests() logic so that it can be extended.

@jgribonvald
Copy link
Contributor

PHP Warning: Invalid callback CAS_Request_CurlRequest::_curlReadHeaders, cannot access private method CAS_Request_CurlRequest::_curlReadHeaders() in /home/myapp/phpCAS-1.3.7/source/CAS/Request/CurlMultiRequest.php on line 134

Probably a bug? Will take a look later. I think _storeResponseBody() should be replaced by storeResponseBody(), and _curlReadHeaders() may need to be protected or public?

If moving _storeResponseBody() and _curlReadHeaders() to public it solves the warning message (tested with 1.3.8 version) ! I didn't see any problem after these change. After is it a good way to do that ? I don't know much on these codes.

@phy25
Copy link
Member

phy25 commented Sep 14, 2020

@jgribonvald It has been a long time so I don't quite remember the detail, but I think it should be fine. Please feel free to make a pull request if you want :-)

@jgribonvald
Copy link
Contributor

jgribonvald commented Sep 22, 2020

@phy25 see here my PR, I didn't change the functions names as I'm not a php developper and that I'm not confident on everywhere this should be changed. Feel free to complete my pull request ;)

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

No branches or pull requests

4 participants