Add support for sending session tickets to S2A

[Ryanfsdf]

This PR is a followup to #67 .

Added support for sending session tickets to S2A once we reach the limit on the number of session tickets or stop receiving session tickets.

---

This change is 

[matthewstevenson88]

Thanks for the PR Ryan! I've left a few comments.

One question I have: do we ever close the connection to the S2A? In the handshake component or here? If I'm understanding correctly, the answer is no. And I can't find it ever being closed in the ALTS code as well. Something to discuss...

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

---

security/s2a/internal/handshaker/handshaker.go, line 242 at r1 (raw file):

		OutSequence:      result.GetState().GetOutSequence(),
		HSAddr:           h.hsAddr,
		ConnectionID:     result.GetState().GetConnectionId(),

Please also modify the unit tests to check for the connection id and local identity.

---

security/s2a/internal/record/record.go, line 741 at r1 (raw file):

}

func (t *ticketSender) sendTicketsToS2A(sessionTickets [][]byte) error {

For testing this API, I would recommend a slightly different strategy:

1. Put all of the logic from sendTicketsToS2A besides creating the stream into a new function writeTicketsToStream that creates the ResumptionTicketReq and writes it to the stream.

2. In the test, create a fake stream (similar to what is done in handshaker_test.go, see the link below) and use it to test the writeTicketsToStream method.

grpc-go/security/s2a/internal/handshaker/handshaker_test.go

Line 143 in fd854f8

type fakeStream struct {

WDYT?

---

security/s2a/internal/record/record.go, line 747 at r1 (raw file):

	}
	client := s2apb.NewS2AServiceClient(hsConn)
	session, err := client.SetUpSession(context.Background())

Full disclaimer: I don't know the intricacies of Go, so we should get confirmation on Cesar.

If I'm understanding this code correctly, then isn't this call to the S2A happening in the same thread as the record protocol's Read operation?

I think it should be done asynchronously. Or is this what context.Background() is supposed to be doing?

[Ryanfsdf]

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

---

security/s2a/internal/handshaker/handshaker.go, line 242 at r1 (raw file):

Previously, matthewstevenson88 wrote…

Please also modify the unit tests to check for the connection id and local identity.

Much of these values are currently not checked in the unit tests either. I think the difficulty here is that we can't peek inside the newConn object after we return it because the conn struct is in a separate package. I'm not sure how to check this in the handshaker package.

---

security/s2a/internal/record/record.go, line 741 at r1 (raw file):

Previously, matthewstevenson88 wrote…

For testing this API, I would recommend a slightly different strategy:

1. Put all of the logic from sendTicketsToS2A besides creating the stream into a new function writeTicketsToStream that creates the ResumptionTicketReq and writes it to the stream.

2. In the test, create a fake stream (similar to what is done in handshaker_test.go, see the link below) and use it to test the writeTicketsToStream method.

grpc-go/security/s2a/internal/handshaker/handshaker_test.go

Line 143 in fd854f8

type fakeStream struct {

WDYT?

I think it would end up being a bit more complicated than just creating a fake stream, for a couple of reasons:

1. service.Dial tries to connect to the handshaker service. We can't replace the Dialer to be a dummy dialer like in handshaker_test.go because the dialer is unexported outside of its package. We can try creating a dummy Dial function, but that results in problem 2.
2. s2apb.NewS2AServiceClient expects an actual hsConn object rather than an empty one. It gets a nil pointer deference error when it is passed an empty hsConn (like what is returned in the handshaker_test.go file. So we need to create a fake for this as well.

Because of the reasons above, we would need to create a fake Dial function, a fake NewS2AServiceClient function, and a fake SetUpSession function, as well as fake Send and Recv functions.

Instead of faking out every part of the sendTicketsToS2A function, I feel that the current setup is much simpler and tests that the tickets are at least being triggered to be sent to S2A. The Hexatests may be a better place to actually verify that the session tickets are being received correctly when it actually communicates with a real S2A object.

WDYT?

[matthewstevenson88]

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

---

security/s2a/internal/handshaker/handshaker.go, line 242 at r1 (raw file):

Previously, Ryanfsdf (Ryan Kim) wrote…

Much of these values are currently not checked in the unit tests either. I think the difficulty here is that we can't peek inside the newConn object after we return it because the conn struct is in a separate package. I'm not sure how to check this in the handshaker package.

Ah great point, let's leave it then.

---

security/s2a/internal/record/record.go, line 741 at r1 (raw file):

Previously, Ryanfsdf (Ryan Kim) wrote…

I think it would end up being a bit more complicated than just creating a fake stream, for a couple of reasons:

1. service.Dial tries to connect to the handshaker service. We can't replace the Dialer to be a dummy dialer like in handshaker_test.go because the dialer is unexported outside of its package. We can try creating a dummy Dial function, but that results in problem 2.
2. s2apb.NewS2AServiceClient expects an actual hsConn object rather than an empty one. It gets a nil pointer deference error when it is passed an empty hsConn (like what is returned in the handshaker_test.go file. So we need to create a fake for this as well.

Because of the reasons above, we would need to create a fake Dial function, a fake NewS2AServiceClient function, and a fake SetUpSession function, as well as fake Send and Recv functions.

Instead of faking out every part of the sendTicketsToS2A function, I feel that the current setup is much simpler and tests that the tickets are at least being triggered to be sent to S2A. The Hexatests may be a better place to actually verify that the session tickets are being received correctly when it actually communicates with a real S2A object.

WDYT?

That makes sense, thanks for the explanation Ryan! Let's leave it as is then. :)

[Ryanfsdf]

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

---

security/s2a/internal/record/record.go, line 741 at r1 (raw file):

Previously, matthewstevenson88 wrote…

That makes sense, thanks for the explanation Ryan! Let's leave it as is then. :)

Actually, I think I misunderstood your initial comment. I've added what you suggested :).

---

security/s2a/internal/record/record.go, line 747 at r1 (raw file):

Previously, matthewstevenson88 wrote…

Full disclaimer: I don't know the intricacies of Go, so we should get confirmation on Cesar.

If I'm understanding this code correctly, then isn't this call to the S2A happening in the same thread as the record protocol's Read operation?

I think it should be done asynchronously. Or is this what context.Background() is supposed to be doing?

I've changed the code to run asynchronously.

[matthewstevenson88]

Approved, modulo our discussion about extracting things to a new file.

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

---

security/s2a/internal/record/record.go, line 756 at r2 (raw file):

			}
			client := s2apb.NewS2AServiceClient(hsConn)
			ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)

Can we make 5 into a constant at the top of the file?

[Ryanfsdf]

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

---

security/s2a/internal/record/record.go, line 756 at r2 (raw file):

Previously, matthewstevenson88 wrote…

Can we make 5 into a constant at the top of the file?

Done.

[cesarghali]

Reviewed 1 of 3 files at r1, 4 of 4 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matthewstevenson88 and @Ryanfsdf)

---

security/s2a/internal/record/ticketsender.go, line 46 at r3 (raw file):

// occurs.
func (t *ticketSender) sendTicketsToS2A(sessionTickets [][]byte) {
	go func() {

I think it might be a bit clearer if we move this go routine to the caller. sendTicketToS2A will be called in a go routine.

---

security/s2a/internal/record/ticketsender.go, line 91 at r3 (raw file):

	}
	if sessionResp.GetStatus().GetCode() != uint32(codes.OK) {
		return errors.New("s2a session ticket response was not OK")

A couple of nits:

1. Maybe change the message to something like: failed to send resumption tickets to S2A: <status message here>.
2. Maybe add the local identity string somewhere in the message?

Rational for #2. If the client is using multiple identities and we see this message in the log, we can't tell where it came from.

[Ryanfsdf]

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

---

security/s2a/internal/record/ticketsender.go, line 46 at r3 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

I think it might be a bit clearer if we move this go routine to the caller. sendTicketToS2A will be called in a go routine.

Done, as discussed.

---

security/s2a/internal/record/ticketsender.go, line 91 at r3 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

A couple of nits:

1. Maybe change the message to something like: failed to send resumption tickets to S2A: <status message here>.
2. Maybe add the local identity string somewhere in the message?

Rational for #2. If the client is using multiple identities and we see this message in the log, we can't tell where it came from.

Done. I've added the identity info to all the errors that are thrown by the ticket sender.

[cesarghali]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matthewstevenson88)