CMS.py Contains TWO Conflicting Definitions of RecipientKeyIdentifier

[cjamescook]

cms.py contains TWO definitions of "class RecipientKeyIdentifier". Unfortunately, the first defines the field "subject_key_identifier" and the latter defines the field "subjectKeyIdentifier". This snowballs, causing problems due to Python binding time rules. That is, the definition of KeyAgreementRecipientIdentifier picks up the former definition, but a program that imports cms.py will pick up the latter.

To wit, the following fails:

>>> temp=RecipientKeyIdentifier()
>>> temp['subjectKeyIdentifier'] = b'12345678'
>>> KeyAgreementRecipientIdentifier(name='r_key_id', value=temp)

The following works, but it took me over a day to figure out what was going on:

>>> temp.native
OrderedDict([('subjectKeyIdentifier', b'12345678'), ('date', None), ('other', None)])
>>> temp2=util.OrderedDict([('subject_key_identifier', b'12345678'), ('date', None), ('other', None)])
>>> KeyAgreementRecipientIdentifier(name='r_key_id', value=temp2)

[geitda]

Commit f003c67 erroneously duplicated the RecipientKeyIdentifier class, which effectively changed the field name from underscores to camelCase, and gums things up as @cjamescook noted. Deleting the duplicate and sticking to using 'subject_key_identifier' seems to make any issues go away, but this might break any other code written since 2019 that interfaces that class with the 'subjectKeyIdentifier' name.
Looks like there aren't any tests to touch RecipientKeyIdentifier, KeyAgreementRecipientIdentifier, or SMIMEEncryptionKeyPreference so that's why this fell through the cracks.
I may submit a PR that removes the duplicate and adds some tests if there's community interest for it and I get enough free time.

[wbond]

I'll have to add some code to work-around the new name.

[wbond]

I have a PR at #266 that should work around this issue.

@cjamescook @geitda Do either of you have an interest in helping provide some sort of example file that we could use a test fixture for this structure?

Alternatively @commonism since you originally added this code, do you have something we could use for a test?

[commonism]

Hi,

IIRC the structures were required to parse SMIME signatures from Outlook signed mails.
I never had to access the fields - I do not have code which will change in behavior when the field gets renamed.
I did not have outlook, could not include foreign signatures and failed to create them manually, so there is no tests on this.

Maybe consider using https://pre-commit.ci/ to make use of something like black/pylint/flake8… to detect re-definition/* and block merging. pre-commit is great.

[commonism]

Possible to create something from
https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-oxoabk/2653a83f-ace5-4543-8908-d6557e2f6ed6
https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-oxoabk/5cac7bfa-cb9e-42a6-9d22-84a3b6f155d3
?

edit

the binary form mismatches the data in the text dump and does not include the OID

    0:d=0  hl=4 l= 623 cons: SEQUENCE          
    4:d=1  hl=4 l= 472 cons: SEQUENCE          
    8:d=2  hl=2 l=   3 cons: cont [ 0 ]        
   10:d=3  hl=2 l=   1 prim: INTEGER           :02
   13:d=2  hl=2 l=  16 prim: INTEGER           :3F924830235200BC496308CD3BAEB76B
   31:d=2  hl=2 l=  13 cons: SEQUENCE          
   33:d=3  hl=2 l=   9 prim: OBJECT            :sha1WithRSAEncryption
   44:d=3  hl=2 l=   0 prim: NULL              
   46:d=2  hl=2 l=  21 cons: SEQUENCE          
   48:d=3  hl=2 l=  19 cons: SET               
   50:d=4  hl=2 l=  17 cons: SEQUENCE          
   52:d=5  hl=2 l=   3 prim: OBJECT            :commonName
   57:d=5  hl=2 l=  10 prim: PRINTABLESTRING   :RSACERTSRV
   69:d=2  hl=2 l=  30 cons: SEQUENCE          
   71:d=3  hl=2 l=  13 prim: UTCTIME           :060321053129Z
   86:d=3  hl=2 l=  13 prim: UTCTIME           :160321054111Z
  101:d=2  hl=2 l=  21 cons: SEQUENCE          
  103:d=3  hl=2 l=  19 cons: SET               
  105:d=4  hl=2 l=  17 cons: SEQUENCE          
  107:d=5  hl=2 l=   3 prim: OBJECT            :commonName
  112:d=5  hl=2 l=  10 prim: PRINTABLESTRING   :RSACERTSRV
  124:d=2  hl=3 l= 159 cons: SEQUENCE          
  127:d=3  hl=2 l=  13 cons: SEQUENCE          
  129:d=4  hl=2 l=   9 prim: OBJECT            :rsaEncryption
  140:d=4  hl=2 l=   0 prim: NULL              
  142:d=3  hl=3 l= 141 prim: BIT STRING        
  286:d=2  hl=3 l= 191 cons: cont [ 3 ]        
  289:d=3  hl=3 l= 188 cons: SEQUENCE          
  292:d=4  hl=2 l=  11 cons: SEQUENCE          
  294:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Key Usage
  299:d=5  hl=2 l=   4 prim: OCTET STRING      [HEX DUMP]:03020186
  305:d=4  hl=2 l=  15 cons: SEQUENCE          
  307:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Basic Constraints
  312:d=5  hl=2 l=   1 prim: BOOLEAN           :255
  315:d=5  hl=2 l=   5 prim: OCTET STRING      [HEX DUMP]:30030101FF
  322:d=4  hl=2 l=  29 cons: SEQUENCE          
  324:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Subject Key Identifier
  329:d=5  hl=2 l=  22 prim: OCTET STRING      [HEX DUMP]:041471DDD1A9BB0D7EE05F82FC9DC59D7A5E098E41EF
  353:d=4  hl=2 l= 107 cons: SEQUENCE          
  355:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 CRL Distribution Points
  360:d=5  hl=2 l= 100 prim: OCTET STRING      [HEX DUMP]:30623060A05EA05C862B687474703A2F2F727361636572747372762F43657274456E726F6C6C2F525341434552545352562E63726C862D66696C653A2F2F5C5C727361636572747372765C43657274456E726F6C6C5C525341434552545352562E63726C
  462:d=4  hl=2 l=  16 cons: SEQUENCE          
  464:d=5  hl=2 l=   9 prim: OBJECT            :1.3.6.1.4.1.311.21.1
  475:d=5  hl=2 l=   3 prim: OCTET STRING      [HEX DUMP]:020100
  480:d=1  hl=2 l=  13 cons: SEQUENCE          
  482:d=2  hl=2 l=   9 prim: OBJECT            :sha1WithRSAEncryption
  493:d=2  hl=2 l=   0 prim: NULL              
  495:d=1  hl=3 l= 129 prim: BIT STRING    

[wbond]

This has been fixed as of 8609892 and will be part of the next release