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

/elmr/session always returns a new session ID #3

Open
ddriddle opened this issue Nov 7, 2018 · 5 comments
Open

/elmr/session always returns a new session ID #3

ddriddle opened this issue Nov 7, 2018 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@ddriddle
Copy link
Contributor

ddriddle commented Nov 7, 2018

@argherna please consider the following curl commands:

$ curl -b cookie.txt -c cookie.txt -I  http://127.0.0.1/elmrsample/attributes
HTTP/1.1 302 
Server: nginx/1.14.0
Date: Wed, 07 Nov 2018 01:33:49 GMT
Connection: keep-alive
Set-Cookie: __edu.illinois.techservices.elmr.serviceUrl=/elmrsample/attributes; Path=/
Location: http://127.0.0.1/auth/elmr/session

$ curl -b cookie.txt -c cookie.txt -I  http://127.0.0.1/auth/elmr/session
HTTP/1.1 302 302
Server: nginx/1.14.0
Date: Wed, 07 Nov 2018 01:34:13 GMT
Connection: keep-alive
Set-Cookie: __edu.illinois.techservices.elmr.servlets.sessionKey=LTcwNTEzNTYwNjYwNjE4MjExMQ==; Path=/
Location: http://127.0.0.1/elmrsample/attributes

$ curl -b cookie.txt -c cookie.txt -I  http://127.0.0.1/auth/elmr/session
HTTP/1.1 302 302
Server: nginx/1.14.0
Date: Wed, 07 Nov 2018 01:34:37 GMT
Connection: keep-alive
Set-Cookie: __edu.illinois.techservices.elmr.servlets.sessionKey=OTQ3MjA4MzcxMTk3MzYyNzY5; Path=/
Location: http://127.0.0.1/elmrsample/attributes

Note that sessionKey changes between the first call to /auth/elmr/session and the last call. This does not seem desirable to me. If the user's Shibboleth session has not changed then why should we change the session key ID? Also this could be a potential DoS attack. This would allow a single user to spam elmr with requests which would eventually blow out the Redis cache.

@ddriddle ddriddle added the bug Something isn't working label Nov 7, 2018
@argherna
Copy link
Contributor

argherna commented Nov 7, 2018

That's true. Perhaps a second key needs to be set, like the HTTP Session in Tomcat. Cookies are used to manage that; perhaps that's the way to do it.

@argherna
Copy link
Contributor

argherna commented Nov 8, 2018

This is tricky though. In thinking this through HTTP sessions would have to be able to scale horizontally, right? If that's the case, then we would need to install a session manager for Tomcat that allowed for that (like this one). It has issues, and would take some work to get moving. But it's an idea. I'd like to discuss our options though before we work on this too much more.

@ddriddle
Copy link
Contributor Author

ddriddle commented Nov 8, 2018

@argherna Yes, we must support horizontal scaling. @kwessel Instead of using a Tomcat session, could we use the Shibboleth session ID?

@argherna
Copy link
Contributor

argherna commented Nov 8, 2018

Another thought. Elmr is temporary for us. We will eventually use congnito with SAML. Could we take a shortcut here and make the session sticky? If so, we can develop a simple solution to this problem.

@argherna
Copy link
Contributor

We can identify the user with the environment variable Shib_Session_ID. This uniquely identifies the user. So, elmr could set a pair with this identifier as the key and the generated session key as the value. Then the generated session key would be used to store the user session data as before.

argherna added a commit that referenced this issue Nov 16, 2018
- Fix for #3 will look for the `Shib_Session_ID` request attribute. This
has to be set in the apache config as a
  JkEnvVar. Can be overridden by setting a system property or context
parameter (see README).

- Fix for #3 also adds a new method to SessionData interface that takes
  a pre-computed key for saving data to the session data store.

- Add system property and context parameters to configure the number of
  minimum and maximum connections to the session data store (see
  README).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants