Mutated X.509 Certificate Chain #71
Conversation
dannyallover
changed the title
|
PTAL @sleevi |
There was a problem hiding this comment.
Some remarks about the shape and structure. I mostly focused on the .proto and .h
I think these are things you'd likely work out as you progressed through the mutations, so don't worry too much about fixing it up. I'm still behind on writing up the Friday conversation, so that's also entirely on me.
| message X509Chain { | ||
| repeated x509_certificate.X509Certificate certificates = 1; | ||
| } |
There was a problem hiding this comment.
It's not clear you need a distinct message type for this type. It should just be fine to have "repeated x509_certificate.X509Certificate" within the main MutatedX509Chain
There was a problem hiding this comment.
Yes, I agree with you and that was the initial intention. The reason why I ended up putting it in its own message was because I wanted to have an EncodeX509Chain helper to DER encode each certificate, and if it wasn't it's own message, then the argument for the function would have been google::protobuf::RepeatedPtrField which seemed bulky.
I realize that's not a good justification though. So I accepted your change!
| MutateSignatureOperation mutate_signature_operation = 1; | ||
| } | ||
| // |index| represents the position of the certificate that will be mutated. | ||
| required CertIndex index = 2; |
There was a problem hiding this comment.
I think that, as we flesh things out, the index is a property of the operation (that is, MutateSignatureOption should have the index member, rather than the Operation type)
Consider, for example, a "Copy Issuer from Cert X to the Subject of Cert Y" operation (or AKI/SKI). Those have both 'to' and 'from'.
|
|
||
| message MutateSignatureOperation { | ||
| // Allow certificate to be valid or invalid for fuzzing. | ||
| required bool is_valid = 1; |
|
|
||
| // Contains the positions of the certiciate chain that will be mutated. | ||
| enum CertIndex { | ||
| pos_0 = 0; |
There was a problem hiding this comment.
Can probably replace pos_0 with CERT_0 to offer more naming context.
| // Applies the |TYPE| operation to |x509| in order to create meaningful | ||
| // certificate chains. | ||
| #define DECLARE_APPLY_OPERATION_FUNCTION(TYPE) \ | ||
| void ApplyOperation(x509_certificate::X509Certificate& x509, \ | ||
| const TYPE& operation) | ||
|
|
||
| DECLARE_APPLY_OPERATION_FUNCTION(Operation); | ||
| DECLARE_APPLY_OPERATION_FUNCTION(MutateSignatureOperation); |
There was a problem hiding this comment.
Let's not prematurely introduce macros here. The macros I think will sort out once we've got several operations defined.
| // DER encodes each |certificate| in |x509_chain| and returns | ||
| // the encoded certificates in |der|. | ||
| std::vector<std::vector<uint8_t>> EncodeX509Chain(const X509Chain& x509_chain); |
There was a problem hiding this comment.
Do we need this method?
Getting rid of the message naturally means this also goes away, but even if the X509Chain message was kept, who would be the caller of this method?
If something is only an implementation detail, it's fine to leave it as a static function/unnamed namespace inside of the .cc file
The same applies to the mutations: do callers ever need to know that implementation detail?
In general, think of the .h as the API you're presenting the world; as much as possible, you want to limit what you support as part of the API because that becomes a thing you can't change (without breaking downstream users). You're right to have a "MutatedChainToDER" function, but that seems to be all you need for the .h?
There was a problem hiding this comment.
Thanks for this. I'm starting to understand now the purpose of a .h file and will refactor accordingly. Although, there are other functions in our other .h files that most likely would not be called outside of the .cc file and would probably also need to refactored later.
|
|
||
| #include "mutated_x509_chain.pb.h" | ||
|
|
||
| namespace mutated_x509_chain { |
There was a problem hiding this comment.
Mentioned on Friday, but don't put this in a separate namespace :) Recall we talked about just making this part of the x509_certificate namespace, because logically, this is part of mutating chains.
|
|
||
| import "x509_certificate.proto"; | ||
|
|
||
| package mutated_x509_chain; |
There was a problem hiding this comment.
On Friday, we talked about having this be part of the same x509_certificate package (recall, multiple protos can be part of the same package), and about removing the redundant "X509"
that is
package x509_certificate
message MutatedChain {
In the x509_certificate package we can talk a bit about Certificate and Chain as their context (X.509) is implied by the package.
There was a problem hiding this comment.
Great points here -- thank you for helping me simplify things and make things more cohesive.
|
Addressed comments @sleevi |
No description provided.