feat: Refactor ParseDN function to fix resource usage and invalid parsings (fixes #487)#497
feat: Refactor ParseDN function to fix resource usage and invalid parsings (fixes #487)#497cpuschma merged 4 commits intogo-ldap:masterfrom cpuschma:master
ParseDN function to fix resource usage and invalid parsings (fixes #487)#497Conversation
|
Additionally, results of some benchmarking benchmark: Summary Old implementation: New implementation:: |
johnweldon
left a comment
There was a problem hiding this comment.
LGTM, except for the comment typo
| } | ||
|
|
||
| var rawValue asn1.RawValue | ||
| result, err := asn1.Unmarshal(decoded, &rawValue) |
There was a problem hiding this comment.
@cpuschma @johnweldon
Great to see you are improving this function!
There is one small issue with this approach that I also figured out too late: asn1.Unmarshal( only supports DER parsing, while the github.com/go-asn1-ber/asn1-ber library supports BER parsing too.
This means that parseDN deviates from the RFC, possibly resulting in unexpected limitations for the user.
There was a problem hiding this comment.
I'll look into this and also extend the test cases. We didn't catch on that either. Thank you for pointing this out, @inteon!
There was a problem hiding this comment.
I also have some extra testcases that might be useful. Will create a PR soon.
This PR reworks the
ParseDNfunction, which has been very resource consuming for reading one byte at a time and couldn't handle certain characters. This implementation is based upon inteons PR for cert-manager, which wasn't fully compatible with the RFC 4514.This PR also:
hexenchexto it's original namehexfor better readabiltyParseDNtest failuresIf this resolves the reported problems, we can revert #466.