Address Code-Review comments

Co-authored-by: Joo <[email protected]> Co-authored-by: Lila <[email protected]> Signed-off-by: Timo Klenk <[email protected]>
carbynestack · Apr 11, 2022 · 5c1f16e · 5c1f16e
1 parent a5309e8
commit 5c1f16e
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 23 deletions.
diff --git a/pkg/ephemeral/io/carrier.go b/pkg/ephemeral/io/carrier.go
@@ -60,7 +60,7 @@ func (c *Carrier) Connect(ctx context.Context, playerID int32, host string, port
 		return err
 	}
 	if playerID == 0 {
-		err = c.readSpec()
+		err = c.readPrime()
 		if err != nil {
 			return err
 		}
@@ -69,7 +69,17 @@ func (c *Carrier) Connect(ctx context.Context, playerID int32, host string, port
 	return nil
 }
 
-func (c Carrier) readSpec() error {
+// readPrime reads the file header from the MP-SPDZ connection
+// In MP-SPDZ connection, this will only be used when player0 connects as client to MP-SPDZ
+//
+// For the header composition, check:
+// https://github.com/data61/MP-SPDZ/issues/418#issuecomment-975424591
+//
+// It is made up as follows:
+//  - Careful: The other header parts are not part of this communication, they are only used when reading tuple files
+//  - length of the prime as 4-byte number little-endian (e.g. 16),
+//  - prime in big-endian (e.g. 170141183460469231731687303715885907969)
+func (c Carrier) readPrime() error {
 	const size = 4
 	readBytes := make([]byte, size)
 	_, err := io.LimitReader(c.Conn, size).Read(readBytes)

diff --git a/pkg/ephemeral/io/carrier_test.go b/pkg/ephemeral/io/carrier_test.go
@@ -64,7 +64,7 @@ var _ = Describe("Carrier", func() {
 		connectionOutput []byte //Will contain (length 4 byte, playerID 1 byte)
 		client, server   net.Conn
 		dialer           func(ctx context.Context, addr, port string) (net.Conn, error)
-		fakeTlsConnector func(conn net.Conn, playerID int32) (net.Conn, error)
+		fakeTLSConnector func(conn net.Conn, playerID int32) (net.Conn, error)
 	)
 	BeforeEach(func() {
 		secret = []amphora.SecretShare{
@@ -76,7 +76,7 @@ var _ = Describe("Carrier", func() {
 		dialer = func(ctx context.Context, addr, port string) (net.Conn, error) {
 			return client, nil
 		}
-		fakeTlsConnector = func(connection net.Conn, playerID int32) (net.Conn, error) {
+		fakeTLSConnector = func(connection net.Conn, playerID int32) (net.Conn, error) {
 			return connection, nil
 		}
 	})
@@ -89,7 +89,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			go server.Read(connectionOutput)
 			carrier.Connect(ctx, playerID, "", "")
@@ -105,7 +105,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			go server.Read(connectionOutput)
 			carrier.Connect(ctx, playerID, "", "")
@@ -122,7 +122,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			go server.Read(connectionOutput)
 			carrier.Connect(ctx, playerID, "", "")
@@ -144,7 +144,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       &packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			go server.Read(connectionOutput)
 			carrier.Connect(ctx, playerID, "", "")
@@ -166,7 +166,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       &packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			go server.Read(connectionOutput)
 			carrier.Connect(ctx, playerID, "", "")
@@ -181,7 +181,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			go server.Read(connectionOutput)
 			carrier.Connect(ctx, playerID, "", "")
@@ -205,7 +205,7 @@ var _ = Describe("Carrier", func() {
 			carrier := Carrier{
 				Dialer:       dialer,
 				Packer:       packer,
-				TLSConnector: fakeTlsConnector,
+				TLSConnector: fakeTLSConnector,
 			}
 			waitGroup := sync.WaitGroup{}
 			waitGroup.Add(1)

diff --git a/pkg/ephemeral/network/tls_connector.go b/pkg/ephemeral/network/tls_connector.go
@@ -6,19 +6,19 @@ import (
 	"net"
 )
 
-// NewTLSConnector creates a TLS connector Function in the default Path "Player-Data"
+// NewTLSConnector creates a TLS connector function in the default path "Player-Data".
 // Simply delegates to NewTLSConnectorWithPath
 func NewTLSConnector() func(conn net.Conn, playerID int32) (net.Conn, error) {
 	return NewTLSConnectorWithPath("Player-Data")
 }
 
-// NewTLSConnectorWithPath creates a new TLS connector Function.
-// The function will accept the Socket Connection and the PlayerID and upgrade it to a TLS encrypted one.
-// Will search for Certificates in the provided folder Path.
-// Certificates must be named in the format that MP-SPDZ uses (<Folder>/C<PlayerID>.pem and .key)
-func NewTLSConnectorWithPath(folderPath string) func(conn net.Conn, playerID int32) (net.Conn, error) {
+// NewTLSConnectorWithPath creates a new TLS connector function.
+// The function will accept the socket connection and the playerID and upgrade it to a TLS encrypted one.
+// Will search for certificates in the provided folder path.
+// Certificates must be named in the format that MP-SPDZ uses (<folder>/C<playerID>.pem and .key).
+func NewTLSConnectorWithPath(folder string) func(conn net.Conn, playerID int32) (net.Conn, error) {
 	return func(conn net.Conn, playerID int32) (net.Conn, error) {
-		tlsConfig, err := getTLSConfig(playerID, folderPath)
+		tlsConfig, err := getTLSConfig(playerID, folder)
 		if err != nil {
 			return nil, err
 		}
@@ -33,8 +33,8 @@ func NewTLSConnectorWithPath(folderPath string) func(conn net.Conn, playerID int
 	}
 }
 
-// getTLSConfig Loads the TLS Config for the provided PlayerId located in the given folder
-// Certificates must be named in the format that MP-SPDZ uses (<Folder>/C<PlayerID>.pem and .key)
+// getTLSConfig Loads the TLS config for the provided playerID located in the given folder.
+// Certificates must be named in the format that MP-SPDZ uses (<folder>/C<playerID>.pem and .key)
 func getTLSConfig(playerID int32, folder string) (*tls.Config, error) {
 	certFile := fmt.Sprintf("%s/C%d.pem", folder, playerID)
 	keyFile := fmt.Sprintf("%s/C%d.key", folder, playerID)
@@ -44,7 +44,8 @@ func getTLSConfig(playerID int32, folder string) (*tls.Config, error) {
 	}
 
 	tlsConfig := &tls.Config{
-		Certificates:       []tls.Certificate{cert},
+		Certificates: []tls.Certificate{cert},
+		// For future improvement, see https://github.com/carbynestack/ephemeral/issues/22
 		InsecureSkipVerify: true,
 	}
 	return tlsConfig, nil

diff --git a/pkg/ephemeral/player.go b/pkg/ephemeral/player.go
@@ -224,5 +224,4 @@ func (c *Callbacker) sendEvent(name, topic string, e interface{}) {
 	}
 	c.pb.PublishWithBody(name, topic, event, c.playerParams.GameID)
 	c.logger.Debugw("Sending event", "event", event, "topic", topic)
-	c.logger.Debugf("Sending event.name %v to topic %s\n", event.Name, topic)
 }
diff --git a/pkg/ephemeral/spdz.go b/pkg/ephemeral/spdz.go
@@ -211,7 +211,6 @@ func (s *SPDZEngine) Compile(ctx *CtxConfig) error {
 	var stdoutSlice []byte
 	var stderrSlice []byte
 	command := fmt.Sprintf("./compile.py -M %s", appName)
-	// TODO: ctx.context is nil at this time.
 	stdoutSlice, stderrSlice, err = s.cmder.CallCMD(context.TODO(), []string{command}, s.baseDir)
 	stdOut := string(stdoutSlice)
 	stdErr := string(stderrSlice)