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.

Sign up for GitHub

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

FISH-766 Improper session synchronization of session map #4479

Merged
merged 8 commits into from
Nov 18, 2020
Merged

FISH-766 Improper session synchronization of session map #4479

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d6e7cd
#4280 Fixed synchronization issue
sgflt Feb 9, 2020
3215ce9
#4281 Added final modifier to field
sgflt Feb 9, 2020
6c64922
Merge remote-tracking branch 'upstream/master' into issue-4280-improp…
sgflt Jul 19, 2020
b298f29
#4280 Replaced HashMap by ConcurrentHashMap in SingleSignOn
sgflt Jul 19, 2020
a00f866
#4280 clean up code in GlassFishSingleSignOn
sgflt Jul 19, 2020
1754479
#4280 Fixed unwanted double expiration
sgflt Jul 19, 2020
dc43453
#4280 Deleted unused variable
sgflt Jul 19, 2020
bef9a82
#4280 Fixed contract violation of findSessions
sgflt Jul 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 15 additions & 18 deletions appserver/web/web-core/src/main/java/org/apache/catalina/authenticator/SingleSignOn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,11 @@
*/
// Portions Copyright [2016-2019] [Payara Foundation and/or its affiliates]
package org.apache.catalina.authenticator;

import java.io.IOException;
import java.security.Principal;
import java.text.MessageFormat;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.logging.Level;

import javax.servlet.ServletException;
Expand Down Expand Up @@ -119,7 +118,7 @@ public class SingleSignOn extends ValveBase
* The cache of SingleSignOnEntry instances for authenticated Principals, keyed by the cookie value that is used to
* select them.
*/
protected Map<String, SingleSignOnEntry> cache = new HashMap<String, SingleSignOnEntry>();
protected final ConcurrentMap<String, SingleSignOnEntry> cache = new ConcurrentHashMap<>();

