Separate Signer from Keymaster operations #497

Merged
merged 2 commits into from Feb 17, 2017

Projects

None yet

2 participants

@gdbelvin
Collaborator

Establishes a 1:1 relationship between the signer and
DigitallySigned.

Keymaster operations on groups of signers and verifiers are
in a separate package with a separate protobuf serialization.

@gdbelvin gdbelvin requested a review from cesarghali Feb 16, 2017
@cesarghali

Great PR, thanks! I just added few comments.

It makes more sense now to have keymaster and signatures libraries separate.

core/crypto/keymaster/interface.go
+
+import (
+ "github.com/google/keytransparency/core/crypto/signatures"
+ kmpb "github.com/google/keytransparency/core/proto/keymaster"
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Please separate protos from other imports. This way we have consistency through the codebase:

Import groups:

  • Standard libraries.
  • keytransparency libraries.
  • Other third-party libraries.
  • Protos.
@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

done

@@ -130,10 +133,11 @@ func (s *KeyMaster) Marshal() ([]byte, error) {
// AddSigningKey adds a new private key to the store.
func (s *KeyMaster) AddSigningKey(status kmpb.SigningKey_KeyStatus, description string, key []byte) (string, error) {
- signer, err := factory.NewSigner(key, time.Now(), description, status)
+ sig, err := factory.NewSignerFromPEM(key)
@cesarghali
cesarghali Feb 17, 2017 Collaborator

How about calling this variable s to be consistent with the rest of the code. You can keep it sig if you want, it's not very important.

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

I can't call it s here because that conflicts with s *KeyMaster

+
+// NewSigner creates a signer object from a private key.
+func NewSigner(s signatures.Signer, addedAt time.Time,
+ description string, status kmpb.SigningKey_KeyStatus) Signer {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

+1 on returning the interface instead of the object :-)

core/crypto/keymaster/signer.go
+}
+
+// SignerFromRawKey creates a signer object from given raw key bytes.
+func SignerFromRawKey(b []byte) (signatures.Signer, error) {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Please consider starting the name of all functions that create keymaster.Signer, keymaster.Verifier, signatures.Signer, and signatures.Verifier objects with New, e.g., NewSignerFromPEM, NewVerifierFromRawKey, etc. We currently have some of these functions start with New and others don't. I might have introduced this inconsistency when I worked on the signatures library a while ago and now I can see that it might cause some confusion in the future.

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

I've gone through and renamed all the ones I know about. Please let me know if I've missed anything.

+-----END EC PRIVATE KEY-----`
+)
+
+func TestSignerFromPEM(t *testing.T) {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Can you please add tests for the other functions in core/crypto/keymaster/signer.go.

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

Done

core/crypto/keymaster/verifier.go
+
+// VerifierFromPEM parses a PEM formatted block and returns a verifier object
+// created using that block.
+func VerifierFromPEM(pemKey []byte) (Verifier, error) {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Please add New to the beginning of the function name.

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

Done

core/crypto/keymaster/verifier.go
+}
+
+// VerifierFromKey creates a verifier object from a PublicKey proto object.
+func VerifierFromKey(key *tpb.PublicKey) (Verifier, error) {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Please add New to the beginning of the function name.

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

Done

core/crypto/keymaster/verifier.go
+}
+
+// VerifierFromRawKey creates a verifier object from given raw key bytes.
+func VerifierFromRawKey(b []byte) (Verifier, error) {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Please add New to the beginning of the function name.

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

Done

@@ -12,23 +12,16 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package factory
+package keymaster
@cesarghali
cesarghali Feb 17, 2017 Collaborator

Can you please add tests for the other functions in Verifier as well?

@gdbelvin
gdbelvin Feb 17, 2017 Collaborator

done

-func signerFromBytes(b []byte, addedAt time.Time, description string, status kmpb.SigningKey_KeyStatus) (signatures.Signer, error) {
- if _, err := x509.ParsePKCS1PrivateKey(b); err == nil {
+ switch pkType := k.(type) {
@cesarghali
cesarghali Feb 17, 2017 Collaborator

๐Ÿ‘

@cesarghali

Please see my comments above.

gdbelvin added some commits Feb 16, 2017
@gdbelvin gdbelvin Separate Signer from Keymaster operations
Establishes a 1:1 relationship between the signer and
DigitallySigned.

Keymaster operations on groups of signers and verifiers are
in a separate package with a separate protobuf serialization.
057952a
@gdbelvin gdbelvin Reviewer comments
1c78218
@cesarghali

LGTM. Thanks for this PR.

@cesarghali cesarghali merged commit 7e09e1a into google:master Feb 17, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment