Minor fixes and new support for multi-value attributes #20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It was by pure chance the tests passed before as maps are unordered, so iterating over the map to create the aggregate string will vary. This change makes the order of the aggregated string fixed. I used the same order that was expected in the tests.
Fully backwards compatible feature enhancement.
The RDN sequence for both subject and issuer is allowed to contain the
same attribute more than once. For example, Entrust certificates have
two OU attributes. Passing the new `multivalue: true` option to either
of the parse functions yields this output, where values for the subject
and issuer are now lists. In the example below, you can see both of the
OU values. Further, the aggregated string also includes both now.
%{
aggregated: "/C=US/CN=Entrust Certification Authority - L1K/O=Entrust, Inc./OU=See www.entrust.net/legal-terms, (c) 2012 Entrust, Inc. - for authorized use only",
CN: ["Entrust Certification Authority - L1K"],
C: ["US"],
L: [],
ST: [],
O: ["Entrust, Inc."],
OU: ["See www.entrust.net/legal-terms",
"(c) 2012 Entrust, Inc. - for authorized use only"],
emailAddress: []
}
Without the `multivalue` option, it parses is it did prior to this
change dropping all but the last of the duplicated attributes, which is
broken technically. In the spirit of backwards compatibility, however,
it parses the same as before:
%{
aggregated: "/C=US/CN=Entrust Certification Authority - L1K/O=Entrust, Inc./OU=(c) 2012 Entrust, Inc. - for authorized use only",
emailAddress: nil,
CN: "Entrust Certification Authority - L1K",
OU: "(c) 2012 Entrust, Inc. - for authorized use only",
O: "Entrust, Inc.",
ST: nil,
L: nil,
C: "US"
}
This is my first foray into Elixir and the first code I've ever written, so I'm not too familiar with the build system, but I was unable to do anything until I made these changes.
It looks like someone refactored their code, but forgot to update the new name of this option in the parse_pem function definition.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Disclaimer: This is my first piece of Elixir code. Be gentle please.]
This PR includes:
Fix to use a fixed order when printing the aggregated subject/issuer string. Maps are unordered, so it was only by chance that the existing tests passed. When I started working on this code, the tests failed immediately until I realized what was going on. So, now the aggregated string is created using a fixed order of attributes. The order I picked was to match how your unit tests were written (assuming that was your desired order).
Fully backwards compatible: Add new support for attributes that appear more than once in the subject/issuer RDN sequences. This happens with Entrust certificates where their issuing cert contains two OU attributes. The previous behavior was to simply discard all but the last value. The new code doesn't change this default behavior, but introduces a
multivalue: trueoption on the parser functions to correctly handle this situation. This requires changing the values of the subject/issuer maps to lists. See commit message for more details. I've added a new cert and several new tests as part of this change.Small tweaks to the build, which may or may not be the right way to handle things. As I mentioned, this is my first foray into Elixir. I couldn't get the project to build without updating those two files.
Fix a previous refactoring error by someone perhaps?
parse_pemhad an option calledreturn_base64, which was passed along toparse_der, which did not use that option. It looks like it was renamed toserializeinparse_der, so this fix just updates the name of the option inparse_pem.