/**
* The lifecycle event support for this component.
Expand Down Expand Up @@ -434,9 +433,7 @@ protected void deregister(String ssoId, Session session) {

// see if we are the last session, if so blow away ssoId
if (sso.isEmpty()) {
synchronized (cache) {
cache.remove(ssoId);
}
this.cache.remove(ssoId);
}
}

Expand All @@ -449,17 +446,21 @@ protected void deregister(String ssoId, Session session) {
* @param username Username used to authenticate this user
* @param password Password used to authenticate this user
*/
protected void register(String ssoId, Principal principal, String authType, String username, char[] password, String realmName) {

protected void register(
final String ssoId,
final Principal principal,
final String authType,
final String username,
final char[] password,
final String realmName
) {
if (debug >= 1) {
String msg = MessageFormat.format(rb.getString(LogFacade.REGISTERING_SSO_INFO),
new Object[] { ssoId, principal.getName(), authType });
log(msg);
}
synchronized (cache) {
cache.put(ssoId, new SingleSignOnEntry(ssoId, 0L, principal, authType, username, realmName));
}

this.cache.put(ssoId, new SingleSignOnEntry(ssoId, 0L, principal, authType, username, realmName));
}

// ------------------------------------------------------ Protected Methods
Expand Down Expand Up @@ -501,12 +502,8 @@ protected void log(String message, Throwable t) {
*
* @param ssoId Single sign on identifier to look up
*/
protected SingleSignOnEntry lookup(String ssoId) {

synchronized (cache) {
return cache.get(ssoId);
}

protected SingleSignOnEntry lookup(final String ssoId) {
return this.cache.get(ssoId);
}

/**
Expand Down
39 changes: 15 additions & 24 deletions ...ver/web/web-glue/src/main/java/com/sun/enterprise/security/web/GlassFishSingleSignOn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,10 @@ protected void deregister(String ssoId) {
}
// S1AS8 6155481 END
// Look up and remove the corresponding SingleSignOnEntry
SingleSignOnEntry sso = null;
synchronized (cache) {
sso = (SingleSignOnEntry) cache.remove(ssoId);
}

if (sso == null)
final SingleSignOnEntry sso = this.cache.remove(ssoId);
if (sso == null) {
return;
}

// Expire any associated sessions
sso.expireSessions();
Expand All @@ -464,44 +461,38 @@ private void processExpires() {
long tooOld = System.currentTimeMillis() - ssoMaxInactive * 1000L;
// S1AS8 6155481 START
if (logger.isLoggable(Level.FINE)) {
logger.log(Level.FINE, LogFacade.SSO_EXPIRATION_STARTED, cache.size());
logger.log(Level.FINE, LogFacade.SSO_EXPIRATION_STARTED, this.cache.size());
}
// S1AS8 6155481 END
ArrayList<String> removals = new ArrayList<String>(cache.size() / 2);
final ArrayList<String> removals = new ArrayList<>(this.cache.size() / 2);

// build list of removal targets

// Note that only those SSO entries which are NOT associated with
// any session are elegible for removal here.
// any session are eligible for removal here.
// Currently no session association ever happens so this covers all
// SSO entries. However, this should be addressed separately.

try {
synchronized (cache) {

Iterator<String> it = cache.keySet().iterator();
while (it.hasNext()) {
String key = it.next();
SingleSignOnEntry sso = (SingleSignOnEntry) cache.get(key);
if (sso.isEmpty() && sso.getLastAccessTime() < tooOld) {
removals.add(key);
}
this.cache.forEach((ssoId, sso) -> {
if (sso.isEmpty() && sso.getLastAccessTime() < tooOld) {
removals.add(ssoId);
}
Copy link
Contributor Author

@sgflt sgflt Jul 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a little change in behavior.

Old implementation locked SSOs and created a list of expired SSOs. No one could create a new SSO.

New implementation uses what is available and does not block creation of a new SSO (which is not taken for expiration in given round). However probability of a new SSO being expired is so small so I think this change should be OK for a real world usage.

}
});

int removalCount = removals.size();
// S1AS8 6155481 START
if (logger.isLoggable(Level.FINE)) {
logger.log(Level.FINE, LogFacade.SSO_CACHE_EXPIRE, removalCount);
}
// S1AS8 6155481 END
// deregister any elegible sso entries
for (int i = 0; i < removalCount; i++) {
// deregister any eligible sso entries
for (final String removal : removals) {
// S1AS8 6155481 START
if (logger.isLoggable(Level.FINE)) {
logger.log(Level.FINE, LogFacade.SSO_EXPRIRATION_REMOVING_ENTRY, removals.get(i));
logger.log(Level.FINE, LogFacade.SSO_EXPRIRATION_REMOVING_ENTRY, removal);
}
deregister(removals.get(i));
deregister(removal);
}
// S1AS8 6155481 END
} catch (Throwable e) { // don't let thread die
Expand Down Expand Up @@ -608,7 +599,7 @@ protected void removeSession(String ssoId, Session session) {
* @return Number of sessions participating in SSO
*/
public int getActiveSessionCount() {
return cache.size();
return this.cache.size();
}

/**
Expand Down
56 changes: 27 additions & 29 deletions appserver/web/web-ha/src/main/java/org/glassfish/web/ha/authenticator/HASingleSignOn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,15 @@

import com.sun.enterprise.container.common.spi.util.JavaEEIOUtils;
import com.sun.enterprise.security.web.GlassFishSingleSignOn;

import org.apache.catalina.Session;
import org.apache.catalina.authenticator.SingleSignOnEntry;
import org.glassfish.ha.store.api.BackingStore;
import org.glassfish.ha.store.api.BackingStoreException;
import org.glassfish.web.ha.LogFacade;
import org.glassfish.web.ha.session.management.HAStoreBase;

import java.util.logging.Logger;
import java.util.logging.Level;
import java.security.Principal;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* @author Shing Wai Chan
Expand All @@ -72,27 +70,24 @@ public HASingleSignOn(JavaEEIOUtils ioUtils,
}

@Override
protected void deregister(String ssoId) {
protected void deregister(final String ssoId) {

//S1AS8 6155481 START
if (logger.isLoggable(Level.FINE)) {
logger.fine("Deregistering sso id '" + ssoId + "'");
}
//S1AS8 6155481 END
// Look up and remove the corresponding SingleSignOnEntry
SingleSignOnEntry sso = null;
synchronized (cache) {
sso = cache.remove(ssoId);
}

if (sso == null)
final SingleSignOnEntry sso = this.cache.remove(ssoId);
if (sso == null) {
return;
}

// Expire any associated sessions
sso.expireSessions();

try {
ssoEntryMetadataBackingStore.remove(ssoId);
this.ssoEntryMetadataBackingStore.remove(ssoId);
} catch(BackingStoreException ex) {
throw new IllegalStateException(ex);
}
Expand All @@ -110,18 +105,23 @@ protected void register(String ssoId, Principal principal, String authType,
+ "' with auth type '" + authType + "' and realmName '" + realmName + "'");
}

HASingleSignOnEntry ssoEntry = null;
synchronized (cache) {
ssoEntry = new HASingleSignOnEntry(ssoId, principal, authType,
username, realmName,
// revisit maxIdleTime 1000000, version 0
System.currentTimeMillis(), 1000000, 0,
ioUtils);
cache.put(ssoId, ssoEntry);
}
final HASingleSignOnEntry ssoEntry = new HASingleSignOnEntry(
ssoId,
principal,
authType,
username,
realmName,
// revisit maxIdleTime 1000000, version 0
System.currentTimeMillis(),
1000000,
0,
ioUtils
);

this.cache.put(ssoId, ssoEntry);

try {
ssoEntryMetadataBackingStore.save(ssoId, ssoEntry.getMetadata(), true);
this.ssoEntryMetadataBackingStore.save(ssoId, ssoEntry.getMetadata(), true);
} catch(BackingStoreException ex) {
throw new IllegalStateException(ex);
}
Expand Down Expand Up @@ -151,23 +151,21 @@ public void associate(String ssoId, long ssoVersion, Session session) {
}

@Override
protected SingleSignOnEntry lookup(String ssoId, long ssoVersion) {
protected SingleSignOnEntry lookup(final String ssoId, final long ssoVersion) {
SingleSignOnEntry ssoEntry = super.lookup(ssoId, ssoVersion);
if (ssoEntry != null && ssoVersion > ssoEntry.getVersion()) {
// clean the old cache
synchronized(cache) {
cache.remove(ssoId);
}
this.cache.remove(ssoId);
ssoEntry = null;
}

if (ssoEntry == null) {
// load from ha store
try {
HASingleSignOnEntryMetadata mdata =
ssoEntryMetadataBackingStore.load(ssoId, null);
final HASingleSignOnEntryMetadata mdata = this.ssoEntryMetadataBackingStore.load(ssoId, null);
if (mdata != null) {
ssoEntry = new HASingleSignOnEntry(getContainer(), mdata, ioUtils);
cache.put(ssoId, ssoEntry);
this.cache.put(ssoId, ssoEntry);
}
} catch(BackingStoreException ex) {
throw new IllegalStateException(ex);
Expand Down