[FAB-17467] Create signature for config update #687
Conversation
pkg/config/signer.go
Outdated
| // NewSigner creates a new signer for configuration updates. The publicCert | ||
| // and privateKey should be pem encoded. | ||
| func NewSigner(publicCert []byte, privateCert []byte, mspID string) (*Signer, error) { | ||
| // NewSigningIdentity creates a new signingIdentity for configuration updates. |
There was a problem hiding this comment.
nit: I feel like this should just read as English and not reference the type, i.e. "signing identity". If we feel it's best to include the type though, let's make sure the capitalization matches. :)
There was a problem hiding this comment.
+1 @wlahti The term signer and signer's public/private key are widely used as far as I know based on the famous crypto book that I have been reading -- Applied Cryptography: Protocols, Algorithms and Source Code in C. Hence, Signer looks more natural to me than SigningIdentity here. This is similar to having Validator or Committer as the struct name (which we have used).
There was a problem hiding this comment.
A signer signs things; this is more than a signer as it also includes the MSPID and the certificate that goes into the the signature header. The overloaded use of “signer” also adds confusion with respect to crypto.Signer from the standard library.
Within Fabric, across components, we refer to this collection of identity, cert, and key as a SigningIdentity. Hence the original request to change.
Line 78 in 43ff898
I believe Will’s comment was that the godoc should use the term “signing identity” instead of the type; if the type is used, it should at least be done in consistent case. He can clarify if I’m wrong.
There was a problem hiding this comment.
Correct, I was simply referring to the godoc in my previous comment. Things look good to me now.
There was a problem hiding this comment.
Got it. Thanks, @sykesm, for the detailed explanation.
There was a problem hiding this comment.
Even though GetSigningIdentity exists in the MSP interface, in my opinion it creates an un-needed coupling between signing and verification.
What if you have an idemix MSP for a specific organization OrgA but not for the rest of the organizations? If a peer from OrgB now gets an API call to verify an idemix signature it needs to ask from the idemix MSP, which means an instance of the said MSP exists, however - the implementation of the idemix MSP on any org other than OrgA is undefined since only OrgA can issue idemix proofs.
If you ask me - the signing part should have never been placed inside the MSP interface, but it should have been in its own interface.
pkg/config/config.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| sh, err := proto.Marshal(signatureHeader) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal signature header: %v", err) |
There was a problem hiding this comment.
Nit. Need to avoid fmt.Errorf() as per error handling guideline. Use github.com/pkg/errors package instead.
There was a problem hiding this comment.
We're treating this package as a library and are avoiding as many dependencies as possible as well as fabric-specific error/logging preferences.
There was a problem hiding this comment.
As Will stated, we are avoiding dependencies and projecting our coding standards into consuming environments.
pkg/config/signer.go
Outdated
| // NewSigner creates a new signer for configuration updates. The publicCert | ||
| // and privateKey should be pem encoded. | ||
| func NewSigner(publicCert []byte, privateCert []byte, mspID string) (*Signer, error) { | ||
| // NewSigningIdentity creates a new signingIdentity for configuration updates. |
There was a problem hiding this comment.
+1 @wlahti The term signer and signer's public/private key are widely used as far as I know based on the famous crypto book that I have been reading -- Applied Cryptography: Protocols, Algorithms and Source Code in C. Hence, Signer looks more natural to me than SigningIdentity here. This is similar to having Validator or Committer as the struct name (which we have used).
DereckLuo
force-pushed
the
FAB-17467
branch
3 times, most recently
from
0d0c2a4
to
442b26b
Compare
| @@ -44,13 +53,3 @@ func concatenateBytes(data ...[]byte) []byte { | |||
| } | |||
There was a problem hiding this comment.
Looking up a few lines, concatenateBytes looks very inefficient. Why isn’t as simple as:
func concatenateBytes(data ...[]byte) []byte {
var res []byte
for i := range data {
res = append(res, data[i]...)
}
return res
}
pkg/config/signer.go
Outdated
| // Signer holds the information necessary to sign a configuration update. | ||
| // It implements the crypto.Signer interface for ECDSA keys. | ||
| type Signer struct { | ||
| // SigningIdentity holds the information necessary to sign a configuration update. |
There was a problem hiding this comment.
If you were a consumer, would this description of a SigningIdentity (or an Identity, or a Signer) be enough to help you understand what it is? I’d expect it to say something like:
A SigningIdentity is an MSP Identity that can be used to sign configuration updates and transaction.
We can elaborate on that later but we want to focus on what it is and its role with respect to consumers, not the implementation.
Signed-off-by: Chongxin Luo <Chongxin.Luo@ibm.com>
|
rebased to current master. will address additional requests in a separate PR. |
Signed-off-by: Chongxin Luo Chongxin.Luo@ibm.com
Type of change
Description
Additional details
Related issues
FAB-17467