-
Notifications
You must be signed in to change notification settings - Fork 711
Add decoding and encoding of ObjectIdentifier
ASN.1 records
#1849
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1849 +/- ##
==========================================
+ Coverage 82.96% 83.00% +0.04%
==========================================
Files 285 285
Lines 49063 49257 +194
Branches 10599 10459 -140
==========================================
+ Hits 40707 40888 +181
- Misses 7196 7567 +371
+ Partials 1160 802 -358
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ObjectIdentifier
ASN.1 recordsObjectIdentifier
ASN.1 records
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Programming-wide LGTM.
std::vector<uint32_t> components; | ||
|
||
uint8_t firstByte = data[currentByteIndex++]; | ||
components.push_back(static_cast<uint32_t>(firstByte / 40)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider explaining what the 40 magic number is in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation here: d690947
{ | ||
uint8_t byte = data[currentByteIndex++]; | ||
|
||
currentComponentValue = (currentComponentValue << 7) | (byte & 0x7f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave some comments here? Like doing which step in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation here: d690947
As part of #1835, X509 certificates require support for ASN.1 records of type
ObjectIdentifier