Skip to content

Pass PEMs through JSON as strings rather than byte-arrays#1374

Merged
achamayou merged 13 commits intomicrosoft:masterfrom
eddyashton:pems_args_as_strings
Jul 3, 2020
Merged

Pass PEMs through JSON as strings rather than byte-arrays#1374
achamayou merged 13 commits intomicrosoft:masterfrom
eddyashton:pems_args_as_strings

Conversation

@eddyashton
Copy link
Member

@eddyashton eddyashton commented Jul 2, 2020

Draft for now, should only be merged after integrating with #1370.

As mentioned in #1352, converting certs for new users and members to byte-arrays is cumbersome. This removes that burden - where we know the contents are a PEM, we will instead support a JSON string. We also currently support a byte-array, so any existing tooling should continue to work - we should discuss if its preferable to break this instead.

This makes proposal parameters human-readable. Since PEMs contain newlines that must be escaped in the JSON string, it still makes sense to construct and edit via Python or some other tool, but its now feasible to do by hand.

This touches a surprising amount of C++ code to prefer explicit tls::Pems over ambiguous std::vector<uint8_t>s, I'll call out the interesting bits inline.

struct MemberInfo : MemberPubInfo
struct MemberInfo
{
std::vector<uint8_t> cert; //< DER to match key in Certs table
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's talk about this one. Should this be a DER or a PEM? It's currently a DER, and I believe this is (as the comment says) so it matches the key used in the member Certs table. But as far as I can tell we never actually use this, there's no code dependency on these being identical. Instead, this object really only exists to store and present information externally, so perhaps it should store PEMs? As in, if a member makes query or read that is trying to list the active members, should they see the DER (which seems to be an internal implementation detail), or the PEM with which the member was added?

There was an odd confusion here previously: since cert is untyped, it actually held a DER when it was a MemberInfo but a PEM whenever we built a MemberPublicInfo (from JSON). I'm tempted to make an explicit DER type to avoid this, but a) we get almost the same thing by making all PEMs be explicitly tls::Pem and b) we have very few true DERs.

@ghost
Copy link

ghost commented Jul 2, 2020

pems_args_as_strings@10348 aka 20200703.28 vs master ewma over 50 builds from 9815 to 10345
images

@eddyashton eddyashton marked this pull request as ready for review July 3, 2020 12:28
@eddyashton eddyashton requested a review from a team as a code owner July 3, 2020 12:28
@achamayou achamayou merged commit bae6451 into microsoft:master Jul 3, 2020
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.

2 participants