#50221 Issue 50220 - attr_encryption test suite failing
Closed 2 years ago by spichugi. Opened 3 years ago by aadhikari.
aadhikari/389-ds-base py3-fix-encryp-attr  into  master

@@ -16,6 +16,7 @@ 

  from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES

  from lib389.backend import Backends

  from lib389.idm.domain import Domain

+ from lib389.encrypted_attributes import EncryptedAttrs

  

  pytestmark = pytest.mark.tier1

  
@@ -37,8 +38,7 @@ 

      log.info("Enables attribute encryption")

      backends = Backends(topo.standalone)

      backend = backends.list()[0]

-     encrypt_attrs = backend.get_encrypted_attrs()

- 

+     encrypt_attrs = EncryptedAttrs(topo.standalone, basedn='cn=encrypted attributes,{}'.format(backend.dn))

      log.info("Enables attribute encryption for employeeNumber and telephoneNumber")

      emp_num_encrypt = encrypt_attrs.create(properties={'cn': 'employeeNumber', 'nsEncryptionAlgorithm': 'AES'})

      telephone_encrypt = encrypt_attrs.create(properties={'cn': 'telephoneNumber', 'nsEncryptionAlgorithm': '3DES'})
@@ -86,7 +86,7 @@ 

      log.info("Extracting values of cn from the list of objects in encrypt_attrs")

      log.info("And appending the cn values in a list")

      enc_attrs_cns = []

-     for enc_attr in encrypt_attrs.list():

+     for enc_attr in encrypt_attrs:

          enc_attrs_cns.append(enc_attr.rdn)

  

      log.info("Check employeenumber encryption is enabled")
@@ -149,7 +149,7 @@ 

      # Offline export

      topo.standalone.stop()

      if not topo.standalone.ldif2db(bename=DEFAULT_BENAME, suffixes=(DEFAULT_SUFFIX,),

-                                    excludeSuffixes=None,  encrypt=False, import_file=import_ldif):

+                                    excludeSuffixes=None, encrypt=False, import_file=import_ldif):

          log.fatal('Failed to run offline ldif2db')

          assert False

      topo.standalone.start()
@@ -160,7 +160,6 @@ 

      assert user.present("telephoneNumber")

  

  

- 

  def test_export_import_plaintext(topo, enable_user_attr_encryption):

      """Configure attribute encryption, store some data, check that we can export the plain text

       :id: b171e215-0456-48a5-245f-c21abc40fc2d
@@ -209,7 +208,7 @@ 

      # Offline export

      topo.standalone.stop()

      if not topo.standalone.ldif2db(bename=DEFAULT_BENAME, suffixes=(DEFAULT_SUFFIX,),

-                                    excludeSuffixes=None,  encrypt=True, import_file=import_ldif):

+                                    excludeSuffixes=None, encrypt=True, import_file=import_ldif):

          log.fatal('Failed to run offline ldif2db')

          assert False

      topo.standalone.start()
@@ -288,8 +287,9 @@ 

  

      # Create backends

      backends = Backends(topo.standalone)

+     backend = backends.list()[0]

      test_backend1 = backends.create(properties={'cn': test_db1,

-                                                'nsslapd-suffix': test_suffix1})

+                                                 'nsslapd-suffix': test_suffix1})

      test_backend2 = backends.create(properties={'cn': test_db2,

                                                  'nsslapd-suffix': test_suffix2})

  
@@ -300,14 +300,14 @@ 

      test2 = suffix2.create(properties={'dc': 'test2'})

  

      log.info("Enables attribute encryption for telephoneNumber in test_backend1")

-     backend1_encrypt_attrs = test_backend1.get_encrypted_attrs()

+     backend1_encrypt_attrs = EncryptedAttrs(topo.standalone, basedn='cn=encrypted attributes,{}'.format(test_backend1.dn))

      b1_encrypt = backend1_encrypt_attrs.create(properties={'cn': 'telephoneNumber',

                                                             'nsEncryptionAlgorithm': 'AES'})

  

      log.info("Enables attribute encryption for employeeNumber in test_backend2")

-     backend2_encrypt_attrs = test_backend2.get_encrypted_attrs()

+     backend2_encrypt_attrs = EncryptedAttrs(topo.standalone, basedn='cn=encrypted attributes,{}'.format(test_backend2.dn))

      b2_encrypt = backend2_encrypt_attrs.create(properties={'cn': 'employeeNumber',

-                                                                  'nsEncryptionAlgorithm': 'AES'})

+                                                            'nsEncryptionAlgorithm': 'AES'})

  

      log.info("Add a test user with encrypted attributes in both backends")

      users = UserAccounts(topo.standalone, test1.dn, None)
@@ -386,7 +386,7 @@ 

      # Create backends

      backends = Backends(topo.standalone)

      test_backend1 = backends.create(properties={'cn': test_db1,

-                                                'nsslapd-suffix': test_suffix1})

+                                                 'nsslapd-suffix': test_suffix1})

      test_backend2 = backends.create(properties={'cn': test_db2,

                                                  'nsslapd-suffix': test_suffix2})

  
@@ -397,7 +397,7 @@ 

      test2 = suffix2.create(properties={'dc': 'test2'})

  

      log.info("Enables attribute encryption for telephoneNumber in test_backend1")

-     backend1_encrypt_attrs = test_backend1.get_encrypted_attrs()

+     backend1_encrypt_attrs = EncryptedAttrs(topo.standalone, basedn='cn=encrypted attributes,{}'.format(test_backend1.dn))

      b1_encrypt = backend1_encrypt_attrs.create(properties={'cn': 'telephoneNumber',

                                                             'nsEncryptionAlgorithm': 'AES'})

  
@@ -451,5 +451,3 @@ 

      # -s for DEBUG mode

      CURRENT_FILE = os.path.realpath(__file__)

      pytest.main("-s %s" % CURRENT_FILE)

- 

- 

Description: Fixed the issue by removing the old function of creating an encrypted attribute
with a new one.

https://pagure.io/389-ds-base/issue/50220

Reviewed by: ??

The point of this (what you removed), is you don't need to play DN shuffle to get the right one - you only ask the backend for it's related encrypted attrs part. So I think you should undo these lines unless there is a specific reason for this.

I don't like this change. I think that this will break and surprise users because the basedn suddenly has content prepended to it. Remember, this library is intended to be used by "not us", and this violates the least surprise principle. The "backend.get_encrypted_attrs()" is the right way to get the associated encrypted attrs object, and the basedn is set correctly in that process.

I don't see how this change is fixing a "breakage" in the tests? I think you have done two things. One is probably fixing the breakage, the other is a usability change to the api (that I don't think I'll approve).

What is the actual test failure you are seeing here? Why is it occuring? Is there a better way to fix it?

I don't like this change. I think that this will break and surprise users because the basedn suddenly has content prepended to it. Remember, this library is intended to be used by "not us", and this violates the least surprise principle. The "backend.get_encrypted_attrs()" is the right way to get the associated encrypted attrs object, and the basedn is set correctly in that process.

@firstyear backend.get_encrypted_attrs() doesn't have any attribute to create. We were seeing attribute error in for eg: emp_num_encrypt = encrypt_attrs.create(properties={'cn': 'employeeNumber', 'nsEncryptionAlgorithm': 'AES'}) seems like some changes in lib389.
While using I got to know it set the dn incorrectly, that is : cn=uid,cn=userRoot,cn=ldbm database,cn=plugins,cn=config whereas it should be cn=uid,cn=encrypted attributes,cn=userRoot,cn=ldbm database,cn=plugins,cn=config. But if it is what it is supposed to like we can fix it in the code, but if it is specific to attr_encrption then we should fix in lib389. if cn=encrypted attributes is not present we are seeing assertion errors.

It looks like the implemented api has a subtle incorrect behaviour. It should be:

list_encrypted_attrs()
list_encrypted_attrs_names()
get_encrypted_attr(name_of_attr)

Additionally, in backend.py, it's using the wrong dn:

EncryptedAttrs(self._instance, basedn=self._dn)
SHOULD BE
basedn="cn=encrypted attributes," + self._dn

That would cause the child EncryptedAttr to have the wrong dn, because it's pre-pending the rdn to the wrong location.

Thinking about it add_encrypted and del_encrypted are wrong to. You actually want:

ea = get_encrypted_attr(name)
ea.delete()
ea.create()

etc.

So to fix this.

remove add_encrypted_attr, del_encrypted_attr, get_encrypted_attrs, make it list_escrypted_attrs (return all of them), list_encrypted_attrs_name(return a list of strings of the cns) and get_encrypted_attr.

Thinking about it add_encrypted and del_encrypted are wrong to. You actually want:
ea = get_encrypted_attr(name)
ea.delete()
ea.create()

etc.

So to fix this.
remove add_encrypted_attr, del_encrypted_attr, get_encrypted_attrs, make it list_escrypted_attrs (return all of them), list_encrypted_attrs_name(return a list of strings of the cns) and get_encrypted_attr.

Thanks for looking into this, will try and do the changes :)

Thinking about it add_encrypted and del_encrypted are wrong to. You actually want:
ea = get_encrypted_attr(name)
ea.delete()
ea.create()

etc.

So to fix this.
remove add_encrypted_attr, del_encrypted_attr, get_encrypted_attrs, make it list_escrypted_attrs (return all of them), list_encrypted_attrs_name(return a list of strings of the cns) and get_encrypted_attr.

@firstyear Can't find any of the above-mentioned functions in the source, can you tell where are these, I have grepped it can't find anything like this. BTW rather than making changes to lib389 we can pass the correct DN, but I still think this should be handled in lib389.

rebased onto 71f1647a2ee43dac8c0e6d67f05ec865b792c2db

3 years ago

rebased onto edff5fbb9699ad59819c30a40a028866526a65a7

3 years ago

These functions can be found in src/lib389/lib389/backend.py

But they are used in the CLI and UI and should not be removed.

These functions can be found in src/lib389/lib389/backend.py
But they are used in the CLI and UI and should not be removed.

Or, remove them and also fix/adjust the CLI and UI :-)

Thinking about it add_encrypted and del_encrypted are wrong to. You actually want:
ea = get_encrypted_attr(name)
ea.delete()
ea.create()
etc.
So to fix this.
remove add_encrypted_attr, del_encrypted_attr, get_encrypted_attrs, make it list_escrypted_attrs (return all of them), list_encrypted_attrs_name(return a list of strings of the cns) and get_encrypted_attr.

@firstyear Can't find any of the above-mentioned functions in the source, can you tell where are these, I have grepped it can't find anything like this. BTW rather than making changes to lib389 we can pass the correct DN, but I still think this should be handled in lib389.

You can't find them because I want you to make them ....

@aadhikari , please rebase it.
As for the refactoring encrypted attributes in lib389, let's open another ticket and fix it there. Refactoring is out of scope of this fix.

rebased onto e8c5583e9055869076763db3360dad4415f78397

3 years ago

@vashirov I have rebased and ran the test again, we can merge this, I will open a new issue for refactoring encrypted attributes in lib389. Thanks!

rebased onto 87338c1

3 years ago

Pull-Request has been merged by vashirov

3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3280

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

2 years ago