From b9860ea97a266857d567408d3f8d8db3386a52d6 Mon Sep 17 00:00:00 2001 From: William Brown Date: Mon, 8 Jun 2020 15:16:55 +1000 Subject: [PATCH 1/3] Syncrepl fails to parse SyncInfoMessage when the message is a syncIdSet Previously, the SyncInfoMessage parser used a for loop to test for the presence of each named choice with with the structure. However, because refreshDelete and refreshPresent both are able to be fully resolved as defaults, and due to how pyasn1 accesses named types, it was not checking the choice tag, and would return a fully populated refreshDelete to the caller instead. This fixes the parser to always return the inner component, and retrieves the name based on the current choice of the tagged item. Author: William Brown --- Lib/ldap/syncrepl.py | 49 ++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/Lib/ldap/syncrepl.py b/Lib/ldap/syncrepl.py index 0de5cec4..f6ac2d1a 100644 --- a/Lib/ldap/syncrepl.py +++ b/Lib/ldap/syncrepl.py @@ -314,34 +314,35 @@ def __init__(self, encodedMessage): self.refreshPresent = None self.syncIdSet = None - for attr in ['newcookie', 'refreshDelete', 'refreshPresent', 'syncIdSet']: - comp = d[0].getComponentByName(attr) - - if comp is not None and comp.hasValue(): - - if attr == 'newcookie': - self.newcookie = str(comp) - return + # Due to the way pyasn1 works, refreshDelete and refreshPresent are both + # valid in the components as they are fully populated defaults. We must + # get the component directly from the message, not by iteration. + attr = d[0].getName() + comp = d[0].getComponent() + + if comp is not None and comp.hasValue(): + if attr == 'newcookie': + self.newcookie = str(comp) + return - val = {} + val = {} - cookie = comp.getComponentByName('cookie') - if cookie.hasValue(): - val['cookie'] = str(cookie) + cookie = comp.getComponentByName('cookie') + if cookie.hasValue(): + val['cookie'] = str(cookie) - if attr.startswith('refresh'): - val['refreshDone'] = bool(comp.getComponentByName('refreshDone')) - elif attr == 'syncIdSet': - uuids = [] - ids = comp.getComponentByName('syncUUIDs') - for i in range(len(ids)): - uuid = UUID(bytes=bytes(ids.getComponentByPosition(i))) - uuids.append(str(uuid)) - val['syncUUIDs'] = uuids - val['refreshDeletes'] = bool(comp.getComponentByName('refreshDeletes')) + if attr.startswith('refresh'): + val['refreshDone'] = bool(comp.getComponentByName('refreshDone')) + elif attr == 'syncIdSet': + uuids = [] + ids = comp.getComponentByName('syncUUIDs') + for i in range(len(ids)): + uuid = UUID(bytes=bytes(ids.getComponentByPosition(i))) + uuids.append(str(uuid)) + val['syncUUIDs'] = uuids + val['refreshDeletes'] = bool(comp.getComponentByName('refreshDeletes')) - setattr(self, attr, val) - return + setattr(self, attr, val) class SyncreplConsumer: From df98957ce3acfeced3aa6dddefe648724bd762ae Mon Sep 17 00:00:00 2001 From: William Brown Date: Tue, 9 Jun 2020 09:56:30 +1000 Subject: [PATCH 2/3] Add test case --- Tests/t_ldap_syncrepl.py | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/Tests/t_ldap_syncrepl.py b/Tests/t_ldap_syncrepl.py index 9398de5b..8d3fc9d6 100644 --- a/Tests/t_ldap_syncrepl.py +++ b/Tests/t_ldap_syncrepl.py @@ -10,6 +10,7 @@ import shelve import sys import unittest +import binascii if sys.version_info[0] <= 2: PY2 = True @@ -21,7 +22,7 @@ import ldap from ldap.ldapobject import SimpleLDAPObject -from ldap.syncrepl import SyncreplConsumer +from ldap.syncrepl import SyncreplConsumer, SyncInfoMessage from slapdtest import SlapdObject, SlapdTestCase @@ -445,6 +446,40 @@ def setUp(self): ) self.suffix = self.server.suffix.encode('utf-8') +class DecodeSyncreplProtoTests(unittest.TestCase): + """ + Tests of the ASN.1 decoder for tricky cases or past issues to ensure that + syncrepl messages are handled correctly. + """ + + def test_syncidset_message(self): + """ + A syncrepl server may send a sync info message, with a syncIdSet + of uuids to delete. A regression was found in the original + sync info message implementation due to how the choice was + evaluated, because refreshPresent and refreshDelete were both + able to be fully expressed as defaults, causing the parser + to mistakenly catch a syncIdSet as a refreshPresent/refereshDelete. + + This tests that a syncIdSet request is properly decoded. + + reference: https://tools.ietf.org/html/rfc4533#section-2.5 + """ + + # This is a dump of a syncidset message from wireshark + 389-ds + msg = """ + a36b04526c6461706b64632e6578616d706c652e636f6d3a333839303123636e + 3d6469726563746f7279206d616e616765723a64633d6578616d706c652c6463 + 3d636f6d3a286f626a656374436c6173733d2a2923330101ff311204108dc446 + 01a93611ea8aaff248c5fa5780 + """.replace(' ', '').replace('\n', '') + + msgraw = binascii.unhexlify(msg) + sim = SyncInfoMessage(msgraw) + self.assertEqual(sim.refreshDelete, None) + self.assertEqual(sim.refreshPresent, None) + self.assertNotEqual(sim.syncIdSet, None) + if __name__ == '__main__': unittest.main() From 544c99d2fe71885ab25d539290d4b0eaa1b72a8b Mon Sep 17 00:00:00 2001 From: Firstyear Date: Tue, 9 Jun 2020 16:33:08 +1000 Subject: [PATCH 3/3] Update Tests/t_ldap_syncrepl.py Good suggestion! Thanks! Co-authored-by: Christian Heimes --- Tests/t_ldap_syncrepl.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Tests/t_ldap_syncrepl.py b/Tests/t_ldap_syncrepl.py index 8d3fc9d6..b8a6ab63 100644 --- a/Tests/t_ldap_syncrepl.py +++ b/Tests/t_ldap_syncrepl.py @@ -478,7 +478,14 @@ def test_syncidset_message(self): sim = SyncInfoMessage(msgraw) self.assertEqual(sim.refreshDelete, None) self.assertEqual(sim.refreshPresent, None) - self.assertNotEqual(sim.syncIdSet, None) + self.assertEqual(sim.newcookie, None) + self.assertEqual(sim.syncIdSet, + { + 'cookie': 'ldapkdc.example.com:38901#cn=directory manager:dc=example,dc=com:(objectClass=*)#3', + 'syncUUIDs': ['8dc44601-a936-11ea-8aaf-f248c5fa5780'], + 'refreshDeletes': True + } + ) if __name__ == '__main__':