SEC-2135: Support Servlet 3.1's HttpServletRequest#changeSessionId() as alternate session fixation protection strategy

[spring-projects-issues]

Nick Williams (Migrated from SEC-2135) said:

The Servlet 3.1 specification, set to release in the next month or two, adds a new method to HttpServletRequest that supports changing the ID of a session as an approach to session fixation protection. This is a more ideal approach than Spring Security's current approach of creating a new session, copying all (or part) of the contents of the current session to the new session, then invalidating the old session. Using the changeSessionId method over Spring Security's current strategies would likely achieve a performance improvement, especially in a high-load scenario.

As a developer using Spring Security, I need SS to support calling changeSessionId as alternate session fixation protection strategy. I would be glad to implement this and submit the pull request, but it's a big change so I'd like to have a discussion and get some buy-in on my planned design before I set to work.

First: Timeline Considerations
I don't know what the product roadmap is for Spring Security, but based on previous release cycles, I'm guessing you're targeting 3.2.0.M2 by March 1, M3 by April 1 and GA by May 3, putting it about 5 months behind Spring 3.2.0.GA (not saying that's a bad thing, just laying out some observations). If that timeline holds true, I'm guessing we can expect Spring Security 4.0 about this time next year and maybe a little later. I'd love to see support for changeSessionId before then.

Second: Technical Considerations
My belief in how this would technically work is that a new option would be added to the session-fixation-protection namespace attribute (currently "none," "newSession," and "migrateSession"). For argument's sake, let's call this new option "changeSessionId." I believe if the documentation clearly stated that this option required Servlet 3.1, we could implement it in SS 3.x. If the "changeSessionId" option was specified in an older Servlet environment, a configuration exception would be thrown. Then we could use reflection to access changeSessionId so that we didn't have to compile against Servlet 3.1.

Third: Design Considerations
I think the largest roadblock to implementing this in SS 3.x is the idea of design. Looking through the code and sketching some things out, I'm having a hard time imagining changing it without subtracting from the public interface of SessionFixationProtectionStrategy. Current algorithm:

• If session-fixation-protection is "none," a different class is used (not a problem).
• If session-fixation-protection is "newSession," use SessionFixationProtectionStrategy with migrateSessionAttributes set to false.
• If session-fixation-protection is "migrateSession," use SessionFixationProtectionStrategy with migrateSessionAttributes set to true.
• An old @Deprecated List<String> retainedAttributes property on SessionFixationProtectionStrategy that allows specifying a list of attributes that will be retained if migrateSessionAttributes is set to false.

The problem is migrateSessionAttributes and retainedAttributes. The code that decides between these is already a little confusing. If we were to add the "changeSessionId" option to session-fixation-protection, we would now have to have a third property and do a mutual exclusion among them all (if we wanted to avoid subtracting from the public interface). The logic becomes sketchy and confusing and would make configuration more difficult.

The simpler alternative would be to remove the migrateSessionAttributes and retainedAttributes completely and replace them with an Enum and a single property for that Enum. The Enum's options would be NEW_SESSION, MIGRATE_SESSION and CHANGE_SESSION_ID and those would map to "newSession," "migrateSession" and "changeSessionId," respectively.

The second option is "the right way," IMO, but that is ignoring the fact that this is an existing software that people are using. The first option is more friendly because it doesn't break backwards compatibility. However, the impact of the second option could be lessened by the fact that most people use the session-fixation-protection namespace attribute to configure this and don't bother with the SessionFixationProtectionStrategy class directly. How many people actually use SessionFixationProtectionStrategy directly? That's a question I don't have the answer to.

An alternative third option would be to pick a suitable default (perhaps "migrateSession," which is currently the default anyway) and deprecate migrateSessionAttributes as well, then change the documentation for migrateSessionAttributes and retainedAttributes to say that they are now ignored and the default will be chosen if you don't use the Enum. This has the advantage of not breaking consuming projects' builds, but likewise has the disadvantage of masking when someone is using a method that isn't supported anymore (assuming they ignore compile warnings).

I know that SpringSource occasionally changes interfaces between ?.x releases (I had compile errors when upgrading from Spring Framework 3.0 to 3.1). So the question is, how big of a deal is it if the interface of this class changes in 3.2? We could always wait for 4.0, but like I said, I reaaaalllly don't want to wait that long. :-)

