From 0d4474fec1884fd2afdd92a30b0b3cce57a143c3 Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Sun, 13 Sep 2026 22:35:45 -0400 Subject: [PATCH 1/2] Withhold sort keys on unreadable attributes from the result order. --- .../ConnectionGraphContainerProvider.php | 16 ++- .../AccessControl/WithheldSortKeyFilter.php | 51 +++++++ .../WithheldAttributeMiddleware.php | 17 ++- .../Storage/Concern/ControlTestsTrait.php | 51 +++++++ .../WithheldSortKeyFilterTest.php | 126 ++++++++++++++++++ .../WithheldAttributeMiddlewareTest.php | 80 ++++++++++- 6 files changed, 327 insertions(+), 14 deletions(-) create mode 100644 src/FreeDSx/Ldap/Server/AccessControl/WithheldSortKeyFilter.php create mode 100644 tests/unit/Server/AccessControl/WithheldSortKeyFilterTest.php diff --git a/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php b/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php index e8160c86..a51eb678 100644 --- a/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php +++ b/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php @@ -27,6 +27,7 @@ use FreeDSx\Ldap\Server\AccessControl\AccessControlInterface; use FreeDSx\Ldap\Server\AccessControl\WithheldAttributePolicy; use FreeDSx\Ldap\Server\AccessControl\WithheldFilterRewriter; +use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; use FreeDSx\Ldap\Server\Middleware\AliasDereferenceMiddleware; use FreeDSx\Ldap\Server\Middleware\AssertionMiddleware; use FreeDSx\Ldap\Server\Middleware\AttributeTypeCanonicalizationMiddleware; @@ -167,12 +168,15 @@ private function makeAliasDereferenceMiddleware(Container $container): AliasDere private function makeWithheldAttributeMiddleware(Container $container): WithheldAttributeMiddleware { - return new WithheldAttributeMiddleware(new WithheldFilterRewriter( - new WithheldAttributePolicy( - $container->get(AccessControlInterface::class), - $container->get(ServerOptions::class)->getSchema(), - ), - )); + $policy = new WithheldAttributePolicy( + $container->get(AccessControlInterface::class), + $container->get(ServerOptions::class)->getSchema(), + ); + + return new WithheldAttributeMiddleware( + new WithheldFilterRewriter($policy), + new WithheldSortKeyFilter($policy), + ); } private function makeAssertionMiddleware(Container $container): AssertionMiddleware diff --git a/src/FreeDSx/Ldap/Server/AccessControl/WithheldSortKeyFilter.php b/src/FreeDSx/Ldap/Server/AccessControl/WithheldSortKeyFilter.php new file mode 100644 index 00000000..f4608fd7 --- /dev/null +++ b/src/FreeDSx/Ldap/Server/AccessControl/WithheldSortKeyFilter.php @@ -0,0 +1,51 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FreeDSx\Ldap\Server\AccessControl; + +use FreeDSx\Ldap\Control\Sorting\SortingControl; +use FreeDSx\Ldap\Control\Sorting\SortKey; +use FreeDSx\Ldap\Server\Token\TokenInterface; + +use function array_filter; +use function array_values; + +/** + * Drops sort keys naming an attribute withheld from the identity, so the result order cannot reflect its value. + * + * @internal + * + * @author Chad Sikorra + */ +final readonly class WithheldSortKeyFilter +{ + public function __construct(private WithheldAttributePolicy $policy) {} + + /** + * Removes any key on a withheld attribute, leaving the control present so the sort is still reported as performed. + */ + public function stripWithheld( + SortingControl $control, + TokenInterface $token, + ): void { + $kept = array_values(array_filter( + $control->getSortKeys(), + fn(SortKey $sortKey): bool => !$this->policy->isWithheldFromFilter( + $sortKey->getAttribute(), + $token, + ), + )); + + $control->setSortKeys(...$kept); + } +} diff --git a/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php b/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php index 123b34d1..76ffd000 100644 --- a/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php +++ b/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php @@ -13,11 +13,14 @@ namespace FreeDSx\Ldap\Server\Middleware; +use FreeDSx\Ldap\Control\Control; +use FreeDSx\Ldap\Control\Sorting\SortingControl; use FreeDSx\Ldap\Operation\Request\CompareRequest; use FreeDSx\Ldap\Operation\Request\SearchRequest; use FreeDSx\Ldap\Protocol\Factory\ResponseFactory; use FreeDSx\Ldap\Protocol\Queue\Response\ResponseStream; use FreeDSx\Ldap\Server\AccessControl\WithheldFilterRewriter; +use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; use FreeDSx\Ldap\Server\Middleware\Pipeline\MiddlewareHandlerInterface; use FreeDSx\Ldap\Server\Middleware\Pipeline\MiddlewareInterface; use FreeDSx\Ldap\Server\Middleware\Pipeline\ServerRequestContext; @@ -25,7 +28,7 @@ use FreeDSx\Ldap\Server\Operation\OperationOutcomeResult; /** - * Withholds attributes from assertions before the request reaches storage. + * Withholds attributes from search and compare assertions, and from sort keys, before the request reaches storage. * * @internal * @@ -35,6 +38,7 @@ { public function __construct( private WithheldFilterRewriter $rewriter, + private WithheldSortKeyFilter $sortKeys, private ResponseFactory $responseFactory = new ResponseFactory(), ) {} @@ -60,9 +64,18 @@ private function processSearch( MiddlewareHandlerInterface $next, SearchRequest $request, ): ResponseStream { + $token = $context->tokenOrFail(); + + $sort = $context->message->controls()->get(Control::OID_SORTING); + if ($sort instanceof SortingControl) { + $this->sortKeys->stripWithheld( + $sort, + $token, + ); + } $rewritten = $this->rewriter->rewrite( $request->getFilter(), - $context->tokenOrFail(), + $token, ); if ($this->rewriter->isAbsoluteFalse($rewritten)) { diff --git a/tests/integration/Storage/Concern/ControlTestsTrait.php b/tests/integration/Storage/Concern/ControlTestsTrait.php index 40d653e1..f7eae6a0 100644 --- a/tests/integration/Storage/Concern/ControlTestsTrait.php +++ b/tests/integration/Storage/Concern/ControlTestsTrait.php @@ -861,6 +861,57 @@ public function testAMalformedControlValueIsAnsweredWithoutEndingTheSession(): v ); } + public function testASortKeyOnAWithheldAttributeDoesNotOrderByItsHiddenValue(): void + { + $this->authenticateAdmin(); + // Distinct passwords, created in an order unrelated to how their hashes sort. + $passwords = ['sort-leak-1' => 'mango', 'sort-leak-2' => 'apple', 'sort-leak-3' => 'cherry']; + foreach ($passwords as $cn => $password) { + $this->ldapClient()->create(Entry::fromArray("cn={$cn},ou=people,dc=foo,dc=bar", [ + 'objectClass' => ['inetOrgPerson'], + 'cn' => [$cn], + 'sn' => ['SortLeakProbe'], + 'userPassword' => ['{SHA}' . base64_encode(sha1($password, true))], + ])); + } + + $this->authenticateUser(); + $order = function (bool $descending): array { + $entries = $this->ldapClient()->search( + Operations::search(Filters::equal('sn', 'SortLeakProbe'), 'cn', 'userPassword') + ->base('ou=people,dc=foo,dc=bar') + ->useSubtreeScope(), + new SortingControl(new SortKey('userPassword', $descending)), + ); + $cns = []; + $visible = 0; + foreach ($entries as $entry) { + $cns[] = $entry->get('cn')?->firstValue(); + $visible += $entry->get('userPassword') === null ? 0 : 1; + } + + return ['cns' => $cns, 'visible' => $visible]; + }; + + $ascending = $order(false); + $descending = $order(true); + + $this->authenticateAdmin(); + foreach (array_keys($passwords) as $cn) { + $this->ldapClient()->delete("cn={$cn},ou=people,dc=foo,dc=bar"); + } + + // The withheld key has no effect, so reversing it cannot reorder the result. + self::assertSame( + $ascending['cns'], + $descending['cns'], + ); + self::assertSame( + 0, + $ascending['visible'], + ); + } + public function testSortControlAscendingOrdersResults(): void { $this->authenticateUser(); diff --git a/tests/unit/Server/AccessControl/WithheldSortKeyFilterTest.php b/tests/unit/Server/AccessControl/WithheldSortKeyFilterTest.php new file mode 100644 index 00000000..f32c1bf1 --- /dev/null +++ b/tests/unit/Server/AccessControl/WithheldSortKeyFilterTest.php @@ -0,0 +1,126 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Tests\Unit\FreeDSx\Ldap\Server\AccessControl; + +use FreeDSx\Ldap\Control\Sorting\SortingControl; +use FreeDSx\Ldap\Control\Sorting\SortKey; +use FreeDSx\Ldap\Schema\SchemaResource; +use FreeDSx\Ldap\Server\AccessControl\AclRules; +use FreeDSx\Ldap\Server\AccessControl\RuleBasedAccessControl; +use FreeDSx\Ldap\Server\AccessControl\WithheldAttributePolicy; +use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; +use FreeDSx\Ldap\Server\Token\BindToken; +use FreeDSx\Ldap\Server\Token\TokenInterface; +use PHPUnit\Framework\TestCase; + +final class WithheldSortKeyFilterTest extends TestCase +{ + private WithheldSortKeyFilter $subject; + + private TokenInterface $token; + + protected function setUp(): void + { + // Core marks userPassword X-CONFIDENTIAL, so it is withheld from filters; cn and sn are not. + $this->token = BindToken::fromDn('cn=user,dc=foo,dc=bar'); + $this->subject = new WithheldSortKeyFilter(new WithheldAttributePolicy( + new RuleBasedAccessControl(AclRules::fromEmpty()), + SchemaResource::Core->load(), + )); + } + + public function test_a_key_on_a_withheld_attribute_is_dropped(): void + { + $control = new SortingControl( + SortKey::ascending('userPassword'), + SortKey::ascending('cn'), + ); + + $this->subject->stripWithheld( + $control, + $this->token, + ); + + self::assertSame( + ['cn'], + $this->attributesOf($control), + ); + } + + public function test_a_control_of_only_withheld_keys_is_left_empty(): void + { + $control = new SortingControl(SortKey::ascending('userPassword')); + + $this->subject->stripWithheld( + $control, + $this->token, + ); + + self::assertSame( + [], + $control->getSortKeys(), + ); + } + + public function test_keys_on_readable_attributes_are_left_untouched(): void + { + $control = new SortingControl( + SortKey::descending('sn'), + SortKey::ascending('cn'), + ); + + $this->subject->stripWithheld( + $control, + $this->token, + ); + + self::assertSame( + ['sn', 'cn'], + $this->attributesOf($control), + ); + } + + public function test_it_preserves_the_order_and_direction_of_surviving_keys(): void + { + $control = new SortingControl( + SortKey::descending('sn'), + SortKey::ascending('userPassword'), + SortKey::ascending('cn'), + ); + + $this->subject->stripWithheld( + $control, + $this->token, + ); + + $kept = $control->getSortKeys(); + self::assertSame( + ['sn', 'cn'], + $this->attributesOf($control), + ); + self::assertTrue($kept[0]->getUseReverseOrder()); + self::assertFalse($kept[1]->getUseReverseOrder()); + } + + /** + * @return list + */ + private function attributesOf(SortingControl $control): array + { + return array_values(array_map( + static fn(SortKey $sortKey): string => $sortKey->getAttribute(), + $control->getSortKeys(), + )); + } +} diff --git a/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php b/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php index 9acd02af..94d9d6c8 100644 --- a/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php +++ b/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php @@ -13,6 +13,9 @@ namespace Tests\Unit\FreeDSx\Ldap\Server\Middleware; +use FreeDSx\Ldap\Control\Control; +use FreeDSx\Ldap\Control\Sorting\SortingControl; +use FreeDSx\Ldap\Control\Sorting\SortKey; use FreeDSx\Ldap\Operation\Request\RequestInterface; use FreeDSx\Ldap\Operation\ResultCode; use FreeDSx\Ldap\Operations; @@ -23,6 +26,7 @@ use FreeDSx\Ldap\Server\AccessControl\AclRules; use FreeDSx\Ldap\Server\AccessControl\WithheldAttributePolicy; use FreeDSx\Ldap\Server\AccessControl\WithheldFilterRewriter; +use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; use FreeDSx\Ldap\Server\AccessControl\RuleBasedAccessControl; use FreeDSx\Ldap\Server\Middleware\WithheldAttributeMiddleware; use FreeDSx\Ldap\Server\Middleware\Pipeline\MiddlewareHandlerInterface; @@ -46,12 +50,14 @@ protected function setUp(): void { $this->next = $this->createMock(MiddlewareHandlerInterface::class); $this->token = BindToken::fromDn('cn=user,dc=foo,dc=bar'); - $this->subject = new WithheldAttributeMiddleware(new WithheldFilterRewriter( - new WithheldAttributePolicy( - new RuleBasedAccessControl(AclRules::fromEmpty()), - SchemaResource::Core->load(), - ), - )); + $policy = new WithheldAttributePolicy( + new RuleBasedAccessControl(AclRules::fromEmpty()), + SchemaResource::Core->load(), + ); + $this->subject = new WithheldAttributeMiddleware( + new WithheldFilterRewriter($policy), + new WithheldSortKeyFilter($policy), + ); } public function test_a_search_on_a_withheld_attribute_succeeds_with_no_entries(): void @@ -184,6 +190,57 @@ public function test_an_unrelated_operation_is_passed_on(): void ); } + public function test_a_sort_key_on_a_withheld_attribute_is_dropped(): void + { + $sort = new SortingControl( + SortKey::ascending('userPassword'), + SortKey::ascending('cn'), + ); + $this->next + ->method('handle') + ->willReturn(ResponseStream::of([], OperationOutcomeResult::succeeded())); + + $this->subject->process( + $this->contextWith( + Operations::search(Filters::present('cn'))->base('dc=foo,dc=bar'), + $sort, + ), + $this->next, + ); + + $kept = $sort->getSortKeys(); + self::assertCount( + 1, + $kept, + ); + self::assertSame( + 'cn', + $kept[0]->getAttribute(), + ); + } + + public function test_a_sort_of_only_withheld_keys_is_left_empty_and_still_passed_on(): void + { + $sort = new SortingControl(SortKey::ascending('userPassword')); + $this->next + ->expects(self::once()) + ->method('handle') + ->willReturn(ResponseStream::of([], OperationOutcomeResult::succeeded())); + + $this->subject->process( + $this->contextWith( + Operations::search(Filters::present('cn'))->base('dc=foo,dc=bar'), + $sort, + ), + $this->next, + ); + + self::assertSame( + [], + $sort->getSortKeys(), + ); + } + private function context(RequestInterface $request): ServerRequestContext { return (new ServerRequestContext(new LdapMessageRequest( @@ -191,4 +248,15 @@ private function context(RequestInterface $request): ServerRequestContext $request, )))->withToken($this->token); } + + private function contextWith( + RequestInterface $request, + Control ...$controls, + ): ServerRequestContext { + return (new ServerRequestContext(new LdapMessageRequest( + 1, + $request, + ...$controls, + )))->withToken($this->token); + } } From 9b31dd8f1e4e4e57768e7a85caa01e249755f9d3 Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Mon, 14 Sep 2026 07:36:07 -0400 Subject: [PATCH 2/2] A value assertion on the identity's own entry discloses nothing it does not already control. --- docs/Server/Access-Control.md | 19 ++- .../ConnectionGraphContainerProvider.php | 2 + .../Subject/MatchesBoundIdentity.php | 42 +++++++ .../Subject/SelfSubjectMatcher.php | 13 +- .../WithheldValueModifyGuard.php | 78 ++++++++++++ .../WithheldAttributeMiddleware.php | 13 +- .../Storage/Concern/DefaultAclTestsTrait.php | 79 +++++++++++- .../WithheldValueModifyGuardTest.php | 116 ++++++++++++++++++ .../WithheldAttributeMiddlewareTest.php | 2 + 9 files changed, 351 insertions(+), 13 deletions(-) create mode 100644 src/FreeDSx/Ldap/Server/AccessControl/Subject/MatchesBoundIdentity.php create mode 100644 src/FreeDSx/Ldap/Server/AccessControl/WithheldValueModifyGuard.php create mode 100644 tests/unit/Server/AccessControl/WithheldValueModifyGuardTest.php diff --git a/docs/Server/Access-Control.md b/docs/Server/Access-Control.md index 5d7b5ef6..cb5e3c96 100644 --- a/docs/Server/Access-Control.md +++ b/docs/Server/Access-Control.md @@ -449,7 +449,8 @@ A denied assertion is folded away before the query runs, so the attribute behave ``` This is what stops a value being recovered a guess at a time. Reads are unaffected: denying the filter does not strip -the value, and denying the read does not stop the filter. Pair the two to get both. +the value, and denying the read does not stop the filter. Pair the two to get both. A value-level modify on a +filter-denied attribute is gated the same way; see [Value-Level Modify](#value-level-modify). An extensible match that names no attribute type is refused with `inappropriateMatching`, since it asserts against every attribute at once and it's not possible to make attribute level ACL decisions against it. @@ -502,10 +503,24 @@ Four things to keep in mind: - A grant is required in addition to read access. A permissive `AttributeRule` cannot re-expose a confidential attribute. - Administrators are locked out too until granted. Only the break-glass manager bypasses this. -- Writes are unaffected, which is what keeps `userPassword` settable by its owner while unreadable. +- Setting the value is unaffected. A whole-attribute replace overwrites unconditionally, so an owner can still set + `userPassword` while unable to read it. - A custom schema must carry the extension. Supplying your own `userPassword` definition through a schema source without it silently drops the protection. +### Value-Level Modify + +A modify that names a specific value, an `add` or a `delete` of one value, is answered from the entry's current values: +adding a value already present, or deleting one that is absent, fails with its own result code. That lets an identity +that may write but not read an attribute confirm a value one guess at a time. Deleting the whole attribute is the same, +revealing whether any value is present at all. + +So on an attribute withheld from filters, whether by `X-CONFIDENTIAL` or a [Filter Rule](#filter-rules), a value-level +`add` or `delete` of another entry is refused with `insufficientAccessRights`. Use a whole-attribute replace, which +overwrites unconditionally and reveals nothing about the prior value; a replace with no values still removes the +attribute. The identity's own entry is exempt, so a self password change that supplies the old value (delete the old, +add the new) still works. + Replication is not affected. A content sync ships every visible entry whole, so a replica receives confidential attributes without a grant for them. See [Replication](Replication.md#access-control). diff --git a/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php b/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php index a51eb678..ee6510ad 100644 --- a/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php +++ b/src/FreeDSx/Ldap/Container/Provider/ConnectionGraphContainerProvider.php @@ -28,6 +28,7 @@ use FreeDSx\Ldap\Server\AccessControl\WithheldAttributePolicy; use FreeDSx\Ldap\Server\AccessControl\WithheldFilterRewriter; use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; +use FreeDSx\Ldap\Server\AccessControl\WithheldValueModifyGuard; use FreeDSx\Ldap\Server\Middleware\AliasDereferenceMiddleware; use FreeDSx\Ldap\Server\Middleware\AssertionMiddleware; use FreeDSx\Ldap\Server\Middleware\AttributeTypeCanonicalizationMiddleware; @@ -176,6 +177,7 @@ private function makeWithheldAttributeMiddleware(Container $container): Withheld return new WithheldAttributeMiddleware( new WithheldFilterRewriter($policy), new WithheldSortKeyFilter($policy), + new WithheldValueModifyGuard($policy), ); } diff --git a/src/FreeDSx/Ldap/Server/AccessControl/Subject/MatchesBoundIdentity.php b/src/FreeDSx/Ldap/Server/AccessControl/Subject/MatchesBoundIdentity.php new file mode 100644 index 00000000..c3bd1df3 --- /dev/null +++ b/src/FreeDSx/Ldap/Server/AccessControl/Subject/MatchesBoundIdentity.php @@ -0,0 +1,42 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FreeDSx\Ldap\Server\AccessControl\Subject; + +use FreeDSx\Ldap\Entry\Dn; +use FreeDSx\Ldap\Server\Token\AuthenticatedTokenInterface; +use FreeDSx\Ldap\Server\Token\TokenInterface; + +/** + * Answers whether a token's bound DN is the given target entry (case-insensitive), the shared notion of "self". + * + * @author Chad Sikorra + */ +trait MatchesBoundIdentity +{ + /** + * Note: Without a target there is nothing to be the same as, and only an authenticated token has a bound DN. + * + * @param TokenInterface $token + * @param Dn|null $target + * @return bool + */ + private function isBoundIdentity( + TokenInterface $token, + ?Dn $target, + ): bool { + return $target !== null + && $token instanceof AuthenticatedTokenInterface + && $token->getResolvedDn()->normalize()->toString() === $target->normalize()->toString(); + } +} diff --git a/src/FreeDSx/Ldap/Server/AccessControl/Subject/SelfSubjectMatcher.php b/src/FreeDSx/Ldap/Server/AccessControl/Subject/SelfSubjectMatcher.php index bfaefdca..8b1e9d75 100644 --- a/src/FreeDSx/Ldap/Server/AccessControl/Subject/SelfSubjectMatcher.php +++ b/src/FreeDSx/Ldap/Server/AccessControl/Subject/SelfSubjectMatcher.php @@ -14,7 +14,6 @@ namespace FreeDSx\Ldap\Server\AccessControl\Subject; use FreeDSx\Ldap\Entry\Dn; -use FreeDSx\Ldap\Server\Token\AuthenticatedTokenInterface; use FreeDSx\Ldap\Server\Token\TokenInterface; /** @@ -24,15 +23,15 @@ */ final class SelfSubjectMatcher implements TargetDependentSubjectInterface { + use MatchesBoundIdentity; + public function matches( TokenInterface $token, ?Dn $targetDn, ): bool { - // Without a target there is nothing to be the same as, so this can never be a self match. - if ($targetDn === null || !$token instanceof AuthenticatedTokenInterface) { - return false; - } - - return $token->getResolvedDn()->normalize()->toString() === $targetDn->normalize()->toString(); + return $this->isBoundIdentity( + $token, + $targetDn, + ); } } diff --git a/src/FreeDSx/Ldap/Server/AccessControl/WithheldValueModifyGuard.php b/src/FreeDSx/Ldap/Server/AccessControl/WithheldValueModifyGuard.php new file mode 100644 index 00000000..34a40f19 --- /dev/null +++ b/src/FreeDSx/Ldap/Server/AccessControl/WithheldValueModifyGuard.php @@ -0,0 +1,78 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FreeDSx\Ldap\Server\AccessControl; + +use FreeDSx\Ldap\Entry\Change; +use FreeDSx\Ldap\Exception\OperationException; +use FreeDSx\Ldap\Operation\Request\ModifyRequest; +use FreeDSx\Ldap\Operation\ResultCode; +use FreeDSx\Ldap\Server\AccessControl\Subject\MatchesBoundIdentity; +use FreeDSx\Ldap\Server\Token\TokenInterface; + +use function in_array; + +/** + * Refuses a value-level add or delete that would confirm a value of an attribute the identity may not read. + * + * @internal + * + * @author Chad Sikorra + */ +final readonly class WithheldValueModifyGuard +{ + use MatchesBoundIdentity; + + public function __construct(private WithheldAttributePolicy $policy) {} + + /** + * A value assertion reveals through its result code whether the value is present, so it needs read of the attribute. + * + * A value assertion on the identity's own entry discloses nothing it does not already control. + * + * @throws OperationException + */ + public function assertAllowed( + ModifyRequest $request, + TokenInterface $token, + ): void { + if ($this->isBoundIdentity($token, $request->getDn())) { + return; + } + + foreach ($request->getChanges() as $change) { + if (!$this->isAddOrDelete($change)) { + continue; + } + + if ($this->policy->isWithheldFromFilter($change->getAttribute()->getName(), $token)) { + throw new OperationException( + 'Insufficient access rights.', + ResultCode::INSUFFICIENT_ACCESS_RIGHTS, + ); + } + } + } + + /** + * An add or delete is answered from the stored values, so its result code reveals them; a replace is unconditional. + */ + private function isAddOrDelete(Change $change): bool + { + return in_array( + $change->getType(), + [Change::TYPE_ADD, Change::TYPE_DELETE], + true, + ); + } +} diff --git a/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php b/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php index 76ffd000..bcd447ed 100644 --- a/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php +++ b/src/FreeDSx/Ldap/Server/Middleware/WithheldAttributeMiddleware.php @@ -16,11 +16,13 @@ use FreeDSx\Ldap\Control\Control; use FreeDSx\Ldap\Control\Sorting\SortingControl; use FreeDSx\Ldap\Operation\Request\CompareRequest; +use FreeDSx\Ldap\Operation\Request\ModifyRequest; use FreeDSx\Ldap\Operation\Request\SearchRequest; use FreeDSx\Ldap\Protocol\Factory\ResponseFactory; use FreeDSx\Ldap\Protocol\Queue\Response\ResponseStream; use FreeDSx\Ldap\Server\AccessControl\WithheldFilterRewriter; use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; +use FreeDSx\Ldap\Server\AccessControl\WithheldValueModifyGuard; use FreeDSx\Ldap\Server\Middleware\Pipeline\MiddlewareHandlerInterface; use FreeDSx\Ldap\Server\Middleware\Pipeline\MiddlewareInterface; use FreeDSx\Ldap\Server\Middleware\Pipeline\ServerRequestContext; @@ -28,7 +30,7 @@ use FreeDSx\Ldap\Server\Operation\OperationOutcomeResult; /** - * Withholds attributes from search and compare assertions, and from sort keys, before the request reaches storage. + * Withholds attributes from search and compare assertions, sort keys, and value-level modifies, before storage. * * @internal * @@ -39,6 +41,7 @@ public function __construct( private WithheldFilterRewriter $rewriter, private WithheldSortKeyFilter $sortKeys, + private WithheldValueModifyGuard $valueModify, private ResponseFactory $responseFactory = new ResponseFactory(), ) {} @@ -51,10 +54,15 @@ public function process( if ($request instanceof SearchRequest) { return $this->processSearch($context, $next, $request); } - if ($request instanceof CompareRequest) { return $this->processCompare($context, $next, $request); } + if ($request instanceof ModifyRequest) { + $this->valueModify->assertAllowed( + $request, + $context->tokenOrFail(), + ); + } return $next->handle($context); } @@ -106,7 +114,6 @@ private function processCompare( if (!$this->rewriter->isAbsoluteFalse($rewritten)) { return $next->handle($context); } - $result = CompareOperationResult::completed( $context->message, false, diff --git a/tests/integration/Storage/Concern/DefaultAclTestsTrait.php b/tests/integration/Storage/Concern/DefaultAclTestsTrait.php index aab640a5..3f3cc629 100644 --- a/tests/integration/Storage/Concern/DefaultAclTestsTrait.php +++ b/tests/integration/Storage/Concern/DefaultAclTestsTrait.php @@ -13,11 +13,15 @@ namespace Tests\Integration\FreeDSx\Ldap\Storage\Concern; +use FreeDSx\Ldap\Entry\Change; +use FreeDSx\Ldap\Entry\Entry; +use FreeDSx\Ldap\Exception\OperationException; +use FreeDSx\Ldap\Operation\ResultCode; use FreeDSx\Ldap\Operations; use FreeDSx\Ldap\Search\Filters; /** - * Behavior enforced by the shipped default ACL, across the search and paging paths. + * Behavior enforced by the shipped default ACL, across the search, paging and write paths. */ trait DefaultAclTestsTrait { @@ -142,4 +146,77 @@ public function testPagingOnAWithheldFilterReturnsNothing(): void $found, ); } + + public function testAValueLevelModifyOfAWithheldAttributeOnAnotherEntryIsRefused(): void + { + $this->authenticateAdmin(); + // The admin may write userPassword but not read it, so a value assertion must not confirm a value. + $dn = 'cn=vp-target,ou=people,dc=foo,dc=bar'; + $stored = '{SHA}' . base64_encode(sha1('known', true)); + $this->ldapClient()->create(Entry::fromArray($dn, [ + 'objectClass' => ['inetOrgPerson'], + 'cn' => ['vp-target'], + 'sn' => ['ValueProbe'], + 'userPassword' => [$stored], + ])); + + $addExisting = null; + $deleteMissing = null; + try { + try { + $this->ldapClient()->send(Operations::modify( + $dn, + Change::add('userPassword', $stored), + )); + } catch (OperationException $e) { + $addExisting = $e->getCode(); + } + try { + $this->ldapClient()->send(Operations::modify( + $dn, + Change::delete('userPassword', '{SHA}bogusvaluenotpresent='), + )); + } catch (OperationException $e) { + $deleteMissing = $e->getCode(); + } + // A whole-attribute replace asserts nothing about the hidden value, so it must still succeed. + $this->ldapClient()->send(Operations::modify( + $dn, + Change::replace('userPassword', '{SHA}' . base64_encode(sha1('reset', true))), + )); + } finally { + $this->ldapClient()->delete($dn); + } + + self::assertSame( + ResultCode::INSUFFICIENT_ACCESS_RIGHTS, + $addExisting, + ); + self::assertSame( + ResultCode::INSUFFICIENT_ACCESS_RIGHTS, + $deleteMissing, + ); + } + + public function testSelfMayMakeAValueLevelModifyOfItsOwnWithheldAttribute(): void + { + // cn=user can write but not read its own userPassword; a value assertion on its own entry is allowed + // through to the ordinary value check rather than refused, so a self password change by delete-old works. + $this->authenticateUser(); + + $code = null; + try { + $this->ldapClient()->send(Operations::modify( + 'cn=user,dc=foo,dc=bar', + Change::delete('userPassword', '{SHA}notthestoredvalue='), + )); + } catch (OperationException $e) { + $code = $e->getCode(); + } + + self::assertSame( + ResultCode::NO_SUCH_ATTRIBUTE, + $code, + ); + } } diff --git a/tests/unit/Server/AccessControl/WithheldValueModifyGuardTest.php b/tests/unit/Server/AccessControl/WithheldValueModifyGuardTest.php new file mode 100644 index 00000000..8e785be8 --- /dev/null +++ b/tests/unit/Server/AccessControl/WithheldValueModifyGuardTest.php @@ -0,0 +1,116 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Tests\Unit\FreeDSx\Ldap\Server\AccessControl; + +use FreeDSx\Ldap\Entry\Change; +use FreeDSx\Ldap\Entry\Dn; +use FreeDSx\Ldap\Exception\OperationException; +use FreeDSx\Ldap\Operation\ResultCode; +use FreeDSx\Ldap\Operations; +use FreeDSx\Ldap\Schema\SchemaResource; +use FreeDSx\Ldap\Server\AccessControl\AclRules; +use FreeDSx\Ldap\Server\AccessControl\PrivilegedBypassAccessControl; +use FreeDSx\Ldap\Server\AccessControl\RuleBasedAccessControl; +use FreeDSx\Ldap\Server\AccessControl\WithheldAttributePolicy; +use FreeDSx\Ldap\Server\AccessControl\WithheldValueModifyGuard; +use FreeDSx\Ldap\Server\Token\BindToken; +use FreeDSx\Ldap\Server\Token\ManagerToken; +use FreeDSx\Ldap\Server\Token\TokenInterface; +use PHPUnit\Framework\TestCase; + +final class WithheldValueModifyGuardTest extends TestCase +{ + private const TARGET = 'cn=user,dc=foo,dc=bar'; + + private WithheldValueModifyGuard $subject; + + private TokenInterface $other; + + protected function setUp(): void + { + // Core marks userPassword X-CONFIDENTIAL, so it is withheld; cn is not. + $this->subject = new WithheldValueModifyGuard(new WithheldAttributePolicy( + new RuleBasedAccessControl(AclRules::fromEmpty()), + SchemaResource::Core->load(), + )); + $this->other = BindToken::fromDn('cn=admin,dc=foo,dc=bar'); + } + + public function test_it_refuses_a_value_add_of_a_withheld_attribute_on_another_entry(): void + { + $this->expectException(OperationException::class); + $this->expectExceptionCode(ResultCode::INSUFFICIENT_ACCESS_RIGHTS); + + $this->subject->assertAllowed( + Operations::modify(self::TARGET, Change::add('userPassword', '{SHA}guess')), + $this->other, + ); + } + + public function test_it_refuses_a_whole_attribute_delete_of_a_withheld_attribute_on_another_entry(): void + { + $this->expectException(OperationException::class); + $this->expectExceptionCode(ResultCode::INSUFFICIENT_ACCESS_RIGHTS); + + $this->subject->assertAllowed( + Operations::modify(self::TARGET, Change::delete('userPassword')), + $this->other, + ); + } + + public function test_it_allows_a_replace_of_a_withheld_attribute_on_another_entry(): void + { + $this->expectNotToPerformAssertions(); + + $this->subject->assertAllowed( + Operations::modify(self::TARGET, Change::replace('userPassword', '{SHA}new')), + $this->other, + ); + } + + public function test_it_allows_a_value_modify_of_a_readable_attribute_on_another_entry(): void + { + $this->expectNotToPerformAssertions(); + + $this->subject->assertAllowed( + Operations::modify(self::TARGET, Change::add('description', 'note')), + $this->other, + ); + } + + public function test_it_allows_a_value_modify_of_a_withheld_attribute_on_the_identitys_own_entry(): void + { + $this->expectNotToPerformAssertions(); + + $this->subject->assertAllowed( + Operations::modify(self::TARGET, Change::delete('userPassword', '{SHA}old')), + BindToken::fromDn(self::TARGET), + ); + } + + public function test_a_privileged_manager_is_not_restricted(): void + { + $this->expectNotToPerformAssertions(); + + $guard = new WithheldValueModifyGuard(new WithheldAttributePolicy( + new PrivilegedBypassAccessControl(new RuleBasedAccessControl(AclRules::fromEmpty())), + SchemaResource::Core->load(), + )); + + $guard->assertAllowed( + Operations::modify(self::TARGET, Change::add('userPassword', '{SHA}guess')), + new ManagerToken(new Dn('cn=manager')), + ); + } +} diff --git a/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php b/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php index 94d9d6c8..c2f264a5 100644 --- a/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php +++ b/tests/unit/Server/Middleware/WithheldAttributeMiddlewareTest.php @@ -27,6 +27,7 @@ use FreeDSx\Ldap\Server\AccessControl\WithheldAttributePolicy; use FreeDSx\Ldap\Server\AccessControl\WithheldFilterRewriter; use FreeDSx\Ldap\Server\AccessControl\WithheldSortKeyFilter; +use FreeDSx\Ldap\Server\AccessControl\WithheldValueModifyGuard; use FreeDSx\Ldap\Server\AccessControl\RuleBasedAccessControl; use FreeDSx\Ldap\Server\Middleware\WithheldAttributeMiddleware; use FreeDSx\Ldap\Server\Middleware\Pipeline\MiddlewareHandlerInterface; @@ -57,6 +58,7 @@ protected function setUp(): void $this->subject = new WithheldAttributeMiddleware( new WithheldFilterRewriter($policy), new WithheldSortKeyFilter($policy), + new WithheldValueModifyGuard($policy), ); }