- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9
Enable multiple policies type #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
67d33d2    to
    28bd138      
    Compare
  
    |  | ||
| message Identity { | ||
| // The identifier of the associated membership service provider | ||
| string msp_id = 1; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Is this field necessary? Can't we infer the MSP from the certificate's issuing CA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is part of Fabric. It is included in the identity. Hence, I have replicated the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found only one reference to this field in utils/signature/verify.go:
idBytes, err := msp.NewSerializedIdentity(s.Identity.MspId, s.Identity.GetCertificate())The question is, can we infer the MSP ID from s.Identity.GetCertificate()? Then pass it to NewSerializedIdentity().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may infer the MSP ID from the Subject: CN=peer0.org1.example.com, OU=peer, O=Hyperledger, ST=North Carolina, C=US in the certificate. I will create an issue to check this later and use the Fabric way for now till we complete the whole feature.
Note that the endorser would fill this Identity field.
7a06098    to
    c357994      
    Compare
  
    c357994    to
    3535672      
    Compare
  
    3535672    to
    29c6b17      
    Compare
  
    29c6b17    to
    4972f6b      
    Compare
  
    This commit updates the proto messages to define policy using various rules such as threshold, signature, and hierarchical. Signed-off-by: Senthil Nathan N <[email protected]> Signed-off-by: senthil <[email protected]>
Signed-off-by: senthil <[email protected]>
4972f6b    to
    e5d771f      
    Compare
  
    |  | ||
| oneof creator { | ||
| // The full raw bytes of the creator's certificate (e.g., an X.509 certificate). | ||
| bytes certificate= 2; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit:
| bytes certificate= 2; | |
| bytes certificate = 2; | 
| message PolicyItem { | ||
| string namespace = 1; | ||
| bytes policy = 2; | ||
| bytes policy = 2; // This holds the complete NamespacePolicy. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Originally, I used bytes policy to allow support for other policies.
The solution in this PR adds PolicyType to the NamespacePolicy, making it generic.
So I think it is best to use NamespacePolicy as the type of policy in the PolicyItem.
| ChannelID: c.SystemConfig.Policy.ChannelID, | ||
| OrdererEndpoints: endpoints, | ||
| MetaNamespaceVerificationKey: metaPolicy.PublicKey, | ||
| MetaNamespaceVerificationKey: key.PublicKey, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What additional changes are needed to remove the need for the MetaNamespaceVerificationKey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the LifecycleEndorsementPolicy field of config block, we need to add policy type to accept either the threshold rule or signature rule. Currently, it supports only the signature rule. Then, we can remove this extra field.
| message NamespacePolicy { | ||
| string scheme = 1; // The scheme for signature verification. | ||
| PolicyType type = 1; // The type of policy used. | ||
| bytes policy = 2; // The policy rule. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Can we use here oneof as well? This will remove the need for double marshalling and unmarshalling.
| Namespaces: validTxNamespaces, | ||
| Endorsements: make([]*protoblocktx.Endorsements, 2), // Too many signatures. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: The test should also address the "Not enough signatures" and "No signatures" test cases.
I suggest using a TX with two namespaces and only one signature for the "Not enough signatures".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check both use cases to ensure future implementations will not panic.
| // ThresholdVerifier verifies a digest. | ||
| type ThresholdVerifier interface { | ||
| Verify(Digest, Signature) error | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using policies.Policy and amending the verifiers accordingly.
| Namespaces: txNode.Tx.Namespaces, | ||
| Signatures: txNode.Signatures, | ||
| Namespaces: txNode.Tx.Namespaces, | ||
| Endorsements: txNode.Signatures, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| Endorsements: txNode.Signatures, | |
| Endorsements: txNode.Endorsements, | 
| TransactionNode struct { | ||
| Tx *protovcservice.Tx | ||
| Signatures [][]byte | ||
| Signatures []*protoblocktx.Endorsements | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor
| Signatures []*protoblocktx.Endorsements | |
| Endorsements []*protoblocktx.Endorsements | 
| set := &protoblocktx.Endorsements{ | ||
| EndorsementsWithIdentity: []*protoblocktx.EndorsementWithIdentity{{Endorsement: sig}}, | ||
| } | ||
| sets = append(sets, set) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: my personal preference is to avoid declaring variables that are only used once whenever possible.
But it is just a suggestion.
| set := &protoblocktx.Endorsements{ | |
| EndorsementsWithIdentity: []*protoblocktx.EndorsementWithIdentity{{Endorsement: sig}}, | |
| } | |
| sets = append(sets, set) | |
| sets = append(sets, &protoblocktx.Endorsements{ | |
| EndorsementsWithIdentity: []*protoblocktx.EndorsementWithIdentity{{Endorsement: sig}}, | |
| }) | 
| // identity (MSP ID and certificate). This is used when a set of signatures | ||
| // must all be present to satisfy a rule (e.g., an AND condition). | ||
| func CreateEndorsementsForSignatureRule(signatures, mspIDs, certBytes [][]byte) *protoblocktx.Endorsements { | ||
| set := &protoblocktx.Endorsements{} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor
| set := &protoblocktx.Endorsements{} | |
| set := &protoblocktx.Endorsements{ | |
| EndorsementsWithIdentity: make([]*protoblocktx.EndorsementWithIdentity, 0, len(signatures), | |
| } | 
Type of change
Description
This commit updates the proto messages to define policy using various rules such as threshold, signature, and hierarchical. Further, it integrates the signature rules with the verifier component.
Related issues