Skip to content

Commit d427ab3

Browse files
ChadSikorraChadSikorra
committed
Make binds atomic with a lock so we avoid (password policy) race conditions.
1 parent 5b13463 commit d427ab3

16 files changed

Lines changed: 241 additions & 173 deletions

‎src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/PdoReplicaPasswordStateStore.php‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,23 @@ public function load(Dn $dn): ReplicaPasswordState
4949
: ReplicaPasswordState::empty();
5050
}
5151

52-
public function apply(
52+
/**
53+
* @param callable(ReplicaPasswordState): OperationalChanges $merge
54+
*/
55+
public function atomicMutate(
5356
Dn $dn,
54-
OperationalChanges $changes,
57+
callable $merge,
5558
): void {
56-
if ($changes->isEmpty()) {
57-
return;
58-
}
59-
60-
$this->transactor->atomic(function () use ($dn, $changes): void {
59+
$this->transactor->atomic(function () use ($dn, $merge): void {
6160
$key = $this->key($dn);
62-
$next = $this->load($dn)->withChanges($changes);
61+
$current = $this->load($dn);
62+
$changes = $merge($current);
63+
64+
if ($changes->isEmpty()) {
65+
return;
66+
}
67+
68+
$next = $current->withChanges($changes);
6369

6470
$this->transactor
6571
->pdo()

‎src/FreeDSx/Ldap/Server/Backend/Write/SystemChange/LocalStateSystemChangeWriter.php‎

Lines changed: 0 additions & 38 deletions
This file was deleted.

‎src/FreeDSx/Ldap/Server/PasswordPolicy/Guard/BindStrategy/EntryBindStrategy.php‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,29 @@
1313

1414
namespace FreeDSx\Ldap\Server\PasswordPolicy\Guard\BindStrategy;
1515

16+
use FreeDSx\Ldap\Control\ControlBag;
17+
use FreeDSx\Ldap\Entry\Entry;
18+
use FreeDSx\Ldap\Server\Backend\Write\WritableLdapBackendInterface;
19+
use FreeDSx\Ldap\Server\Backend\Write\WriteContext;
1620
use FreeDSx\Ldap\Server\PasswordPolicy\Attempt\PasswordBindAttempt;
21+
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\OperationalChanges;
1722
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\PasswordPolicyOutcome;
23+
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\RecordedOutcome;
1824
use FreeDSx\Ldap\Server\PasswordPolicy\PasswordPolicyEngine;
1925
use FreeDSx\Ldap\Server\PasswordPolicy\UserPasswordState;
26+
use FreeDSx\Ldap\Server\Token\SystemToken;
2027

2128
/**
22-
* Evaluates every bind operation against the authoritative entry state (the primary / writable server).
29+
* Evaluates and records every bind against the authoritative entry state (the primary / writable server).
2330
*
2431
* @author Chad Sikorra <Chad.Sikorra@gmail.com>
2532
*/
2633
final readonly class EntryBindStrategy implements PasswordPolicyBindStrategyInterface
2734
{
28-
public function __construct(private PasswordPolicyEngine $engine) {}
35+
public function __construct(
36+
private PasswordPolicyEngine $engine,
37+
private WritableLdapBackendInterface $backend,
38+
) {}
2939

3040
public function preBindOutcome(PasswordBindAttempt $attempt): PasswordPolicyOutcome
3141
{
@@ -35,13 +45,28 @@ public function preBindOutcome(PasswordBindAttempt $attempt): PasswordPolicyOutc
3545
);
3646
}
3747

38-
public function failureState(PasswordBindAttempt $attempt): UserPasswordState
39-
{
40-
return $attempt->state;
41-
}
48+
public function record(
49+
PasswordBindAttempt $attempt,
50+
callable $decide,
51+
): RecordedOutcome {
52+
$recorded = null;
4253

43-
public function successState(PasswordBindAttempt $attempt): UserPasswordState
44-
{
45-
return $attempt->state;
54+
$this->backend->atomicUpdate(
55+
$attempt->dn,
56+
WriteContext::system(
57+
new SystemToken(),
58+
new ControlBag(),
59+
),
60+
function (Entry $entry) use ($decide, &$recorded): array {
61+
$recorded = $decide(UserPasswordState::fromEntry($entry));
62+
63+
return $recorded->changes->changes;
64+
},
65+
);
66+
67+
return $recorded ?? new RecordedOutcome(
68+
PasswordPolicyOutcome::allow(),
69+
OperationalChanges::none(),
70+
);
4671
}
4772
}

‎src/FreeDSx/Ldap/Server/PasswordPolicy/Guard/BindStrategy/PasswordPolicyBindStrategyInterface.php‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@
1515

1616
use FreeDSx\Ldap\Server\PasswordPolicy\Attempt\PasswordBindAttempt;
1717
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\PasswordPolicyOutcome;
18+
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\RecordedOutcome;
1819
use FreeDSx\Ldap\Server\PasswordPolicy\UserPasswordState;
1920

2021
/**
21-
* Supplies the password-policy state each bind operation is evaluated and recorded against.
22+
* Evaluates and atomically records each bind against the password-policy state that governs it.
2223
*
2324
* @author Chad Sikorra <Chad.Sikorra@gmail.com>
2425
*/
@@ -30,12 +31,13 @@ interface PasswordPolicyBindStrategyInterface
3031
public function preBindOutcome(PasswordBindAttempt $attempt): PasswordPolicyOutcome;
3132

3233
/**
33-
* The state a failed bind is counted and recorded against.
34+
* Atomically read the subject's current state, run $decide against it, persist the changes it yields, and return
35+
* the decision, all under an exclusive lock so concurrent binds cannot lose an update.
36+
*
37+
* @param callable(UserPasswordState): RecordedOutcome $decide
3438
*/
35-
public function failureState(PasswordBindAttempt $attempt): UserPasswordState;
36-
37-
/**
38-
* The state a successful bind is evaluated and cleared against.
39-
*/
40-
public function successState(PasswordBindAttempt $attempt): UserPasswordState;
39+
public function record(
40+
PasswordBindAttempt $attempt,
41+
callable $decide,
42+
): RecordedOutcome;
4143
}

‎src/FreeDSx/Ldap/Server/PasswordPolicy/Guard/BindStrategy/ReplicaBindStrategy.php‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,19 @@
1313

1414
namespace FreeDSx\Ldap\Server\PasswordPolicy\Guard\BindStrategy;
1515

16+
use FreeDSx\Ldap\Entry\Dn;
1617
use FreeDSx\Ldap\Server\PasswordPolicy\Attempt\PasswordBindAttempt;
18+
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\OperationalChanges;
1719
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\PasswordPolicyOutcome;
20+
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\RecordedOutcome;
1821
use FreeDSx\Ldap\Server\PasswordPolicy\PasswordPolicyEngine;
22+
use FreeDSx\Ldap\Server\PasswordPolicy\Replica\ReplicaPasswordState;
1923
use FreeDSx\Ldap\Server\PasswordPolicy\Replica\ReplicaPasswordStateStoreInterface;
2024
use FreeDSx\Ldap\Server\PasswordPolicy\UserPasswordState;
2125

2226
/**
23-
* Evaluates the worst of the replicated entry state and the replica-local bind state on a read-only replica.
27+
* Evaluates the worst of the replicated entry state and the replica-local bind state, and records failures/successes
28+
* to the replica-local store on a read-only replica.
2429
*
2530
* @author Chad Sikorra <Chad.Sikorra@gmail.com>
2631
*/
@@ -46,23 +51,46 @@ public function preBindOutcome(PasswordBindAttempt $attempt): PasswordPolicyOutc
4651
}
4752

4853
return $this->engine->evaluateLocalLockout(
49-
$this->localState($attempt),
54+
$this->store->load($attempt->dn)->toUserPasswordState($attempt->dn),
5055
$attempt->policy,
5156
);
5257
}
5358

54-
public function failureState(PasswordBindAttempt $attempt): UserPasswordState
55-
{
56-
return $this->localState($attempt);
59+
public function record(
60+
PasswordBindAttempt $attempt,
61+
callable $decide,
62+
): RecordedOutcome {
63+
$recorded = null;
64+
65+
$this->store->atomicMutate(
66+
$attempt->dn,
67+
function (ReplicaPasswordState $local) use ($attempt, $decide, &$recorded): OperationalChanges {
68+
$recorded = $decide($this->combine(
69+
$attempt->state,
70+
$local,
71+
$attempt->dn,
72+
));
73+
74+
return $recorded->changes;
75+
},
76+
);
77+
78+
return $recorded ?? new RecordedOutcome(
79+
PasswordPolicyOutcome::allow(),
80+
OperationalChanges::none(),
81+
);
5782
}
5883

5984
/**
60-
* Combine primary expiry/validity with replica-local volatile state so success clears local failures and grace.
85+
* Combine primary-authored entry fields (expiry / validity / must-change) with the replica-local volatile state so
86+
* the engine decides against the worst of both.
6187
*/
62-
public function successState(PasswordBindAttempt $attempt): UserPasswordState
63-
{
64-
$entry = $attempt->state;
65-
$local = $this->localState($attempt);
88+
private function combine(
89+
UserPasswordState $entry,
90+
ReplicaPasswordState $localState,
91+
Dn $dn,
92+
): UserPasswordState {
93+
$local = $localState->toUserPasswordState($dn);
6694

6795
return new UserPasswordState(
6896
changedAt: $entry->changedAt,
@@ -77,11 +105,4 @@ public function successState(PasswordBindAttempt $attempt): UserPasswordState
77105
lastSuccess: $local->lastSuccess,
78106
);
79107
}
80-
81-
private function localState(PasswordBindAttempt $attempt): UserPasswordState
82-
{
83-
return $this->store
84-
->load($attempt->dn)
85-
->toUserPasswordState($attempt->dn);
86-
}
87108
}

‎src/FreeDSx/Ldap/Server/PasswordPolicy/Guard/PasswordPolicyBindGuard.php‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@
1515

1616
use FreeDSx\Ldap\Control\PwdPolicyError;
1717
use FreeDSx\Ldap\Exception\OperationException;
18-
use FreeDSx\Ldap\Server\Backend\Write\SystemChange\SystemChangeWriterInterface;
1918
use FreeDSx\Ldap\Server\Clock\Sleeper\SleeperInterface;
2019
use FreeDSx\Ldap\Server\Logging\EventContext;
2120
use FreeDSx\Ldap\Server\Logging\EventLogger;
2221
use FreeDSx\Ldap\Server\Logging\ServerEvent;
2322
use FreeDSx\Ldap\Server\PasswordPolicy\Attempt\PasswordBindAttempt;
2423
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\PasswordPolicyOutcome;
24+
use FreeDSx\Ldap\Server\PasswordPolicy\Decision\RecordedOutcome;
2525
use FreeDSx\Ldap\Server\PasswordPolicy\Guard\BindStrategy\PasswordPolicyBindStrategyInterface;
2626
use FreeDSx\Ldap\Server\PasswordPolicy\PasswordPolicyContext;
2727
use FreeDSx\Ldap\Server\PasswordPolicy\PasswordPolicyEngine;
@@ -35,7 +35,6 @@
3535
public function __construct(
3636
private PasswordPolicyEngine $engine,
3737
private PasswordPolicyBindStrategyInterface $strategy,
38-
private SystemChangeWriterInterface $writer,
3938
private PasswordPolicyContext $context,
4039
private EventLogger $eventLogger,
4140
private SleeperInterface $sleeper,
@@ -68,13 +67,12 @@ public function preBind(PasswordBindAttempt $attempt): void
6867
*/
6968
public function recordFailure(PasswordBindAttempt $attempt): void
7069
{
71-
$recorded = $this->engine->recordBindFailure(
72-
$this->strategy->failureState($attempt),
73-
$attempt->policy,
74-
);
75-
$this->writer->write(
76-
$attempt->dn,
77-
$recorded->changes,
70+
$recorded = $this->strategy->record(
71+
$attempt,
72+
fn(UserPasswordState $state): RecordedOutcome => $this->engine->recordBindFailure(
73+
$state,
74+
$attempt->policy,
75+
),
7876
);
7977

8078
if ($recorded->outcome->denied) {
@@ -93,10 +91,17 @@ public function recordFailure(PasswordBindAttempt $attempt): void
9391
*/
9492
public function recordSuccess(PasswordBindAttempt $attempt): void
9593
{
96-
$state = $this->strategy->successState($attempt);
97-
$recorded = $this->engine->recordBindSuccess(
98-
$state,
99-
$attempt->policy,
94+
$state = null;
95+
$recorded = $this->strategy->record(
96+
$attempt,
97+
function (UserPasswordState $current) use ($attempt, &$state): RecordedOutcome {
98+
$state = $current;
99+
100+
return $this->engine->recordBindSuccess(
101+
$current,
102+
$attempt->policy,
103+
);
104+
},
100105
);
101106
$outcome = $recorded->outcome;
102107

@@ -113,14 +118,10 @@ public function recordSuccess(PasswordBindAttempt $attempt): void
113118
);
114119
}
115120

116-
$this->writer->write(
117-
$attempt->dn,
118-
$recorded->changes,
119-
);
120121
$this->context->setOutcome($outcome);
121122
$this->emitSuccessEvents(
122123
$attempt,
123-
$state,
124+
$state ?? $attempt->state,
124125
$outcome,
125126
);
126127
}

‎src/FreeDSx/Ldap/Server/PasswordPolicy/Replica/InMemoryReplicaPasswordStateStore.php‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,21 @@ public function load(Dn $dn): ReplicaPasswordState
3434
?? ReplicaPasswordState::empty();
3535
}
3636

37-
public function apply(
37+
/**
38+
* @param callable(ReplicaPasswordState): OperationalChanges $merge
39+
*/
40+
public function atomicMutate(
3841
Dn $dn,
39-
OperationalChanges $changes,
42+
callable $merge,
4043
): void {
44+
$key = $this->key($dn);
45+
$current = $this->states[$key] ?? ReplicaPasswordState::empty();
46+
$changes = $merge($current);
4147
if ($changes->isEmpty()) {
4248
return;
4349
}
4450

45-
$key = $this->key($dn);
46-
$state = ($this->states[$key] ?? ReplicaPasswordState::empty())
47-
->withChanges($changes);
48-
51+
$state = $current->withChanges($changes);
4952
if ($state->isEmpty()) {
5053
unset($this->states[$key]);
5154

‎src/FreeDSx/Ldap/Server/PasswordPolicy/Replica/ReplicaPasswordStateStoreInterface.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,13 @@ interface ReplicaPasswordStateStoreInterface
2929
public function load(Dn $dn): ReplicaPasswordState;
3030

3131
/**
32-
* Apply engine-emitted operational deltas to the subject's locally tracked state.
32+
* Atomically read the subject's current local state, derive changes from it via $merge, and apply them under an
33+
* exclusive lock so concurrent binds cannot lose an update.
34+
*
35+
* @param callable(ReplicaPasswordState): OperationalChanges $merge
3336
*/
34-
public function apply(
37+
public function atomicMutate(
3538
Dn $dn,
36-
OperationalChanges $changes,
39+
callable $merge,
3740
): void;
3841
}

0 commit comments

Comments
 (0)