Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BitString initialization via namedValues #32

Merged
merged 1 commit into from Mar 28, 2017

Conversation

nekolyanich
Copy link
Contributor

  class KeyUsage(BitString):
      namedValues = NamedValues(
          ('digitalSignature', 0),
          ('nonRepudiation', 1),
          ('keyEncipherment', 2),
          ('dataEncipherment', 3),
          ('keyAgreement', 4),
          ('keyCertSign', 5),
          ('cRLSign', 6),
          ('encipherOnly', 7),
          ('decipherOnly', 8),
      )

  encode(KeyUsage('keyCertSign,cRLSign')) != encode(KeyUsage("'0000011'B"))
                                                                    ^^
                                                                    ||
                                                bit #5 -------------+|
                                                bit #6 --------------+

  class KeyUsage(BitString):
      namedValues = NamedValues(
          ('digitalSignature', 0),
          ('nonRepudiation', 1),
          ('keyEncipherment', 2),
          ('dataEncipherment', 3),
          ('keyAgreement', 4),
          ('keyCertSign', 5),
          ('cRLSign', 6),
          ('encipherOnly', 7),
          ('decipherOnly', 8),
      )

  encode(KeyUsage('keyCertSign,cRLSign')) != encode(KeyUsage("'0000011'B"))
                                                                    ^^
                                                                    ||
                                                bit etingof#5 -------------+|
                                                bit etingof#6 --------------+
@codecov-io
Copy link

codecov-io commented Mar 28, 2017

Codecov Report

Merging #32 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   82.03%   82.04%   +0.01%     
==========================================
  Files          25       25              
  Lines        3590     3593       +3     
==========================================
+ Hits         2945     2948       +3     
  Misses        645      645
Impacted Files Coverage Δ
pyasn1/type/univ.py 77.92% <100%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf038d4...b3a09b8. Read the comment docs.

@etingof
Copy link
Owner

etingof commented Mar 28, 2017

Hey, thank you for your PR!

Would you mind explaining why do you think this KeyUsage('keyCertSign,cRLSign') should turn into 0000011 rather than into 1100000 as it currently does? Assuming keyCertSign is bit #5 and cRLSign is bit #6, both seem to reside where I'd expect them to, no?

Here is what happens when I run your test case over current master:

>>> KeyUsage('keyCertSign,cRLSign')
KeyUsage('1100000')
>>> encode(KeyUsage('keyCertSign,cRLSign')), encode(KeyUsage("'0000011'B"))
(b'\x03\x02\x01\xc0', b'\x03\x02\x01\x06')

@nekolyanich
Copy link
Contributor Author

Thank you for quick response. Right encoded value for KeyUsage('keyCertSign,cRLSign') is b'\x03\x02\x01\x06'. For example GeoTrusts Global CA (RootCA for google.com) has this permissions in KeyUsage.

@nekolyanich
Copy link
Contributor Author

FYI pyasn1-v0.2.2 have valid behavior

@etingof
Copy link
Owner

etingof commented Mar 28, 2017

Ah, you are right! The ordering of the bits initializer is reversed compared to hex, for example. So I'm going to merge you fix now to make pyasn1readily operational.

Later on I'll look into optimizing out list operation (which is slow), add more tests to capture such regressions and push out a release.

Thank you!

@etingof etingof merged commit 1e07910 into etingof:master Mar 28, 2017
@etingof
Copy link
Owner

etingof commented Mar 28, 2017

So I refactored the fix a little. Hope to release 0.2.4 by the end of this week. Thank you!

hugovk pushed a commit to hugovk/pyasn1 that referenced this pull request Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants