Mutated Chain with Trust Parameter

[dannyallover]

Continuation off of #71.

Added a parameter for each certificate that indicates if it's trusted or not.

[dannyallover]

PTAL @sleevi

[sleevi]

Hey Danny,

Sorry for the delays! I think I mentioned I was going to be OOO, but now I'm back and picking these up. As I mention in the review comments below, I'm working forward (in terms of what needs to land first), but also trying to "think backwards" (in terms of how stuff will be used).

So this round of comments is mostly my unvarnished take based on the protobuf and the headers, but I haven't paged in the full picture. As such, it's not a direct call for changes in some of these areas (you'll see which), but definitely wanting to make sure we're on the same page and to get your thoughts about some of the design elements, so we can figure out how to document them.

[sleevi]

Sorry, I was out of the office on vacation, which means I've definitely forgotten more than I remember, but to try and recap:

• We talked about how 'trusted' isn't really a property of the DER, it's more a property of the target API being fuzzed
  • That is, the target API has a notion of "trusted" or not, as well as things like purpose, hostname to be verified, etc
• This is why it doesn't belong as a property of the X509Certificate; it's not really an encoding option
• Because it's not a property of the X509Certificate, it also doesn't lend itself to being in mutations`
  • This is because when we apply a Mutation, what we're doing is structurally modifying one (or more) X509Certificates in chain to expressing something in the proto schema that is intended to match the ASN.1

Does this match your memory as well?

What I'm wondering here is whether we should split further, based on separating out the "encoding" (which may fail to decode on the target-under-fuzzing side) and the API parameters. I think we talked a little about this in your final week, about the tension between target-specific API parameters (e.g. BoringSSL vs NSS API parameters) and those generic to RFC 5280, but that are still not encoding parameters (such as the set of trust anchors, or trusted policy OIDs, etc).

Concretely, I'm curious for your thoughts on

message MutatedChain {
  repeated x509_certificate.X509Certificate chain = 1;
  repeated Mutation mutations = 2;
}

message TrustedChain {
  MutatedChain chain = 1;
  repeated TrustedCertificate certs = 2;
}

message TrustedCertificate {
  required CertIndex index = 1;
}

I'm not 100% sold on this; it might be coupling the Protobuf layout too much to how the code would be structured, so I'm curious your thoughts. But the reason for that proposal above is:

1. MutatedChain only handles "things expressed via DER"
2. TrustedChain (for lack of a better naming) contains the set of generic-to-all-libraries API parameters, like those talked about in RFC 5280, Section 6.1.1 (handwave handwave)
3. We let TrustedCertificate act as all the trust parameter-specific configuration (6.1.1 (d) from RFC 5280)

The reason I'm not entirely sold is I'm trying to work backwards, from a "here's specifically how a target will use this" (e.g. BoringSSL, NSS), and then trying to make sure we have an API that makes sense for that, and if we have an API that makes sense, that we have the Protobufs that make sense for it. That is, from a design perspective, we "work backwards" to work out the design, but since I'm reviewing it, I'm having to "work forward" (first the protobuf, then the API, then the target integration). So there's a lot of state to keep in mind, and I haven't paged it all in, but wanted to share my initial thoughts.

Note: Not asking you to make changes yet, especially as I page in all the reviews, but wanted to share initial thoughts. If we do keep it as-is, which could be perfectly fine, I think we want to add documentation here that talks about a "Here's how you use this" that also reflects how we'll extend it, so we've got a bit of a coherent design.

[sleevi]

With the design I mentioned above, I think we can omit trusted here, since its mere preference implies a certificate is trusted.

That said, with your current API, I think it's not unreasonable to keep, so that the fuzzer won't just make every cert trusted :)

[sleevi]

Suggested change

	CERT_4 = 4;
	CERT_4 = 4;
	CERT_5 = 5;
	CERT_6 = 6;
	CERT_7 = 7;
	CERT_8 = 8;
	CERT_9 = 9;

"10 certs should be enough for anyone" - Bill Gates

[sleevi]

Suggested change

	struct X509 {
	struct FuzzedCertificate {

I don't recall if we used that name yet? Just thought it might be a better name? Definitely one of the hard problems of computer science, so take what you will.

My concern with "X509" is that it isn't that descriptive, especially with the numbers, and so trying to find something easier for folks to comprehend. Of course, this may change in time.

[sleevi]

Suggested change

	namespace {

We should move all the 'utility' functions (those not part of the header) within an unnamed namespace, to prevent the symbols from triggering the One Definition Rule

That is, it's a nested unnamed namespace within the x509_certificate namespace.

This is discussed in the Google style guide at https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

[sleevi]

Suggested change

	void Mutate(const Mutation& mutation,
	void ApplyMutation(const Mutation& mutation,

[sleevi]

Suggested change

	void Mutate(const MutateSignature& mutation,
	void MutateSignature(const MutateSignature& mutation,

Combined with my suggestion below, trying to make it explicitly clear in the code that these are different mutations that are happening; that is, trying not to use overloads.

[sleevi]

Suggested change

	void SetTrust(
	void ApplyTrustParameters(

Handwavy naming, but "SetTrust" felt a little less descriptive.

[sleevi]

Suggested change

	} // namespace

This is where the unnamed namespace would end, since only lines 84 - 104 are relevant.

[sleevi]

Suggested change

	auto chain = mutated_chain.chain();
	auto trust_parameters = mutated_chain.trust_parameters();
	auto mutations = mutated_chain.mutations();
	if (chain.empty()) {
	return {{}};
	}
	for (const auto& mutation : mutations) {
	Mutate(mutation, chain);
	}
	std::vector<X509> encoded_mutated_chain;
	for (const auto& encoded_cert : EncodeChain(chain)) {
	encoded_mutated_chain.push_back({encoded_cert, false});
	}
	SetTrust(encoded_mutated_chain, trust_parameters);
	std::vector<X509> encoded_chain;
	// Copy the chain, to allow for local modification
	auto chain = mutated_chain.chain();
	if (chain.empty()) {
	return encoded_chain;
	}
	// Apply all pending mutations to the local copy.
	for (const auto& mutation : mutated_chain.mutations()) {
	ApplyMutation(mutation, chain);
	}
	// Encode the local copy to DER.
	for (const auto& encoded_cert : EncodeChain(chain)) {
	encoded_chain.push_back({encoded_cert, false});
	}
	// Update trust settings.
	ApplyTrustParameters(encoded_chain, mutated_chain.trust_parameters());

Mostly, I'm restructuring according to https://google.github.io/styleguide/cppguide.html#Local_Variables , to try to declare it before it's used, as well as avoid unnecessary locals, especially when they may end up copying fields (which I don't believe is what you intended, except for chain).

[jvoisin]

@dannyallover any plans to keep on working on this, or shall we close this PR?