Thoughts?

[spring-projects-issues]

Rob Winch said:

First of all passivity is very important. The past aside, Spring Security should remain passive between any minor release except for a vulnerability that has no other means of being resolved.

A third option would be to create a new implementation of SessionAuthenticationStrategy that is called ChangeSessionAuthenticationStrategy

[spring-projects-issues]

Nick Williams said:

That was my gut reaction, too. However, the problem with that is that ConcurrentSessionControlStrategy extends SessionFixationProtectionStrategy, so creating a new class (ChangeSessionAuthenticationStrategy) would break that completely. There would have to be two ConcurrentSessionControlStrategy s: one to extend each. That's poor design.

Unfortunately, to keep that inheritance working, these changes must be made to SessionFixationProtectionStrategy.

When I upgraded from Spring Framework 3.0.x to Spring Framework 3.1.x, I had dozens of compile errors due to lack of passivity (and I'm fairly certain they were due to new features, not fixing showstopper bugs). I (and thousands of other users) apparently got over that without much fuss. Does Spring Security have different rules about this than Spring Framework? I would think as an organization you would want to keep something that important consistent across all projects.

[spring-projects-issues]

Rob Winch said:

Unfortunately, to keep that inheritance working, these changes must be made to SessionFixationProtectionStrategy.

This is a good point. Perhaps you could look into...

• Deprecate ConcurrentSessionControlStrategy

• Provide a replacement for ConcurrentSessionControlStrategy that does not extend SessionFixationProtectionStrategy

• Provide a CompositeSessionControlStrategy that iterates over multiple SessionControlStrategy implementations

• Users can then use CompositeSessionControlStrategy injected with multiple implementations of SessionControlStrategy (i.e. the new ConcurrentSessionControlStrategy and SessionFixationProtectionStrategy or the new ConcurrentSessionControlStrategy and ChangeSessionAuthenticationStrategy)

  When I upgraded from Spring Framework 3.0.x to Spring Framework 3.1.x, I had dozens of compile errors due to lack of passivity (and I'm fairly certain they were due to new features, not fixing showstopper bugs). I (and thousands of other users) apparently got over that without much fuss. Does Spring Security have different rules about this than Spring Framework? I would think as an organization you would want to keep something that important consistent across all projects.

With more and more extensions (Kerberos, SAML, OAuth, Spring Social provides integration, etc) being built on top of Spring Security it is critical that we are passive between minor releases. Not to mention we do not want to frustrate our users nor do we want to delay them from being able to update. In short, it must be passive or wait till a major version change.

[spring-projects-issues]

Nick Williams said:

I like that better. It's cleaner AND it's passive. We really shouldn't have one session control strategy inheriting from another, because it limits the ability to create more session control strategies. In fact, THIS IS WHY WE CAN'T DISABLE SESSION FIXATION PROTECTION!

Recall in SEC-2002 that I said we had two problems: 1) no events about session migration, 2) even if we set session-fixation-protection="none", sessions still got migrated? Here's our config:

<session-management invalid-session-url="/web/login/timedOut"
                    session-fixation-protection="none"
                    session-authentication-error-url="">
    <concurrency-control expired-url="/web/login/expired"
                         max-sessions="5"
                         error-if-maximum-exceeded="false"
                         session-registry-alias="sessionRegistry" />
</session-management>

The presence of <concurrency-control> causes a ConcurrentSessionControlStrategy to be created. Since this extends SessionFixationProtectionStrategy it forces the enabling of session migration, even if session fixation protection is set to "none". I have verified this by looking at the code. This is understandably a problem, and I will now file a bug about it since I understand why it's happening. My changes will resolve that bug and this enhancement request.

So am I clear to move ahead with these changes as discussed?

One question I'd like some input on. Deprecating the current ConcurrentSessionControlStrategy makes sense, but what on this good Earth do I call the new one!? "NewConcurrentSessionControlStrategy"? "BugFreeConcurrentSessionControlStrategy"? LOL! :-/

[spring-projects-issues]

Rob Winch said:

So am I clear to move ahead with these changes as discussed?

