Handshaker component, part 3: Handshaker newConn impl and tests

[davisgu]

waiting for #31

---

This change is 

[matthewstevenson88]

Thanks for the PR Davis! I left a few small comments.

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @davisgu and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 63 at r3 (raw file):

	// authorization check by the S2A if it is provided.
	TargetName string
	// HandshakerServiceAddress stores the address of the S2A handshaker service.

s/stores/is

---

security/s2a/internal/handshaker/handshaker.go, line 228 at r3 (raw file):

	}

	var hsAddr string

Could we hide this assignment behind a getHandshakerServiceAddress() API?

---

security/s2a/internal/handshaker/handshaker.go, line 234 at r3 (raw file):

		hsAddr = h.serverOpts.HandshakerServiceAddress
	}
	newConn, err := crypter.NewConn(&crypter.ConnOptions{

Please handle err appropriately.

[davisgu]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 63 at r3 (raw file):

Previously, matthewstevenson88 wrote…

s/stores/is

Done.

---

security/s2a/internal/handshaker/handshaker.go, line 228 at r3 (raw file):

Previously, matthewstevenson88 wrote…

Could we hide this assignment behind a getHandshakerServiceAddress() API?

Done.

---

security/s2a/internal/handshaker/handshaker.go, line 234 at r3 (raw file):

Previously, matthewstevenson88 wrote…

Please handle err appropriately.

Done.

[davisgu]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker_test.go, line 302 at r4 (raw file):

			return errors.New("Authinfo s2a context incorrect")
		}
		if reflect.ValueOf(newConn).Elem().Field(0).Interface() != chs.conn {

Is there a better way to check this? This is the best I could come up with... @matthewstevenson88

[matthewstevenson88]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @davisgu)

---

security/s2a/internal/handshaker/handshaker.go, line 228 at r4 (raw file):

	}

	newConn, err := crypter.NewConn(&crypter.ConnOptions{

Please add a comment describing what this call is doing.

---

security/s2a/internal/handshaker/handshaker.go, line 246 at r4 (raw file):

}

func (h *s2aHandshaker) getHandshakerServiceAddress() {

Should we add a return type to this function?

Also, can we move this function to the very end of the file, since it is just a utility function?

---

security/s2a/internal/handshaker/handshaker_test.go, line 302 at r4 (raw file):

Previously, davisgu wrote…

Is there a better way to check this? This is the best I could come up with... @matthewstevenson88

After thinking about this for a bit, I think we should simplify the check and only check that newConn is not nil. The reason for this is: the handshaker is only responsible for creating a non-nil conn object, and it is not responsible for its function. That should be tested in the record protocol's unit tests. WDYT?

[davisgu]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 228 at r4 (raw file):

Previously, matthewstevenson88 wrote…

Please add a comment describing what this call is doing.

Done.

---

security/s2a/internal/handshaker/handshaker.go, line 246 at r4 (raw file):

Previously, matthewstevenson88 wrote…

Should we add a return type to this function?

Also, can we move this function to the very end of the file, since it is just a utility function?

Done.

---

security/s2a/internal/handshaker/handshaker_test.go, line 302 at r4 (raw file):

Previously, matthewstevenson88 wrote…

After thinking about this for a bit, I think we should simplify the check and only check that newConn is not nil. The reason for this is: the handshaker is only responsible for creating a non-nil conn object, and it is not responsible for its function. That should be tested in the record protocol's unit tests. WDYT?

sounds good!

[matthewstevenson88]

Approved with 1 nit. Thanks Davis!

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @davisgu)

---

security/s2a/internal/handshaker/handshaker.go, line 227 at r5 (raw file):

		return nil, nil, err
	}
	// Create a new net.Conn with updated information.

s/net.Conn/TLS record protocol

s/with update information/using the SessionResult.

[davisgu]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @davisgu and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 227 at r5 (raw file):

Previously, matthewstevenson88 wrote…

s/net.Conn/TLS record protocol

s/with update information/using the SessionResult.

Done.

[cesarghali]

Reviewed 3 of 4 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @davisgu and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 64 at r5 (raw file):

	TargetName string
	// HandshakerServiceAddress is the address of the S2A handshaker service.
	HandshakerServiceAddress string

This is not a handshaker option and since it needed in the record protocol, maybe we should pass it as a parameter to New*. WDYT?

Same below.

@matthewstevenson88 FYI.

---

security/s2a/internal/handshaker/handshaker.go, line 313 at r5 (raw file):

}

func (h *s2aHandshaker) getHandshakerServiceAddress() string {

If we add the handshaker service address to New*, we would store it as part of s2aHandshaker. So there won't be a need for this API.

---

security/s2a/internal/handshaker/handshaker_test.go, line 302 at r6 (raw file):

		}
		if newConn == nil {
			return errors.New("Expected non-nil net.Conn")

Please change the test to make this error message and the one below a bit different.

---

security/s2a/internal/record/record.go, line 59 at r6 (raw file):

// ConnOptions holds the options used for creating a new conn object.
type ConnOptions struct {
	NetConn                                      net.Conn

We need comments for all these exported fields.

[davisgu]

Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @cesarghali, @davisgu, and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker_test.go, line 302 at r6 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

Please change the test to make this error message and the one below a bit different.

Done. (ignored)

---

security/s2a/internal/record/record.go, line 59 at r6 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

We need comments for all these exported fields.

Done.

[davisgu]

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @cesarghali, @davisgu, and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 64 at r5 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

This is not a handshaker option and since it needed in the record protocol, maybe we should pass it as a parameter to New*. WDYT?

Same below.

@matthewstevenson88 FYI.

Done.

---

security/s2a/internal/handshaker/handshaker.go, line 313 at r5 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

If we add the handshaker service address to New*, we would store it as part of s2aHandshaker. So there won't be a need for this API.

Done.

[cesarghali]

Reviewed 1 of 1 files at r7, 2 of 2 files at r8.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @davisgu and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 92 at r8 (raw file):

	isClient bool
	// HandshakerServiceAddress stores the address of the S2A handshaker service.
	handshakerServiceAddress string

Please rename to hsAddr.

---

security/s2a/internal/record/record.go, line 66 at r8 (raw file):

	// TlsVersion is the TLS version number that the S2A's handshaker module
	// used to set up the session.
	TlsVersion s2apb.TLSVersion

TLSVersion and everywhere else where Tls is used.

---

security/s2a/internal/record/record.go, line 82 at r8 (raw file):

}

func NewConn(o *ConnOptions) (net.Conn, error) {

Thinking about it more, I don't think we should pass things using an option struct. Using struct makes what's needed and what's optional ambiguous. We should pass all these in arguments. If desired, we can create structs for TrafficSecrets and Sequences.

We can address this in a separate PR.

@matthewstevenson88 @Ryanfsdf FYI.

[davisgu]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @cesarghali, @davisgu, and @matthewstevenson88)

---

security/s2a/internal/handshaker/handshaker.go, line 92 at r8 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

Please rename to hsAddr.

Done.

---

security/s2a/internal/record/record.go, line 66 at r8 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

TLSVersion and everywhere else where Tls is used.

Done.