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

Prevent SP Metadata to be saved in Session #1053

Open
wants to merge 1 commit into
base: simplesamlphp-1.15
from

Conversation

Projects
None yet
3 participants
@m0ark
Copy link
Contributor

m0ark commented Feb 21, 2019

Currently SP Metadata is saved in the session for every AuthN request. This can lead to heavily polluted sessions in case of unprocessed AuthN requests (in my case some services frontend trying to access protected data once every 5 seconds).
Combine this with the default configured session.state.timeout of 60 minutes and we had to face sessions of ~72mb size for every user per 100kb of SP metadata.

This patch prevents saving the whole SP Metadata and only saves the EntityId.

@thijskh thijskh added this to the 2.0 milestone Mar 7, 2019

@thijskh

This comment has been minimized.

Copy link
Member

thijskh commented Mar 7, 2019

Seems reasonable. However, it does change the API quite a bit. But might be good to merge for 2.0?

@jaimeperez

This comment has been minimized.

Copy link
Member

jaimeperez commented Mar 7, 2019

Hi @m0ark!

The metadata is getting into the state array by design. With your approach, a change in the metadata that happens during an ongoing authentication flow would lead to an inconsistency in the state, and as such, to unexpected behaviour. This is something we would like to avoid, and therefore, the entire metadata array ended up in the state array which is in turn stored in the session.

Now, I find it really weird that you have 100Kb of SP metadata stored in the state array, not to talk about the ~72Mb size sessions. I suspect there must be something wrong going on in your deployment, because what I see here is around 4Kb size per session. Of course, if you start the authentication flow again without having completed it before, that would lead to a new state array with its corresponding metadata saved to the session. But I don't think that's a normal situation. What is it that you have in the metadata that weights so much? Why are you ending up with SP metadata being stored to the session numerous times per user?

That said, I noticed a huge possibility for improvement in the way we do garbage collection in the SimpleSAML\Session class. Basically, garbage collection is being done only when some data is actually stored into the session, and at no other time. Would you mind applying the following patch (you might need to adapt it if you are not running the latest stable version of the software) and telling me how that works for you?

diff --git a/lib/SimpleSAML/Session.php b/lib/SimpleSAML/Session.php
index cd158b3fe..6f19fc0d1 100644
--- a/lib/SimpleSAML/Session.php
+++ b/lib/SimpleSAML/Session.php
@@ -445,6 +445,9 @@ class Session implements \Serializable, Utils\ClearableState
      */
     public function save()
     {
+        // clean out old data
+        $this->expireData();
+
         if (!$this->dirty) {
             // session hasn't changed, don't bother saving it
             return;
@@ -871,9 +874,6 @@ class Session implements \Serializable, Utils\ClearableState
         assert(is_string($id));
         assert(is_int($timeout) || $timeout === null || $timeout === self::DATA_TIMEOUT_SESSION_END);
 
-        // clean out old data
-        $this->expireData();
-
         if ($timeout === null) {
             // use the default timeout
             $timeout = self::$config->getInteger('session.datastore.timeout', null);
@@ -928,6 +928,7 @@ class Session implements \Serializable, Utils\ClearableState
 
                 if ($ct > $info['expires']) {
                     unset($typedData[$id]);
+                    $this->markDirty();
                 }
             }
         }
@@ -953,8 +954,6 @@ class Session implements \Serializable, Utils\ClearableState
             return null;
         }
 
-        $this->expireData();
-
         if (!array_key_exists($type, $this->dataStore)) {
             return null;
         }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.