As it stands I think the changes sound reasonable. It may still take a few iterations once we discuss the code, but as it stands it sounds good. Keep in mind that this is no different than when I implement features (often times I go through a few implementations before picking one).

One question I'd like some input on. Deprecating the current ConcurrentSessionControlStrategy makes sense, but what on this good Earth do I call the new one!? "NewConcurrentSessionControlStrategy"? "BugFreeConcurrentSessionControlStrategy"? LOL! :-/

That is a good question...for now I leave that to you. One strategy is to put Default in front of the class but I try to avoid this strategy if possible

[spring-projects-issues]

Nick Williams said:

Yea, "Default" has always bugged me...

One thought. I still don't think I like the session fixation protection to be broken into two classes, because it's the same feature. I could understand going the extreme and having THREE classes (one for "newSession," one for "migrateSession" and one for "changeSessionId"), but having one for "newSession" and "migrateSession" and another for "changeSessionId" seems like a very inconsistent architecture to me. What if we did this (using "Default" for now in absence of better idea):

• Deprecate ConcurrentSessionControlStrategy and don't change it otherwise - users who use this directly can continue to do so for now.
• Deprecate SessionFixationProtectionStrategy and don't change it otherwise - users who use this directly can continue to do so for now.
• Create new class DefaultConcurrentSessionControlStrategy that doesn't extend SessionFixationProtectionStrategy but does the same thing the current one does.
• Create new enum SessionFixationProtectionScheme with values NEW_SESSION, MIGRATE_SESSION and CHANGE_SESSION_ID.
• Create new class DefaultSessionFixationProtectionStrategy that supports "newSession", "migrateSession" AND "changeSessionId" using SessionFixationProtectionScheme. (Maybe call this new class SessionFixationProtectionSchemeStrategy??)
• Change the configuration class that creates strategies to use the new strategies instead of the deprecated ones.
• In Spring Security 4.0 we can consider removing the deprecated ConcurrentSessionControlStrategy and SessionFixationProtectionStrategy classes completely.

It's passive (no broken builds), it's well-designed (just in my opinion, of course), it's flexible for future ways to protect from session fixation, and existing namespace configurations should work like they always have without any changes, (with the exception that SEC-2137 will now be fixed).

[spring-projects-issues]

Rob Winch said:

In reply to comment #6:

One thought. I still don't think I like the session fixation protection to be
broken into two classes, because it's the same feature. I could understand going

The interface, SessionAuthenticationStrategy, is what we want to do apply some sort of strategy to authentication. There can be numerous implementations of how we protect against session fixation. To me this makes sense...think of it in terms of Java Collection interface with ArrayList and LinkedList both being a List (protect against session fixation). In some instances, one implementation may be better than the other (just as we may want to protect against session fixation by changing the id if it is available or using the existing approach if we are on another container). It also makes sense to make distinct implementations because the migrate session attributes property makes no sense on the change session id implementation. By having separate implementations, we ensure that it is easier for users to use.

[spring-projects-issues]

Nick Williams said:

Understood, and I'm sorry that I wasn't clear on my statement that you referenced. I was saying that it seemed to me that there was a way to achieve this without separating the two and still make it clean, and I explained my idea in the bullet points. The idea I proposed would NOT have a migrateSessionAttributes method. Instead, a Java enum would indicate which scheme was being used to protect against session fixation. Both the original concurrent session control strategy AND the original session fixation protection strategy would be deprecated (instead of just the original concurrent session control strategy as you had previously suggested).

Essentially, the entire current system would be replaced as a whole instead of replaced in pieces (which would be easier, really) and the old system would be deprecated so that users could keep using it. Does this make sense?

So are you saying you disagree with that strategy?

[spring-projects-issues]

Rob Winch said:

I see...I'd probably have to think on it a bit but I think that it doesn't make sense to have retainedAttributes on the change session implementation either. They seem like they are small amounts of refactoring at this point (i.e. moving code to a different) class so I would probably put something together and we can see what you come up with (perhaps your idea is better than mine and I will realize it when I see it).

[spring-projects-issues]

Nick Williams said:

Here's a quick mock-up-kind-of-pseudo-code (don't want to spend too much time going down this path unless you think it makes sense).

This is just like you suggested:

