-
Notifications
You must be signed in to change notification settings - Fork 6.1k
SEC-2002: Added events to notify of session ID change #30
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
Changes from all commits
06a1cb7
b40b122
5287353
830c2a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* | ||
* Copyright 2002-2012 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.security.web.authentication.session; | ||
|
||
import org.springframework.security.authentication.event.AbstractAuthenticationEvent; | ||
import org.springframework.security.core.Authentication; | ||
import org.springframework.util.Assert; | ||
|
||
/** | ||
* Indicates a session ID was changed for the purposes of session fixation protection. | ||
* | ||
* @since 3.2 | ||
* @see SessionFixationProtectionStrategy | ||
* @author Nick Williams <[email protected]> | ||
*/ | ||
public class SessionFixationProtectionEvent extends AbstractAuthenticationEvent { | ||
//~ Instance fields ================================================================================================ | ||
|
||
private final String oldSessionId; | ||
|
||
private final String newSessionId; | ||
|
||
//~ Constructors =================================================================================================== | ||
|
||
/** | ||
* Constructs a new session fixation protection event. | ||
* | ||
* @param authentication The authentication object | ||
* @param oldSessionId The old session ID before it was changed | ||
* @param newSessionId The new session ID after it was changed | ||
*/ | ||
public SessionFixationProtectionEvent(Authentication authentication, String oldSessionId, String newSessionId) { | ||
super(authentication); | ||
Assert.hasLength(oldSessionId); | ||
Assert.hasLength(newSessionId); | ||
this.oldSessionId = oldSessionId; | ||
this.newSessionId = newSessionId; | ||
} | ||
|
||
//~ Methods ======================================================================================================== | ||
|
||
/** | ||
* Getter for the session ID before it was changed. | ||
* | ||
* @return the old session ID. | ||
*/ | ||
public String getOldSessionId() { | ||
return this.oldSessionId; | ||
} | ||
|
||
/** | ||
* Getter for the session ID after it was changed. | ||
* | ||
* @return the new session ID. | ||
*/ | ||
public String getNewSessionId() { | ||
return this.newSessionId; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* | ||
* Copyright 2002-2012 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.security.web.authentication.session; | ||
|
||
import org.springframework.security.core.Authentication; | ||
|
||
/** | ||
* Indicates a session was migrated for the purposes of session fixation protection. | ||
* | ||
* @since 3.2 | ||
* @see SessionFixationProtectionStrategy | ||
* @author Nick Williams <[email protected]> | ||
*/ | ||
public class SessionFixationProtectionMigrationEvent extends SessionFixationProtectionEvent { | ||
//~ Instance fields ================================================================================================ | ||
|
||
private final boolean sessionAttributesMigrated; | ||
|
||
//~ Constructors =================================================================================================== | ||
|
||
/** | ||
* Constructs a new session migration event. | ||
* | ||
* @param authentication The authentication object | ||
* @param oldSessionId The old session ID before the session was migrated | ||
* @param newSessionId The new session ID after the session was migrated | ||
* @param sessionAttributesMigrated Whether or not all session attributes were migrated | ||
*/ | ||
public SessionFixationProtectionMigrationEvent(Authentication authentication, String oldSessionId, | ||
String newSessionId, boolean sessionAttributesMigrated) { | ||
super(authentication, oldSessionId, newSessionId); | ||
this.sessionAttributesMigrated = sessionAttributesMigrated; | ||
} | ||
|
||
/** | ||
* Getter that indicates whether all session attributes were migrated. If all session attributes were not migrated | ||
* (due to the session fixation protection strategy being "new session"), the Spring Security-related session | ||
* attributes were still migrated, regardless. | ||
* | ||
* @return {@code true} if all session attributes were migrated, {@code false} otherwise. | ||
*/ | ||
public boolean sessionAttributesWereMigrated() { | ||
return this.sessionAttributesMigrated; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
|
||
import org.apache.commons.logging.Log; | ||
import org.apache.commons.logging.LogFactory; | ||
import org.springframework.context.ApplicationEventPublisher; | ||
import org.springframework.context.ApplicationEventPublisherAware; | ||
import org.springframework.security.core.Authentication; | ||
import org.springframework.util.Assert; | ||
|
||
|
@@ -36,9 +38,12 @@ | |
* @author Luke Taylor | ||
* @since 3.0 | ||
*/ | ||
public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy { | ||
public class SessionFixationProtectionStrategy implements SessionAuthenticationStrategy, | ||
ApplicationEventPublisherAware { | ||
protected final Log logger = LogFactory.getLog(this.getClass()); | ||
|
||
private ApplicationEventPublisher applicationEventPublisher; | ||
|
||
/** | ||
* Indicates that the session attributes of an existing session | ||
* should be migrated to the new session. Defaults to <code>true</code>. | ||
|
@@ -112,12 +117,22 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r | |
/** | ||
* Called when the session has been changed and the old attributes have been migrated to the new session. | ||
* Only called if a session existed to start with. Allows subclasses to plug in additional behaviour. | ||
* <p> | ||
* The default implementation of this method publishes a {@link SessionFixationProtectionMigrationEvent} to notify | ||
* the application that the session ID has changed and about which attributes were migrated. If you override this | ||
* method and still wish these events to be published, you should call {@code super.onSessionChange()} within your | ||
* overriding method. | ||
* | ||
* @param originalSessionId the original session identifier | ||
* @param newSession the newly created session | ||
* @param auth the token for the newly authenticated principal | ||
*/ | ||
protected void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too convinced on the naming conventions of the two classes being published. The reason is that there are many reasons a session would change, which are not related to session fixation. The name SessionIdChangedEvent does not take this into account (although the Javadoc does). I think perhaps a better approach is creating a single SessionFixationProtectionEvent which contains a boolean to indicate if the parameters were migrated. This ensures the event matches closely with why it is being fired. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to make the event hierarchy clean while also not making my job to implement https://jira.springsource.org/browse/SEC-2135 harder. How about these two options. OPTION 1 Like you said, the session ID could be changed for many reasons (but really? is the session ID ever changed anywhere else in Spring Security?), so this could actually mask that. If someone every wanted to change the session ID for some reason OTHER than session fixation, they would need a whole new event. OR, we could do this instead: OPTION 2 I think maybe I like that one the best. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the class SessionIdChangedEvent either as that can happen for any number of reasons. I don't want to imply that it will be linked to the HttpServletRequest#changeSessionId() method. Can we stick with SessionFixationProtectionEvent for a non-migration event and a SessionMigrationProtectionEvent (extends SessionFixationProtectionEvent) for a migration event? I'd really like to nail down an event that captures only instances we intend to fire the event on and in which we guarantee it will happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if someone ever needs to change an session ID for some other reason they can implement SessionIdChangeEvent separate from the events we are creating here. Okay. Final proposal. I think you'll be okay with it:
Notice I changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that my proposal was awkward phrasing...I like your proposal better than mine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why can you have more than one Here's the best concrete example I can think of: I create an application that has both an external and internal portal. I want them to be the same deployment because they share the same database, resources, repositories, services, etc., but they interact with the public differently than with employees. Therefore, there are two login screens, two user databases and two sets of permissions. Instead of using permissions to differentiate these users, I want two completely different security contexts, because I want different rules on login retry, remember-me, SSL, session concurrency, etc. This is a perfectly plausible, possible need, and using multiple
My suggestions are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Supporting multiple elements allows for protecting a single application that acts as both a service and a web application differently. For example, you may expose the same endpoint that delivers HTML to the browser and JSON to services. Authentication is likely to differ quite a bit when dealing with such situations. In this instance, multiple elements are very useful. You can see this approach is used quite heavily within the Spring Security OAuth applications.
This is true, but what value is there in stating in how the session fixation protection event was performed? From a security standpoint, what would a developer want to gain from this? In both instances we are stating that session fixation protection was performed.
Spring Security will never solve all cases, but should allow others to extend it to meet edge cases. Before we add both events I think we need to be able to answer why would being able to distinguish between the two events benefit an application from a security standpoint. I'm not saying that there isn't an answer to why...just that off the top of my head I cannot think of any (perhaps you have an example). Another point is if we need to, we can easily make the changes passively (one event extends another). However, if we add it now we cannot remove it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gonna be honest with you, Rob, I'm a little frustrated. I've put in considerable time in this, and at this point I'm not sure it's ever going to get accepted. I've never had this much trouble contributing to a project before... I really think this is the best design, and I see exactly zero downsides to it. I gave you the best example I can think of (user portal vs. employee portal). If that's just not going to be good enough for you, let me know now, because I'm going to have to essentially scrap it and do it again. Unfortunately, I don't think I will have time for that for a long time. I've fallen behind on the book because I was sick for 2 weeks, and it has to be my #1 priority right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My biggest complaint about this entire process is how the requirements keep changing. I understand a little bit of back and forth to get things right, as it should be for such an enterprise project, but I expect the "game changers" to come early in the process...not over a month later. I really wish you had said something about not wanting separate events the first day you reviewed it--when all indications were I just needed a few minor tweaks to make it acceptable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm sorry for your frustration.
It is important we get things right. The explanation you provided specified "how" two different session fixation protection strategies could be used, but it did not clarify:
We want to ensure that we can answer these questions before adding SessionFixationProtectionMigrationEvent to the code base. Additionally, we want to be convinced that the examples are required for a majority of users. If there is a reason that a small minority of users might need this, then we can ensure we provide hooks to allow customization to support the edge cases. It is important that we justify the addition of code to keep the code base as simple to maintain and understand as possible. If we cannot provide reasons at this time, just let me know and we can leave SessionFixationProtectionMigrationEvent out. If we come up with a reason later, there is a very easy way to introduce SessionFixationProtectionMigrationEvent passively.
I don't think you need to scrap what you have. The feedback I have provided would require the removal of SessionFixationProtectionMigrationEvent and modify references to it to point to SessionFixationProtectionEvent.
I did not want to make these changes without providing you the opporitunity to provde the answers to my questions in the event there is a reason to include SessionFixationProtectionMigrationEvent. If we cannot come up with any reasons and you are busy, let me know and I would be glad to make the changes for you.
Both requirements and implementations can and will change...isn't that why the Agile Manifesto exists? You can find a few examples of changing requirements/implementations within the Spring portfolio below:
Perhaps I am misunderstanding what you mean by "game changers", but I do not view the removal of SessionFixationProtectionMigrationEvent as a "game changer".
I empathize with your frustration. While I would like to pretend I get everything right the first time, I (like you) am human and overlook things. As with the other examples I provided, it can take time to get code to be production ready and as we recognize ways we can improve the code we should make the necessary updates to the code. In short, I think it is important to make the code the best we can even though these questions did not get asked until after a few revisions. |
||
if(applicationEventPublisher != null) { | ||
applicationEventPublisher.publishEvent(new SessionFixationProtectionMigrationEvent( | ||
auth, originalSessionId, newSession.getId(), migrateSessionAttributes | ||
)); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -193,6 +208,10 @@ public void setMigrateSessionAttributes(boolean migrateSessionAttributes) { | |
this.migrateSessionAttributes = migrateSessionAttributes; | ||
} | ||
|
||
public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { | ||
this.applicationEventPublisher = applicationEventPublisher; | ||
} | ||
|
||
/** | ||
* @deprecated Override the {@code extractAttributes} method instead | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change the class to initialize applicationEventPubisher to a do nothing implementation like NullEventPublisher in ProviderManager to ensure passivity. While this implements ApplicationEventPublisherAware, applicationEventPublisher may not be initialized if a user instantiates an instance of SessionFixationProtectionStrategy outside of the Spring container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: Please add a test for this change (i.e. instantiate SessionFixationProtectionStrategy manually and ensure the onSessionChange method does not throw a NullPointerException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Will make this change and update. Do I just make another commit to my fork and then submit a new pull request with both commits in it? Or add my new commit to this pull request? I'm very new to this Git thing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since no one is building on your repository, you can just make the changes and amend the commit:
Then you can force push (typically bad, but since no one is using your fork it is ok). Assuming origin is pointed at your repo it would look like
Note: typically it is best to create a branch for any work you do. So in your instance you would have wanted to create a branch named SEC-2002 to do all your work on. If you read the contributor guidelines it talks about this and gives some links. I don't view this as critical, but some projects may be more strict about this than others. It certainly helps if you need to rebase (although if you know what you are doing you can still rebase with no problems)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I couldn't figure out the difference between fork and clone and branch. I wasn't sure whether I was doing what I was supposed to or not. Do I have to fork spring-security and THEN branch my fork? Or can I just branch directly from spring-security?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of forking and branching doesn't matter, but it is typically easier to:
git checkout -b SEC-2002
git add .; git commit -m 'SEC-2002: did some things'
git push origin SEC-2002
You might read the contributor guidelines which points to github doc. Alternatively, this is a rather streamlined article that might help get you going http://blog.springsource.org/2010/12/21/social-coding-in-spring-projects/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rob, there is no such thing as
NullEventPublisher
in Spring Framework or Spring Security. There are no implementations ofApplicationEventPublisher
that I can find in either framework that do what you're suggesting. Additionally, Spring Security classesAbstractSecurityInterceptor
,AbstractJaasAuthenticationProvider
and many others in Spring Security initialize theirApplicationEventPublisher
s to null.However, what they DO all do is check to make sure the event publisher isn't null before publishing an event. I can do that to ensure that a NPE is never thrown, and add an appropriate test as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is private, but NullEventPublisher does exist. You will want to copy the implementation and keep it private. It can be found inside Eclipse use Shift+Ctrl+T and inside IntelliJ use Ctrl+N You can also enter the following search into github to find it
@SpringSource/spring-security NullEventPublisher
The concern here is passivity. As the code stands today if I created SessionFixationProtectionStrategy outside of the Spring container I would not need to inject an ApplicationEventPublisher for it to work. With these changes I would need to inject it to prevent a NullPointerException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the new code for this in the new commit below:
Should never result in a NPE. Is that sufficient, or is the
NullEventPublisher
really needed here for something I'm not seeing?