Repository navigation
[#1107] Fall back to memory locking when counter writes fail - #1216
Conversation
maximthomas
left a comment
There was a problem hiding this comment.
praise: The fallback state is published and updated carefully.
ISAccountLockout.useMemoryLockingsets the fallback duration beforeputIfAbsentpublishes it, so a concurrent request never counts against a zero duration.- The store-catch retry recomputes from the shared state; removing that
synchronizedturnssimultaneousFirstWriteFailuresPreserveEveryAttemptred 153 of 200 runs. AMAccountLockout.isLockedOut(AMIdentity)checks the administrator's physical lock before the memory counter, pinned byfallbackStillHonorsAnAdministrativelyDisabledAccount.
issue (blocking): For an identity in fallback, the lockout mail and the plugin callback run while the shared per-identity monitor is held.
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:224-227, :290-295, :309-333
For a fallback identity, invalidPasswd runs recordInvalidPasswd inside synchronized (memoryInfo). At fail_count == failureLockoutCount that block calls sendLockOutNotice, which is a synchronous AMSendMail.postMail, and processedAccounttLockout. AMSendMail's session sets only mail.smtp.host/port (AMSendMail.java:55-56, :144), so no connect or read timeout applies. getAcInfo (:375-380), resetLockoutAttempts (:756-760) and AMAccountLockout.checkMemoryLockout (:332-342) take the same monitor. The trigger needs three conditions: an identity in fallback (every identity on a read-only store, the #1107 case), a lockout-notification address, and an SMTP server that accepts the connection but never answers. Then every later login for that user name, including the AMLoginContext:611 pre-check, parks a container thread on the monitor. With no timeout set, nothing bounds the wait. At BASE no shared lock was held while the mail was sent, so the worst case was one stuck thread per lockout cycle. ISAccountLockout is created per AMAccountLockout (AMAccountLockout.java:99), which is created per request (AMLoginContext.java:610, :673), so an instance field is request-local. inactivateUserAccount (:310-312) can stay where it is: it runs only for identities that are not in fallback. The store-catch arm needs no change, because its synchronized block has ended by the time control returns to invalidPasswd. The invalidPasswd(AMIdentity, AccountLockoutInfo) overload at :486 takes no shared monitor.
private long lockoutEventAt;
public int invalidPasswd(String userDN, String userName, AMIdentity amIdentity, AccountLockoutInfo acInfo) {
AccountLockoutInfo memoryInfo = memoryLockouts.get(userDN);
int warning;
if (memoryInfo != null) {
synchronized (memoryInfo) {
warning = recordInvalidPasswd(userDN, userName, amIdentity, memoryInfo, false);
}
} else {
warning = recordInvalidPasswd(userDN, userName, amIdentity, acInfo, storeInvalidAttemptsInDS);
}
notifyLockout(userName, amIdentity); // no shared monitor is held here
return warning;
}
// recordInvalidPasswd, in place of the try block at :313-333:
// lockoutEventAt = now;
private void notifyLockout(String userName, AMIdentity amIdentity) {
long at = lockoutEventAt;
if (at <= 0) {
return;
}
lockoutEventAt = 0;
try {
sendLockOutNotice(userName);
callbackImpl = AMAuthCallBackImpl.getInstance(amIdentity.getRealm());
callbackImpl.processedAccounttLockout(at, userName);
} catch (AMAuthCallBackException e) {
if (debug.getState() >= IDebug.ERROR) {
debug.error("ISAccountLockout invalidPasswd : error getting callback implementation "
+ "instance or error from callback module", e);
}
} catch (Exception ex) {
debug.message("Error activating account/sending notification ", ex);
}
}question (non-blocking): Was one transient write error on a writable store meant to keep the identity in server-local memory until restart?
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:340-358, :375-380, :310; openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:317-318
The code never removes an entry from memoryLockouts, and getAcInfo returns that entry before it reads the store. Take physical mode (duration 0) and one transient IdRepoException on a counter or reset write. Until restart, that identity then gets a 30-minute memory lock each time it reaches the threshold, never inetuserstatus=Inactive, so an attacker gets the threshold again every 30 minutes. Setting the user Active or clearing sunAMAuthInvalidAttemptsData no longer lifts the lock. The body names the per-server counters; it does not name either of these effects. If fallback is only for stores that keep rejecting writes, this becomes blocking: a later successful persisted write, or the reset after a successful login, should then remove the entry. Either way, the upgrade notes (chap-upgrade-servers.adoc) should say that an administrator cannot unlock a fallback identity.
question (non-blocking): Should anyone be able to lock amadmin for the lockout duration in the default configuration?
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:290-295; openam-core/src/main/java/com/sun/identity/authentication/service/AuthD.java:1171-1172; openam-core/src/main/java/com/sun/identity/idm/plugins/internal/SpecialRepo.java:639-645
The counter is written through AuthD.getIdentity, which uses the AdminTokenAction (dsameuser) session. SpecialRepo.setAttributes rejects every token except amadmin's, so amadmin's write always throws. BASE returned -1 and never locked amadmin. HEAD moves amadmin into fallback, so anyone who posts the threshold number of wrong passwords locks it for the lockout duration (30 minutes with the amAuth.xml default). The body's "special identities follow the same fallback policy" seems to intend this, so it is non-blocking as stated. Operators only read the upgrade notes, so put it in chap-upgrade-servers.adoc, not only in the PR body. Not run against a server.
question (non-blocking): Are the threshold-1 and multiplier-0 changes meant for every identity, not only fallback ones?
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:259, :792
The window conjunct is gone from the lock condition, and the expiry reset skips a 0 multiplier, on every path, including writable stores and configured memory mode. With threshold 1, BASE locked at the second failure and HEAD locks at the first. With multiplier 0, BASE's expiry reset persisted duration 0, so no later cycle ever locked; HEAD keeps the duration. Both look like fixes. However, docs/superpowers/specs/2026-11-03-account-lockout-fallback-design.md:25 and the plan's :23 say that writable stores and explicit memory mode "retain their behavior". If the changes are intended, say so in the body and the upgrade notes, fix those two lines, and pin them (next comment). If not, confine both changes to fallback identities.
suggestion (non-blocking): No test pins the threshold-1 and multiplier-0 changes for identities whose store accepts writes.
openam-core/src/test/java/com/sun/identity/authentication/service/AMAccountLockoutTest.java:156-174; openam-core/src/test/java/com/sun/identity/common/ISAccountLockoutTest.java:314-315
In AMAccountLockoutTest, store() always throws (:74), and ISAccountLockoutTest.lockout() hard-codes count 5 and multiplier 1. Two mutants keep the BASE arithmetic wherever memoryLockouts lacks the userDN, and both leave the suite green (427/427). Both rows below fail at BASE and under both mutants (not run):
private ISAccountLockout lockout(long minutes, int count, int multiplier) {
return new ISAccountLockout(true, MINUTES.toMillis(5), count, null, 3, null, null,
MINUTES.toMillis(minutes), multiplier, ATTRIBUTE, "amAuth");
}
@Test
public void writableStoreLocksOnTheFirstFailureWhenThresholdIsOne() {
rejectWrites = false;
ISAccountLockout lockout = lockout(10, 1, 1);
lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
assertTrue(lockout(10, 1, 1).isLockedOut(lockout(10, 1, 1).getAcInfo(userDN, identity)));
assertEquals(writes, 1);
}
@Test
public void writableStoreKeepsTheDurationWhenTheMultiplierIsZero() {
rejectWrites = false;
for (int i = 0; i < 2; i++) {
ISAccountLockout lockout = lockout(10, 2, 0);
lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
}
FrozenTimeService.INSTANCE.fastForward(11, MINUTES);
ISAccountLockout reset = lockout(10, 2, 0);
reset.resetLockoutAttempts(userDN, identity, reset.getAcInfo(userDN, identity), false);
assertEquals(lockout(10, 2, 0).getAcInfo(userDN, identity).getActualLockoutDuration(), MINUTES.toMillis(10));
}suggestion (non-blocking): No test pins the synchronized (memoryInfo) around fallback increments.
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:224-227; openam-core/src/test/java/com/sun/identity/common/ISAccountLockoutTest.java:200-226
staleSnapshotsDoNotLoseConcurrentFallbackAttempts publishes the fallback entry before its threads start, so it depends on four short read-modify-writes colliding. With the synchronized removed, the row survived all 200 invocations.
Pin: make the collision deterministic. ISAccountLockout.debug is already a mock; stub its messageEnabled() to park only the first worker thread on a latch (it is called between reading fail_count and writing it back). Then start a second invalidPasswd on the same identity, wait about 200 ms, release the latch, and assert that the count grew by 2. Without the monitor, the count grows by 1.
suggestion (non-blocking): No test row uses a lockout multiplier above 1.
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:251, :259-263
The PR moves the multiplied duration into the local actualLockoutDuration, which keeps the store-catch retry from multiplying twice. Every test row uses multiplier 1 or 0, so deleting :261 leaves the suite green (427/427), and so does going back to mutating acInfo before the store. This row, which uses the helper above, fails on both mutants: it gets 10 minutes without :261 and 40 with the pre-store mutation (not run).
@Test
public void multiplierAppliesOnceWhenTheThresholdWriteFails() {
rejectWrites = false;
for (int i = 0; i < 5; i++) {
ISAccountLockout lockout = lockout(10, 5, 2);
lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
}
FrozenTimeService.INSTANCE.fastForward(11, MINUTES);
for (int i = 0; i < 4; i++) {
ISAccountLockout lockout = lockout(10, 5, 2);
lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
}
rejectWrites = true;
ISAccountLockout lockout = lockout(10, 5, 2);
lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
assertEquals(lockout(10, 5, 2).getAcInfo(userDN, identity).getActualLockoutDuration(), MINUTES.toMillis(20));
}suggestion (non-blocking): checkMemoryLockout holds the shared monitor during resetPasswdLockout's directory lookup.
openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:332-342, :194
On an expired fallback lock, resetPasswdLockout calls AuthD.getAuth().getIdentity inside synchronized (acInfo), and acInfo here is the shared memoryInfo. Once per lock cycle, every other request for that identity waits on a lookup against the very store whose failures caused the fallback. Resolve amIdentity and the key before you take the monitor; isMemoryLockout and isAccountLocked(String) already have both. Then, under the monitor, call isAccountLockout.resetLockoutAttempts(userDN, amIdentity, acInfo, false) and set warnUser = 0. The key must be the one resetPasswdLockout computes: normalizeDN(token) when storeInDS is false. delayedExpiryResetCannotEraseANewLockoutCycle swallows its TimeoutException, so it passes either way.
suggestion (non-blocking): isLockedOut(String) now reads the directory on every login in realms with lockout duration 0.
openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:246
memoryLocking is true whenever the duration is above 0 (ISAccountLockout.java:151-152), so the default configuration already paid for this lookup at BASE. Realms with duration 0 and sunStoreInvalidAttemptsInDS at its default (true) now pay one AuthD.getIdentity and one attribute read per AMLoginContext:611 check and per LoginState lockout call. The result is still false unless the identity is in fallback. Skip the lookup when lockout is disabled, and when no identity is in fallback (a static check that memoryLockouts is non-empty).
suggestion (non-blocking): No AMAccountLockout test runs in configured memory mode.
openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:246, :317-318
Every AMAccountLockoutTest row sets storeInDS=true and makes store() throw. Two mutants therefore survive: one reduces :246 to getStoreInvalidAttemptsInDS(), so a locked memory-mode user with the right password gets in; the other adds && getStoreInvalidAttemptsInDS() at :317. This row kills both (not run). The fresh name keeps the static loginFailHash entry out of other rows:
@Test
public void configuredMemoryModeLocksWithoutTheStore() {
when(state.getLoginFailureLockoutStoreInDS()).thenReturn(false);
when(state.getLoginFailureLockoutDuration()).thenReturn(MINUTES.toMillis(10));
String name = "memory-" + UUID.randomUUID();
for (int i = 0; i < 5; i++) {
new AMAccountLockout(state).invalidPasswd(name);
}
assertTrue(new AMAccountLockout(state).isLockedOut(name));
assertTrue(new AMAccountLockout(state).isAccountLocked(name));
}suggestion (non-blocking): No test pins the design doc's rule that "a read failure alone does not activate fallback".
openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:393-395
The code is right. However, the only row with a failing read also rejects the write. A mutant that publishes the fresh acInfo in recordInvalidPasswd's null branch, and still writes, stays green. This row kills it (not run):
@Test
public void readFailureAloneKeepsPersisting() throws Exception {
rejectWrites = false;
when(identity.getAttribute(anyString()))
.thenThrow(new IdRepoException("Unavailable"))
.thenAnswer(call -> stored.get(call.getArgument(0)));
failAttempt(10);
failAttempt(10);
assertEquals(writes, 2);
}suggestion (non-blocking): The physical-lock rows run only with duration 0, so no test pins the timed-mode branch at :311.
openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:311
fallbackStillHonorsACustomPhysicalLock and fallbackStillHonorsAnAdministrativelyDisabledAccount use duration 0. Every timed-mode row uses an active identity, so changing :311 to always run the physical check survives.
Pin: add a row with duration above 0 and identity.isActive() false. Fail five times, let the lock expire with FrozenTimeService.fastForward, and assert the isAccountLocked("alice") outcome you intend (false is BASE parity).
note (non-blocking): Files outside the stated scope; the body does not mention them.
docs/superpowers/plans/2026-11-03-account-lockout-fallback.md— agent-workflow plandocs/superpowers/specs/2026-11-03-account-lockout-fallback-design.md— agent-workflow spec
7e8a342 to
b5518f4
Compare
|
Addressed the review in this update.
Validation on Java 17: all 236 standard openam-core test classes passed, with 2,302 tests and no failures, errors, or skips, including all 44 lockout cases. Before the fixes, seven new cases failed for the expected notification/lookup/mode problems. Six isolated mutations were detected: missing increment serialization, missing atomic expiry/reset, the old threshold-one condition, the old zero-multiplier reset, missing duration multiplication, and pre-store multiplication that doubles the duration on retry. git diff --check also passed. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The round-1 blocker is fixed, and the new regression cases are real pins.
- Lockout mail and the plugin callback now run in
notifyLockoutafter the counter monitor is released (ISAccountLockout.java:335-353). - The seven cases the description names fail against the previous commit's main code, and the other 37 pass (measured: 44 run, 7 failed). Those seven are the four
blockedNotificationDoesNotBlockCounterReadersrows plusexpiryResetReusesResolvedIdentityOutsideTheCounterMonitor,physicalModeWithoutFallbackDoesNotReadTheDirectoryanddisabledLockoutDoesNotReadTheDirectoryEvenWithFallback. - The failure path checks the lock and counts the attempt under the same
normalizeDN(IdentityUtils.getDN(..))key (AMAccountLockout.java:121-124,:302-305).
question (non-blocking): When lockout is disabled, should an existing lock stop being enforced? Right now the answer depends on the auth index type.
openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:242-244, :321
The early return added in this round gates isLockedOut(String) only. isAccountLocked(String) still runs checkMemoryLockout, and no caller gates it (AMLoginContext.java:611, :674, LoginState.java:3915).
Take a lockout that is turned off (mode off, or count 0) while a timed DS lock or a fallback lock has not yet expired:
- A correct-password login with
authIndexType=usernow succeeds, becauseAMLoginContext:611is its only lock check. Before this PR it was rejected. - The same login through the default org/service chain is still rejected at
LoginState.java:2554→isAccountLocked. - A wrong password still reports the user as locked.
The early return implements the previous round's suggestion ("skip the lookup when lockout is disabled"). That suggestion did not say the result would change.
This is minor either way, since nothing is throttled once lockout is off. It becomes major only if a lock must outlive disabling lockout, because then the USER-index path regressed.
If locks are meant to be released, gate the memory branch of isAccountLocked(String) too. Leave the physical branch (:315-320) as it is, so an administrator's inactivation still holds:
if (!locked && acInfo != null && isAccountLockout.isLockoutEnabled()
&& isAccountLockout.isMemoryLocking(userDN)) {
locked = checkMemoryLockout(userDN, amIdentity);
}Or, if locks must outlive disabling lockout: drop the early return at :242-244 and rely on the hasMemoryLockouts() gate at :249-250 to avoid the lookup.
suggestion (non-blocking): No test checks that lockout mail and the plugin callback still fire when the threshold write succeeds.
openam-core/src/test/java/com/sun/identity/common/ISAccountLockoutTest.java:333-367, openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:328
Only the four rows of blockedNotificationDoesNotBlockCounterReaders verify sendLockOutNotice and processedAccounttLockout (:362-363). Each row sets rejectWrites = true before the threshold attempt (:337), so the threshold is always reached on a persist=false path.
If lockoutEventAt = now (ISAccountLockout.java:328) ran only when !persist, both lockout test classes would stay green. Yet that change stops the mail and the plugin event for every healthy writable store, which is the normal production path. This was traced, not run as a mutant.
@Test
public void healthyWriteStillSendsTheLockoutNotification() {
rejectWrites = false;
ISAccountLockout lockout = spy(lockout(10));
doAnswer(call -> null).when(lockout).sendLockOutNotice("alice");
AMAuthCallBackImpl plugin = AMAuthCallBackImpl.getInstance("/");
for (int i = 0; i < 5; i++) {
lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
}
assertTrue(lockout.isLockedOut(lockout.getAcInfo(userDN, identity)));
verify(lockout, times(1)).sendLockOutNotice("alice");
verify(plugin, times(1)).processedAccounttLockout(any(Long.class), eq("alice"));
}Pin: this row fails if the :328 assignment is skipped on the persisted path. Configured memory mode (storeInvalidAttemptsInDS=false) has no notification row either, and that is the other half.
|
Addressed both follow-up points.
Validation on Java 17: all 236 standard openam-core test classes passed, with 2,327 tests and no failures, errors, or skips, including all 69 lockout cases. Before the fixes, 12 disabled-lockout cases and the configured-memory callback case failed as expected. Six isolated mutations were detected: skipped persisted notifications, skipped configured-memory notifications, missing disabled-mode check, bypassed physical restrictions, missing login realm, and incorrect callback-realm precedence. Independent final review found no actionable findings, and |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The follow-up settles both round-2 points, and the tests for them fail on the previous head.
AMAccountLockout.isAccountLockednow checksisLockoutEnabled()before its memory branch, so turning the mode off or setting the count or interval to 0 stops the temporary lock in both checks. All 12disablingLockoutStopsBothTimedChecksWithoutClearingTheCounterrows fail when run against b5518f4's main code.ISAccountLockout.notifyLockoutfalls back to the login realm (openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:130passesloginState.getOrgDN()). Configured in-memory counters now reach the callback plugins instead of failing on a null identity. The 3configuredMemoryModeNotifiesTheLoginRealmWithoutResolvingAnIdentityrows fail on b5518f4.- Both policy changes are documented in
chap-upgrade-servers.adoc.
A failed account-lockout counter write currently stops counting subsequent failures. Preserve the current attempt and switch only the affected identity to server-local memory locking across authentication requests. Keep positive lockout durations; use 30 minutes when the configured duration is zero.
Fallback covers counter increments and resets, retains persisted history, and logs the transition once per identity. Even a transient write failure keeps that identity in fallback until server restart; successful authentication resets the counter without restoring persistence. Counters are per server and are lost on restart. Setting the directory account status to Active or clearing the persisted counter does not clear a fallback lock. Existing physical account restrictions in physical-locking mode remain effective.
Special identities follow the same policy without name-based exemptions. With the current default lockout settings, five incorrect passwords can now temporarily lock amadmin for 30 minutes. Upgrade notes describe these operational changes.
Keep identity keys consistent across aliases and realms. Serialize fallback increments and expiry resets while sending lockout mail and invoking plugins outside the shared monitor. Reuse the resolved identity for expiry resets, and avoid directory lookups in the timed-lock pre-check when lockout is disabled or physical mode has no fallback counters.
Disabling lockout, or setting its failure count or failure interval to zero, stops enforcing temporary locks in both account-lock checks, consistently across authentication index types. This preserves the counters: re-enabling lockout before expiry enforces the existing temporary lock again. Physical account and profile restrictions retain their existing checks.
Lockout mail and plugin callbacks remain covered for healthy writable stores and configured memory counters. Configured memory mode also fixes an existing skipped-callback path: pass the login realm when no identity is resolved, without adding an identity-store lookup. When an identity is available, continue using its realm. Preserve the existing four-argument API.
Two related corrections also apply to writable stores and configured memory mode: threshold 1 starts a timed lock on the first failure, and multiplier 0 preserves the duration for subsequent lock cycles.
Validation:
Fixes #1107