...
+ @Deprecated
  public class ConcurrentSessionControlStrategy extends SessionFixationProtectionStrategy
...

This is a change I'm suggesting:

...
+ @Deprecated
  public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy {
...

This one is just as you suggested, except neither of us like the "Default" in the name.

...
public class DefaultConcurrentSessionControlStrategy implements SessionAuthenticationStrategy {
...
[does some thing as current ConcurrentSessionControlStrategy]

This is a change I'm suggesting:

public enum SessionFixationProtectionScheme {
    NONE("none") {
        @Override
        void applySessionFixationProtection(...) {
            // do nothing
        }
    },

    NEW_SESSION("newSession") {
        @Override
        void applySessionFixationProtection(...) { ... }
    },

    MIGRATE_SESSION("migrateSession") {
        @Override
        void applySessionFixationProtection(...) { ... }
    },

    CHANGE_SESSION_ID("changeSessionId") {
        @Override
        public void checkConfigurationValid(ServletContext servletContext) {
            if(not servlet 3.1)
                throw some kind of exception;
        }

        @Override
        void applySessionFixationProtection(...) { ... }
    };

    private static final Map<String, SessionFixationProtectionScheme> 
        namespaceValues = new Hashtable<...>(4);

    private final String namespaceValue;

    private SessionFixationProtectionScheme(String namespaceValue) {
        this.namespaceValue = namespaceValue;
        namespaceValues.put(this.namespaceValue, this);
    }

    public final String getNamespaceValue() {
        return this.namespaceValue;
    }

    public void checkConfigurationValid(ServletContext servletContext) {
        // usually valid, do nothing
    }

    abstract void applySessionFixationProtection(
        Authentication authentication, HttpServletRequest request,
        HttpServletResponse response
    );

    public static SessionFixationProtectionScheme getFromNamespaceValue(
        String namespaceValue
    ) {
        return namespaceValues.get(namespaceValue) or throw exception;
    }
}

This is a change I'm suggesting:

public class SessionFixationProtectionSchemeStrategy implements SessionAuthenticationStrategy {
...
    private SessionFixationProtectionScheme sessionFixationProtectionScheme;
....
    @Override
    onAuthentication(Authentication authentication, HttpServletRequest request, 
                     HttpServletResponse response) {
        String originalSessionId;
        ...
        this.sessionFixationProtectionScheme.applySessionFixationProtection(
            authentication, request, response
        );
        this.onSessionChange(originalSessionId, request.getSession(), ...);
    }
...
    public void setSessionFixationProtectionScheme(
            SessionFixationProtectionScheme sessionFixationProtectionScheme) {
        this.SessionFixationProtectionScheme = sessionFixationProtectionScheme;
    }
...
}

This is just like you suggested (although I'm still evaluating how these strategies are used, and it may already work like this without using the composite strategy):

public class CompositeSessionAuthenticationStrategy implements SessionAuthenticationStrategy {
    [delegate to many composited SessionAuthenticationStrategys]
}

This is a change I'm suggesting.

...
        SessionFixationProtectionScheme scheme =
            SessionFixationProtectionScheme.getFromNamespaceValue(namespaceValue);
        scheme.checkConfigurationValid(servletContext);
        SessionFixationProtectionSchemeStrategy strategy = new SessionFixationProtectionSchemeStrategy();
        strategy.setSessionFixationProtectionScheme(scheme);
...

I think that just about covers it.

[spring-projects-issues]

Nick Williams said:

Just realized it may not be obvious from the pseudo-code, but the new SessionFixationProtectionSchemeStrategy class has NEITHER a migrateSessionAttributes property NOR a retainedAttributes property, because they aren't needed. That has been replaced completely by the class and enum.

[spring-projects-issues]

Rob Winch said:

Just so you know historically speaking code I can pull into an IDE gets looked at faster than design docs..even if it has lots of TODOs in it.

[spring-projects-issues]

Nick Williams said:

I have begun work on this in https://github.com/beamerblvd/spring-security/tree/SEC-2135.

It doesn't appear that I have the ability to change the status of this to "In Progress." If it matters to anyone, feel free to change the status.

[spring-projects-issues]

Nick Williams said:

Pull request 35 submitted to complete this new feature.