Handshaker component, part 2: Handshaker implementation #31
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 96 at r1 (raw file):
} // newClientHandshakerInternal is for testing purposes only.
Please remove Internal
security/s2a/internal/handshaker/handshaker.go, line 115 at r1 (raw file):
} // newClientHandshakerInternal is for testing purposes only.
Please remove Internal
security/s2a/internal/handshaker/handshaker.go, line 131 at r1 (raw file):
} // Create target identities from service account list.
Please change this comment to "Prepare a client start message to send to the S2A handshaker service."
security/s2a/internal/handshaker/handshaker.go, line 165 at r1 (raw file):
} p := make([]byte, 64*1024) // temp length?
Please make this length into a constant at the top of the file.
security/s2a/internal/handshaker/handshaker.go, line 170 at r1 (raw file):
return nil, nil, err } req := &s2apb.SessionReq{
Please add the following comment "Prepare a server start message to send to the S2A handshaker service."
security/s2a/internal/handshaker/handshaker.go, line 194 at r1 (raw file):
} func (h *s2aHandshaker) setUpSession(req *s2apb.SessionReq) (net.Conn, *s2apb.SessionResult, error) {
Please add a comment for this method.
security/s2a/internal/handshaker/handshaker.go, line 221 at r1 (raw file):
} func (h *s2aHandshaker) accessHandshakerService(req *s2apb.SessionReq) (*s2apb.SessionResp, error) {
Please add a comment for this method.
security/s2a/internal/handshaker/handshaker.go, line 283 at r1 (raw file):
// the secure connection at the end of the handshake; otherwise it is a no-op. func (h *s2aHandshaker) Close() { h.stream.CloseSend()
What happens if we call CloseSend twice? Is the second call a no-op?
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 96 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please remove
Internal
Done.
security/s2a/internal/handshaker/handshaker.go, line 115 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please remove
Internal
Done.
security/s2a/internal/handshaker/handshaker.go, line 131 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please change this comment to "Prepare a client start message to send to the S2A handshaker service."
Done.
security/s2a/internal/handshaker/handshaker.go, line 165 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please make this length into a constant at the top of the file.
Done.
security/s2a/internal/handshaker/handshaker.go, line 170 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please add the following comment "Prepare a server start message to send to the S2A handshaker service."
Done.
security/s2a/internal/handshaker/handshaker.go, line 194 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment for this method.
Done.
security/s2a/internal/handshaker/handshaker.go, line 221 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment for this method.
Done.
security/s2a/internal/handshaker/handshaker.go, line 283 at r1 (raw file):
Previously, matthewstevenson88 wrote…
What happens if we call
CloseSendtwice? Is the second call a no-op?
yes, it just returns nil without performing any actions.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 96 at r1 (raw file):
Previously, davisgu wrote…
Done.
If it's used for testing purposes only, would it be better to move it to the handshaker_test.go file instead?
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 22 unresolved discussions (waiting on @davisgu and @Ryanfsdf)
security/s2a/internal/handshaker/handshaker.go, line 96 at r1 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
If it's used for testing purposes only, would it be better to move it to the
handshaker_test.gofile instead?
It is also called by the NewClientHandshaker API, so maybe we should remove this comment altogether. WDYT?
security/s2a/internal/handshaker/handshaker.go, line 115 at r1 (raw file):
Previously, davisgu wrote…
Done.
Same as above, should we remove this comment altogether?
security/s2a/internal/handshaker/handshaker.go, line 194 at r1 (raw file):
Previously, davisgu wrote…
Done.
Please change the comment to "proxies messages between the peer and the S2A handshaker service."
security/s2a/internal/handshaker/handshaker.go, line 36 at r3 (raw file):
var ( appProtocols = []string{"grpc"}
I think this should only be 1 application protocol, not a list.
security/s2a/internal/handshaker/handshaker.go, line 207 at r3 (raw file):
if req.GetServerStart() != nil { if resp.GetBytesConsumed() > uint32(len(req.GetServerStart().GetInBytes())) { return nil, nil, errors.New("handshaker service consumed bytes value is out-of-bound")
s/out-of-bound/out-of-bounds
security/s2a/internal/handshaker/handshaker.go, line 215 at r3 (raw file):
return nil, nil, err } // TODO: implement record protocol & new Conn
Please add your username to the TODO.
security/s2a/internal/handshaker/handshaker.go, line 217 at r3 (raw file):
// TODO: implement record protocol & new Conn in := bytes.NewBuffer(MakeFrame("ServerInit"))
Where are these coming from?
security/s2a/internal/handshaker/handshaker.go, line 218 at r3 (raw file):
in := bytes.NewBuffer(MakeFrame("ServerInit")) in.Write(MakeFrame("ServerFinished"))
Where is this coming from?
security/s2a/internal/handshaker/handshaker.go, line 219 at r3 (raw file):
in := bytes.NewBuffer(MakeFrame("ServerInit")) in.Write(MakeFrame("ServerFinished")) c := &fakeConn{
Why is there a fake in the real code? Can we just leave this unimplemented for now?
security/s2a/internal/handshaker/handshaker.go, line 239 at r3 (raw file):
} // processUntilDone processes the handshake until the handshaker service returns
IIUC, extra consists of bytes that were unused from the last interaction with the handshaker service. If so, please rename it to unusedBytes. This will make the terminology consistent across all 3 languages.
security/s2a/internal/handshaker/handshaker.go, line 240 at r3 (raw file):
// processUntilDone processes the handshake until the handshaker service returns // the results.
Please document what extra is in this comment.
security/s2a/internal/handshaker/handshaker.go, line 256 at r3 (raw file):
return nil, nil, err } // If there is nothing to send to the handshaker service, and
Please remove this comma.
security/s2a/internal/handshaker/handshaker.go, line 278 at r3 (raw file):
return nil, nil, err } // Set extra based on handshaker service response.
Please add the between on and handshaker,
security/s2a/internal/handshaker/handshaker.go, line 280 at r3 (raw file):
// Set extra based on handshaker service response. if resp.GetBytesConsumed() > uint32(len(p)) { return nil, nil, errors.New("handshaker service consumed bytes value is out-of-bound")
s/out-of-bound/out-of-bounds
security/s2a/internal/handshaker/handshaker_test.go, line 120 at r3 (raw file):
// MakeFrame creates a handshake frame. func MakeFrame(pl string) []byte {
Why is this exported?
security/s2a/internal/handshaker/handshaker_test.go, line 128 at r3 (raw file):
// TestNewClientHandshaker creates a fake stream, and ensures that // newClientHandshakerInternal returns a valid client-side handshaker instance.
Please remove Internal.
security/s2a/internal/handshaker/handshaker_test.go, line 144 at r3 (raw file):
// TestNewServerHandshaker creates a fake stream, and ensures that // newServerHandshakerInternal returns a valid server-side handshaker instance.
Please remove Internal.
security/s2a/internal/handshaker/handshaker_test.go, line 176 at r3 (raw file):
} go func() { _, context, err := chs.ClientHandshake(context.Background())
If _ is unused because the record protocol cannot yet be created, please document this.
security/s2a/internal/handshaker/handshaker_test.go, line 177 at r3 (raw file):
go func() { _, context, err := chs.ClientHandshake(context.Background()) if err == nil && context == nil {
Shouldn't this test check that context is what we expect it to be?
security/s2a/internal/handshaker/handshaker_test.go, line 202 at r3 (raw file):
} go func() { _, context, err := shs.ServerHandshake(context.Background())
Please document why _ is unused.
security/s2a/internal/handshaker/handshaker_test.go, line 203 at r3 (raw file):
go func() { _, context, err := shs.ServerHandshake(context.Background()) if err == nil && context == nil {
Should we check what context is?
security/s2a/internal/handshaker/handshaker_test.go, line 224 at r3 (raw file):
// TestPeerNotResponding uses an invalid net.Conn instance and performs a // handshake to test PeerNotRespondingError
s/PeerNotRespondingError/the case when the peer is not responding.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 22 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/handshaker/handshaker.go, line 36 at r3 (raw file):
Previously, matthewstevenson88 wrote…
I think this should only be 1 application protocol, not a list.
the proto requires a list of string.
security/s2a/internal/handshaker/handshaker.go, line 207 at r3 (raw file):
Previously, matthewstevenson88 wrote…
s/out-of-bound/out-of-bounds
Done.
security/s2a/internal/handshaker/handshaker.go, line 215 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please add your username to the TODO.
Done.
security/s2a/internal/handshaker/handshaker.go, line 217 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Where are these coming from?
Done. removed. was temporary there for testing
security/s2a/internal/handshaker/handshaker.go, line 218 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Where is this coming from?
Done. removed.
security/s2a/internal/handshaker/handshaker.go, line 219 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Why is there a fake in the real code? Can we just leave this unimplemented for now?
Done. removed
security/s2a/internal/handshaker/handshaker.go, line 239 at r3 (raw file):
Previously, matthewstevenson88 wrote…
IIUC,
extraconsists of bytes that were unused from the last interaction with the handshaker service. If so, please rename it tounusedBytes. This will make the terminology consistent across all 3 languages.
Done.
security/s2a/internal/handshaker/handshaker.go, line 240 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please document what
extrais in this comment.
Done.
security/s2a/internal/handshaker/handshaker.go, line 256 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please remove this comma.
Done.
security/s2a/internal/handshaker/handshaker.go, line 278 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please add
thebetweenonandhandshaker,
Done.
security/s2a/internal/handshaker/handshaker.go, line 280 at r3 (raw file):
Previously, matthewstevenson88 wrote…
s/out-of-bound/out-of-bounds
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 120 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Why is this exported?
Should I create a testutil and move it there?
security/s2a/internal/handshaker/handshaker_test.go, line 128 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please remove
Internal.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 144 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please remove
Internal.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 176 at r3 (raw file):
Previously, matthewstevenson88 wrote…
If
_is unused because the record protocol cannot yet be created, please document this.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 177 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Shouldn't this test check that
contextis what we expect it to be?
context in this case is the authInfo for the handshake. Is there a need to check it?
security/s2a/internal/handshaker/handshaker_test.go, line 202 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please document why
_is unused.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 224 at r3 (raw file):
Previously, matthewstevenson88 wrote…
s/PeerNotRespondingError/the case when the peer is not responding.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 29 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 36 at r3 (raw file):
Previously, davisgu wrote…
the proto requires a list of string.
That's fine, but there is only ever one string that we use, namely "grpc". So we could have a constant that stores the application protocol, and build the list when we populate the proto. WDYT?
security/s2a/internal/handshaker/handshaker.go, line 215 at r3 (raw file):
Previously, davisgu wrote…
Done.
Please format the TODO as follows: TODO(gud): implement when PR #29 is merged.
security/s2a/internal/handshaker/handshaker.go, line 192 at r4 (raw file):
// service func (h *s2aHandshaker) setUpSession(req *s2apb.SessionReq) (net.Conn, *s2apb.SessionResult, error) { resp, err := h.accessHandshakerService(req)
Should we check if req == nil before calling accessHandshakerService?
security/s2a/internal/handshaker/handshaker.go, line 217 at r4 (raw file):
} // accessHandshakerService sends the session request over the Handshaker service
s/over/to
s/the Handshaker/the S2A handshaker
Please remove the word stream.
s/the response/the session response
security/s2a/internal/handshaker/handshaker.go, line 202 at r5 (raw file):
} } var extra []byte
Please add a comment to describe what this block of code is doing.
Also, is there any test case that covers this block of code?
security/s2a/internal/handshaker/handshaker.go, line 207 at r5 (raw file):
return nil, nil, errors.New("handshaker service consumed bytes value is out-of-bounds") } extra = req.GetServerStart().GetInBytes()[resp.GetBytesConsumed():]
Is there any test case that covers this?
security/s2a/internal/handshaker/handshaker.go, line 230 at r5 (raw file):
} // processUntilDone processes the handshake until the handshaker service returns
s/processes the handshake/continues proxying messages between the peer and the S2A handshaker service
s/the results/the SessionResult at the end of the handshake or an error occurs.
security/s2a/internal/handshaker/handshaker.go, line 243 at r5 (raw file):
} buf := make([]byte, frameLimit) n, err := h.conn.Read(buf)
This call to read blocks, correct?
security/s2a/internal/handshaker/handshaker_test.go, line 120 at r3 (raw file):
Previously, davisgu wrote…
Should I create a testutil and move it there?
We could, but if there's nothing else that would go in the testutil file then I am fine leaving it here.
Rather, I meant renaming the function makeFrame.
security/s2a/internal/handshaker/handshaker_test.go, line 92 at r5 (raw file):
grpc.ClientStream t *testing.T ExpectedResp *s2apb.SessionResp
Does ExpectedResp need to be exported?
security/s2a/internal/handshaker/handshaker_test.go, line 93 at r5 (raw file):
t *testing.T ExpectedResp *s2apb.SessionResp first bool
Please give a more descriptive variable name.
security/s2a/internal/handshaker/handshaker_test.go, line 105 at r5 (raw file):
var resp *s2apb.SessionResp if !fs.first { // Generate the bytes to be returned by Recv() for the initial
s/for the initial handshaking/in the first handshake message.
security/s2a/internal/handshaker/handshaker_test.go, line 110 at r5 (raw file):
if fs.isClient { resp = &s2apb.SessionResp{ OutFrames: MakeFrame("ClientInit"),
Please use ClientHello rather than ClientInit.
security/s2a/internal/handshaker/handshaker_test.go, line 111 at r5 (raw file):
resp = &s2apb.SessionResp{ OutFrames: MakeFrame("ClientInit"), // Simulate consuming ServerInit.
s/ServerInit/ServerHello
security/s2a/internal/handshaker/handshaker_test.go, line 112 at r5 (raw file):
OutFrames: MakeFrame("ClientInit"), // Simulate consuming ServerInit. BytesConsumed: 14,
Why 14?
security/s2a/internal/handshaker/handshaker_test.go, line 116 at r5 (raw file):
} else { resp = &s2apb.SessionResp{ OutFrames: MakeFrame("ServerInit"),
s/ServerInit/ServerHello
security/s2a/internal/handshaker/handshaker_test.go, line 117 at r5 (raw file):
resp = &s2apb.SessionResp{ OutFrames: MakeFrame("ServerInit"), // Simulate consuming ClientInit.
s/ClientInit/ClientHello
security/s2a/internal/handshaker/handshaker_test.go, line 118 at r5 (raw file):
OutFrames: MakeFrame("ServerInit"), // Simulate consuming ClientInit. BytesConsumed: 14,
Why 14?
security/s2a/internal/handshaker/handshaker_test.go, line 130 at r5 (raw file):
Result: result, // Simulate consuming ClientFinished or ServerFinished. BytesConsumed: 18,
Why 18?
security/s2a/internal/handshaker/handshaker_test.go, line 138 at r5 (raw file):
func (*fakeStream) CloseSend() error { return nil } //fakeInvalidStream is a fake implementation of an invalid grpc.ClientStream
Please add a space between // and the start of the comment.
security/s2a/internal/handshaker/handshaker_test.go, line 170 at r5 (raw file):
func (fc *fakeInvalidConn) Close() error { return nil } // MakeFrame creates a handshake frame.
I don't think this function does anything related to the handshake, it's a little more general than that. Please alter the comment to reflect that.
security/s2a/internal/handshaker/handshaker_test.go, line 182 at r5 (raw file):
func TestNewClientHandshaker(t *testing.T) { stream := &fakeStream{} in := bytes.NewBuffer(MakeFrame("ClientInit"))
s/ClientInit/ClientHello
security/s2a/internal/handshaker/handshaker_test.go, line 198 at r5 (raw file):
func TestNewServerHandshaker(t *testing.T) { stream := &fakeStream{} in := bytes.NewBuffer(MakeFrame("ServerInit"))
s/ServerInit/ServerHello
security/s2a/internal/handshaker/handshaker_test.go, line 230 at r5 (raw file):
} go func() { // returned conn is ignored until record Protocol is implemented.
s/returned/Returned
s/Protocol/protocol
Please add a TODO for yourself to alter this when #29 is merged.
security/s2a/internal/handshaker/handshaker_test.go, line 233 at r5 (raw file):
_, _, err := chs.ClientHandshake(context.Background()) if err != nil { panic("expected non-nil S2A context")
Should we use t.Errorf rather than panic?
security/s2a/internal/handshaker/handshaker_test.go, line 264 at r5 (raw file):
} go func() { // returned conn is ignored until record Protocol is implemented.
s/returned conn/The conn returned by ServerHandshake
s/Protocol/protocol
security/s2a/internal/handshaker/handshaker_test.go, line 286 at r5 (raw file):
_, _, err = emptyHS.ServerHandshake(context.Background()) if err == nil { t.Error("ServerHandshake() shouldnt' work with empty ServerOptions")
s/shouldnt'/shouldn't
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 29 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 36 at r3 (raw file):
Previously, matthewstevenson88 wrote…
That's fine, but there is only ever one string that we use, namely "grpc". So we could have a constant that stores the application protocol, and build the list when we populate the proto. WDYT?
Done.
security/s2a/internal/handshaker/handshaker.go, line 192 at r4 (raw file):
Previously, matthewstevenson88 wrote…
Should we check if
req == nilbefore callingaccessHandshakerService?
Sure, but setUpSession is only called by Server/ClientHandshake after being initialized. Done.
security/s2a/internal/handshaker/handshaker.go, line 217 at r4 (raw file):
Previously, matthewstevenson88 wrote…
s/over/to
s/the Handshaker/the S2A handshaker
Please remove the word
stream.s/the response/the session response
Done.
security/s2a/internal/handshaker/handshaker.go, line 202 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment to describe what this block of code is doing.
Also, is there any test case that covers this block of code?
Done. most handshake tests will use this block of code.
security/s2a/internal/handshaker/handshaker.go, line 207 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Is there any test case that covers this?
most handshake tests will use this block of code(gud):
security/s2a/internal/handshaker/handshaker.go, line 230 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/processes the handshake/continues proxying messages between the peer and the S2A handshaker service
s/the results/the SessionResult at the end of the handshake or an error occurs.
Done.
security/s2a/internal/handshaker/handshaker.go, line 243 at r5 (raw file):
Previously, matthewstevenson88 wrote…
This call to read blocks, correct?
I believe so.
security/s2a/internal/handshaker/handshaker_test.go, line 120 at r3 (raw file):
Previously, matthewstevenson88 wrote…
We could, but if there's nothing else that would go in the testutil file then I am fine leaving it here.
Rather, I meant renaming the function
makeFrame.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 92 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Does
ExpectedRespneed to be exported?
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 93 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please give a more descriptive variable name.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 105 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/for the initial handshaking/in the first handshake message.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 110 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please use
ClientHellorather thanClientInit.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 111 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/ServerInit/ServerHello
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 116 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/ServerInit/ServerHello
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 117 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/ClientInit/ClientHello
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 138 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add a space between
//and the start of the comment.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 182 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/ClientInit/ClientHello
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 198 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/ServerInit/ServerHello
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 230 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/returned/Returned
s/Protocol/protocol
Please add a TODO for yourself to alter this when #29 is merged.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 264 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/returned conn/The conn returned by ServerHandshake
s/Protocol/protocol
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 286 at r5 (raw file):
Previously, matthewstevenson88 wrote…
s/shouldnt'/shouldn't
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 29 unresolved discussions (waiting on @matthewstevenson88 and @Ryanfsdf)
security/s2a/internal/handshaker/handshaker_test.go, line 203 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Should we check what context is?
Removed. Done.
security/s2a/internal/handshaker/handshaker_test.go, line 112 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Why 14?
fixed. Should be msgLen + tlsRecordHeaderSize(which is @Ryanfsdf's PR). added todo to makeFrame.
security/s2a/internal/handshaker/handshaker_test.go, line 118 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Why 14?
Fixed
security/s2a/internal/handshaker/handshaker_test.go, line 130 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Why 18?
Fixed
security/s2a/internal/handshaker/handshaker_test.go, line 170 at r5 (raw file):
Previously, matthewstevenson88 wrote…
I don't think this function does anything related to the handshake, it's a little more general than that. Please alter the comment to reflect that.
Done
security/s2a/internal/handshaker/handshaker_test.go, line 233 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Should we use
t.Errorfrather thanpanic?
removed entirely, because errors will be handled after the goroutine. Done
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 15 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/handshaker/handshaker.go, line 127 at r6 (raw file):
func (h *s2aHandshaker) ClientHandshake(ctx context.Context) (net.Conn, *authinfo.S2AAuthInfo, error) { if h.clientOpts == nil { return nil, nil, errors.New("Only handshakers created using NewClientHandshaker can perform a client handshaker.")
s/a client handshaker/a client-side handshake
security/s2a/internal/handshaker/handshaker.go, line 158 at r6 (raw file):
func (h *s2aHandshaker) ServerHandshake(ctx context.Context) (net.Conn, *authinfo.S2AAuthInfo, error) { if h.serverOpts == nil { return nil, nil, errors.New("Only handshakers created using NewServerHandshaker can perform a server handshaker.")
s/a server handshaker/a server-side handshake
security/s2a/internal/handshaker/handshaker.go, line 193 at r6 (raw file):
func (h *s2aHandshaker) setUpSession(req *s2apb.SessionReq) (net.Conn, *s2apb.SessionResult, error) { if req == nil { return nil,nil,errors.New("Session Request can not be nil.")
s/Session Request/req
security/s2a/internal/handshaker/handshaker.go, line 218 at r6 (raw file):
return nil, nil, err } // TODO(gud): implement record protocol & new Conn
Please write a more descriptive comment, e.g. use the NewConn API to construct the record protocol when PR #29 is merged.
security/s2a/internal/handshaker/handshaker.go, line 237 at r6 (raw file):
// processUntilDone continues proxying messages between the peer and the S2A // handshaker service until the handshaker service returns the SessionResult // at the end of the handshake or an error occurs.
Please remove the extra space after //.
security/s2a/internal/handshaker/handshaker_test.go, line 177 at r3 (raw file):
Previously, davisgu wrote…
context in this case is the authInfo for the handshake. Is there a need to check it?
Yes, I think we want to ensure that the authInfo is what we expect it to be after the handshake
security/s2a/internal/handshaker/handshaker_test.go, line 112 at r5 (raw file):
Previously, davisgu wrote…
fixed. Should be msgLen + tlsRecordHeaderSize(which is @Ryanfsdf's PR). added todo to makeFrame.
I don't think that's correct. This is the number of bytes from the in_bytes that have been consumed. In particular, if the fake handshaker service has just received a ClientSessionStartReq, then it contains no in_bytes so this should be zero. Please see #33 for the correct values.
Please add a comment that describes where this number comes from.
security/s2a/internal/handshaker/handshaker_test.go, line 118 at r5 (raw file):
Previously, davisgu wrote…
Fixed
Please add a comment describing where 16 comes from.
security/s2a/internal/handshaker/handshaker_test.go, line 130 at r5 (raw file):
Previously, davisgu wrote…
Fixed
Please add a comment describing where 19 comes from.
security/s2a/internal/handshaker/handshaker_test.go, line 119 at r6 (raw file):
resp = &s2apb.SessionResp{ OutFrames: makeFrame("ServerHello"), // Simulate consuming ClientHello.
s/ClientHello/the ClientHello message.
security/s2a/internal/handshaker/handshaker_test.go, line 125 at r6 (raw file):
} else { // Generate the response to be returned by Recv() for the // follow-up handshaking.
Please replace this comment with: Construct a SessionResp message that contains the handshake result.
security/s2a/internal/handshaker/handshaker_test.go, line 127 at r6 (raw file):
// follow-up handshaking. result := &s2apb.SessionResult{ ApplicationProtocol: "grpc",
Please add add all of the fields from SessionResult.
security/s2a/internal/handshaker/handshaker_test.go, line 131 at r6 (raw file):
resp = &s2apb.SessionResp{ Result: result, // Simulate consuming ClientFinished or ServerFinished.
Please add the between consuming and ClientFinished.
Please add message at the end of the sentence.
security/s2a/internal/handshaker/handshaker_test.go, line 173 at r6 (raw file):
// makeFrame creates a frame. func makeFrame(pl string) []byte {
Now that I read the tests more carefully, do we need the makeFrame API at all? It looks like it could be removed.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 15 unresolved discussions (waiting on @matthewstevenson88 and @Ryanfsdf)
security/s2a/internal/handshaker/handshaker.go, line 127 at r6 (raw file):
Previously, matthewstevenson88 wrote…
s/a client handshaker/a client-side handshake
Done.
security/s2a/internal/handshaker/handshaker.go, line 158 at r6 (raw file):
Previously, matthewstevenson88 wrote…
s/a server handshaker/a server-side handshake
Done.
security/s2a/internal/handshaker/handshaker.go, line 193 at r6 (raw file):
Previously, matthewstevenson88 wrote…
s/Session Request/req
Done.
security/s2a/internal/handshaker/handshaker.go, line 218 at r6 (raw file):
Previously, matthewstevenson88 wrote…
Please write a more descriptive comment, e.g.
use the NewConn API to construct the record protocol when PR #29 is merged.
Done.
security/s2a/internal/handshaker/handshaker.go, line 237 at r6 (raw file):
Previously, matthewstevenson88 wrote…
Please remove the extra space after
//.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 177 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Yes, I think we want to ensure that the
authInfois what we expect it to be after the handshake
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 112 at r5 (raw file):
Previously, matthewstevenson88 wrote…
I don't think that's correct. This is the number of bytes from the
in_bytesthat have been consumed. In particular, if the fake handshaker service has just received aClientSessionStartReq, then it contains noin_bytesso this should be zero. Please see #33 for the correct values.Please add a comment that describes where this number comes from.
replaced
security/s2a/internal/handshaker/handshaker_test.go, line 118 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment describing where 16 comes from.
replaced
security/s2a/internal/handshaker/handshaker_test.go, line 130 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment describing where 19 comes from.
replaced
security/s2a/internal/handshaker/handshaker_test.go, line 119 at r6 (raw file):
Previously, matthewstevenson88 wrote…
s/ClientHello/the ClientHello message.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 125 at r6 (raw file):
Previously, matthewstevenson88 wrote…
Please replace this comment with:
Construct a SessionResp message that contains the handshake result.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 127 at r6 (raw file):
Previously, matthewstevenson88 wrote…
Please add add all of the fields from
SessionResult.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 131 at r6 (raw file):
Previously, matthewstevenson88 wrote…
Please add
thebetweenconsumingandClientFinished.Please add
messageat the end of the sentence.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 173 at r6 (raw file):
Previously, matthewstevenson88 wrote…
Now that I read the tests more carefully, do we need the
makeFrameAPI at all? It looks like it could be removed.
Used it to create the byte buffers for fakeConn.
remove?
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 15 unresolved discussions (waiting on @matthewstevenson88 and @Ryanfsdf)
security/s2a/internal/handshaker/handshaker_test.go, line 173 at r6 (raw file):
Previously, davisgu wrote…
Used it to create the byte buffers for fakeConn.
remove?
Edit: Removed. Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 15 unresolved discussions (waiting on @davisgu)
security/s2a/internal/handshaker/handshaker.go, line 37 at r7 (raw file):
appProtocols = "grpc" frameLimit = 1024 * 64 PeerNotRespondingError = errors.New("Peer server is not responding and re-connection should be attempted.")
Please remove server. The peer does not need to be a server, it could be a client.
security/s2a/internal/handshaker/handshaker.go, line 193 at r7 (raw file):
func (h *s2aHandshaker) setUpSession(req *s2apb.SessionReq) (net.Conn, *s2apb.SessionResult, error) { if req == nil { return nil, nil, errors.New("req can not be nil.")
s/can not/cannot
security/s2a/internal/handshaker/handshaker_test.go, line 34 at r7 (raw file):
var ( // testClientHandshakerOptions are the client handshaker options used for testing
s/client/client-side
Please add a period at the end of the sentence.
security/s2a/internal/handshaker/handshaker_test.go, line 63 at r7 (raw file):
} // testServerHandshakerOptions are the server handshaker options used for testing
s/server/server-side
Please add a period at the end of the sentence.
security/s2a/internal/handshaker/handshaker_test.go, line 90 at r7 (raw file):
State: &s2apb.SessionState{ TlsVersion: s2apb.TLSVersion_TLS1_3, TlsCiphersuite: s2apb.Ciphersuite_AES_128_GCM_SHA256,
Please also set the in/out sequence numbers to zero and set the in/out keys to something.
security/s2a/internal/handshaker/handshaker_test.go, line 101 at r7 (raw file):
SpiffeId: "server_local_spiffe_id", }, },
Please also set the peer_cert_fingerprint to "server_cert_fingerprint", and the local_cert_fingerprint to "client_cert_fingerprint".
security/s2a/internal/handshaker/handshaker_test.go, line 108 at r7 (raw file):
State: &s2apb.SessionState{ TlsVersion: s2apb.TLSVersion_TLS1_3, TlsCiphersuite: s2apb.Ciphersuite_AES_128_GCM_SHA256,
Same as above, please set the in/out sequence numbers and in/out keys.
security/s2a/internal/handshaker/handshaker_test.go, line 119 at r7 (raw file):
SpiffeId: "client_local_spiffe_id", }, },
Please also set the peer_cert_fingerprint to "client_cert_fingerprint", and the local_cert_fingerprint to "server_cert_fingerprint".
security/s2a/internal/handshaker/handshaker_test.go, line 128 at r7 (raw file):
grpc.ClientStream t *testing.T // expectedResp is the expected Session Response from the handshaker service.
s/Session Response/SessionResp message
security/s2a/internal/handshaker/handshaker_test.go, line 130 at r7 (raw file):
// expectedResp is the expected Session Response from the handshaker service. expectedResp *s2apb.SessionResp // isFirstAccess determines if it is the first access to the Handshaker Service.
Please replace this comment with:
isFirstAccess indicates whether the first call to the handshaker service has been made or not.
security/s2a/internal/handshaker/handshaker_test.go, line 148 at r7 (raw file):
resp = &s2apb.SessionResp{ OutFrames: []byte("ClientHello"), // Simulate consuming the ServerHello message.
Replace this comment with: There are no consumed bytes for a client start message.
security/s2a/internal/handshaker/handshaker_test.go, line 149 at r7 (raw file):
OutFrames: []byte("ClientHello"), // Simulate consuming the ServerHello message. BytesConsumed: uint32(len("ServerHello")),
Replace this with zero.
security/s2a/internal/handshaker/handshaker_test.go, line 230 at r7 (raw file):
// TestNewServerHandshaker creates a fake stream, and ensures that // newServerHandshaker returns a valid server-side handshaker instance.
Is there an extra space between newServerHandshaker and returns?
security/s2a/internal/handshaker/handshaker_test.go, line 271 at r7 (raw file):
errc <- err if auth.ApplicationProtocol() != result.GetApplicationProtocol() || auth.TLSVersion() != result.GetState().GetTlsVersion() ||
When they are added, please also check the peer/local cert fingerprint.
security/s2a/internal/handshaker/handshaker_test.go, line 313 at r7 (raw file):
if auth.ApplicationProtocol() != result.GetApplicationProtocol() || auth.TLSVersion() != result.GetState().GetTlsVersion() || auth.Ciphersuite() != result.GetState().GetTlsCiphersuite() ||
When they are added, please also test the peer/local cert fingerprint.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 15 unresolved discussions (waiting on @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 37 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Please remove
server. The peer does not need to be a server, it could be a client.
Done.
security/s2a/internal/handshaker/handshaker.go, line 193 at r7 (raw file):
Previously, matthewstevenson88 wrote…
s/can not/cannot
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 34 at r7 (raw file):
Previously, matthewstevenson88 wrote…
s/client/client-side
Please add a period at the end of the sentence.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 63 at r7 (raw file):
Previously, matthewstevenson88 wrote…
s/server/server-side
Please add a period at the end of the sentence.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 90 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Please also set the in/out sequence numbers to zero and set the in/out keys to something.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 101 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Please also set the
peer_cert_fingerprintto "server_cert_fingerprint", and thelocal_cert_fingerprintto "client_cert_fingerprint".
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 108 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Same as above, please set the in/out sequence numbers and in/out keys.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 119 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Please also set the
peer_cert_fingerprintto "client_cert_fingerprint", and thelocal_cert_fingerprintto "server_cert_fingerprint".
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 128 at r7 (raw file):
Previously, matthewstevenson88 wrote…
s/Session Response/SessionResp message
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 130 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Please replace this comment with:
isFirstAccess indicates whether the first call to the handshaker service has been made or not.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 148 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Replace this comment with:
There are no consumed bytes for a client start message.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 149 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Replace this with zero.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 230 at r7 (raw file):
Previously, matthewstevenson88 wrote…
Is there an extra space between
newServerHandshakerandreturns?
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 271 at r7 (raw file):
Previously, matthewstevenson88 wrote…
When they are added, please also check the peer/local cert fingerprint.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 313 at r7 (raw file):
Previously, matthewstevenson88 wrote…
When they are added, please also test the peer/local cert fingerprint.
Done.
There was a problem hiding this comment.
Thanks for the PR Davis! Approved. Please add Cesar as a reviewer for Go readability.
Reviewable status: 0 of 2 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 2 of 2 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @davisgu)
security/s2a/internal/handshaker/handshaker.go, line 28 at r8 (raw file):
"net" grpc "google.golang.org/grpc"
No need to rename this for two reasons: it's renamed to the same name and there's no conflict of imported package names.
security/s2a/internal/handshaker/handshaker.go, line 37 at r8 (raw file):
appProtocols = "grpc" frameLimit = 1024 * 64 PeerNotRespondingError = errors.New("Peer is not responding and re-connection should be attempted.")
Please add a comment for all unexported members. Also, does this need to be unexported?
security/s2a/internal/handshaker/handshaker.go, line 124 at r8 (raw file):
// ClientHandshake performs a client-side TLS handshake using the S2A handshaker // service. When complete, returns a secure TLS connection.
Remove "secure". TLS connections are by definition secure :)
security/s2a/internal/handshaker/handshaker.go, line 125 at r8 (raw file):
// ClientHandshake performs a client-side TLS handshake using the S2A handshaker // service. When complete, returns a secure TLS connection. func (h *s2aHandshaker) ClientHandshake(ctx context.Context) (net.Conn, *authinfo.S2AAuthInfo, error) {
Are we planning to enforce an upper limit on the number of concurrent handshakes that an application can make?
https://github.com/grpc/grpc-go/blob/master/credentials/alts/internal/handshaker/handshaker.go#L192
security/s2a/internal/handshaker/handshaker.go, line 126 at r8 (raw file):
// service. When complete, returns a secure TLS connection. func (h *s2aHandshaker) ClientHandshake(ctx context.Context) (net.Conn, *authinfo.S2AAuthInfo, error) { if h.clientOpts == nil {
Actually, you were right at first, it might be good to have a way to differentiate the side:
https://github.com/grpc/grpc-go/blob/master/credentials/alts/internal/handshaker/handshaker.go#L197
security/s2a/internal/handshaker/handshaker.go, line 192 at r8 (raw file):
// service. func (h *s2aHandshaker) setUpSession(req *s2apb.SessionReq) (net.Conn, *s2apb.SessionResult, error) { if req == nil {
We don't need to check this. setUpSession is a private function and we control how we call it. We should not call it with a nil req.
security/s2a/internal/handshaker/handshaker.go, line 223 at r8 (raw file):
} // accessHandshakerService sends the session request to the S2A Handshaker service
This might not be 80 chars.
security/s2a/internal/handshaker/handshaker.go, line 237 at r8 (raw file):
// processUntilDone continues proxying messages between the peer and the S2A // handshaker service until the handshaker service returns the SessionResult
The at from the next line can fit at the end of this line :)
security/s2a/internal/handshaker/handshaker_test.go, line 34 at r8 (raw file):
var ( // testClientHandshakerOptions are the client-side handshaker options used for testing.
This doesn't seem to be 80 chars.
security/s2a/internal/handshaker/handshaker_test.go, line 63 at r8 (raw file):
} // testServerHandshakerOptions are the server-side handshaker options used for testing.
Same here.
security/s2a/internal/handshaker/handshaker_test.go, line 140 at r8 (raw file):
grpc.ClientStream t *testing.T // expectedResp is the expected SessionResp message from the handshaker service.
Not 80 chars?
security/s2a/internal/handshaker/handshaker_test.go, line 142 at r8 (raw file):
// expectedResp is the expected SessionResp message from the handshaker service. expectedResp *s2apb.SessionResp // isFirstAccess indicates whether the first call to the handshaker service has
Not 80 chars?
security/s2a/internal/handshaker/handshaker_test.go, line 156 at r8 (raw file):
var resp *s2apb.SessionResp if !fs.isFirstAccess { // Generate the bytes to be returned by Recv() for the first handshake message.
Not 80 chars?
security/s2a/internal/handshaker/handshaker_test.go, line 216 at r8 (raw file):
func (fc *fakeConn) Close() error { return nil } // fakeInvalidConn is a fake implementation of a invalid net.Conn interface that is
Not 80 chars?
security/s2a/internal/handshaker/handshaker_test.go, line 261 at r8 (raw file):
// handshake. func TestClientHandshake(t *testing.T) { errc := make(chan error)
Can we use errgroup?
https://godoc.org/golang.org/x/sync/errgroup
security/s2a/internal/handshaker/handshaker_test.go, line 303 at r8 (raw file):
// handshake. func TestServerHandshake(t *testing.T) { errc := make(chan error)
Same here.
security/s2a/internal/handshaker/handshaker_test.go, line 347 at r8 (raw file):
_, _, err := emptyHS.ClientHandshake(context.Background()) if err == nil { t.Error("ClientHandshake() shouldn't work with empty ClientOptions")
s/shouldn't work/should fail
security/s2a/internal/handshaker/handshaker_test.go, line 351 at r8 (raw file):
_, _, err = emptyHS.ServerHandshake(context.Background()) if err == nil { t.Error("ServerHandshake() shouldn't work with empty ServerOptions")
should fail
security/s2a/internal/handshaker/handshaker_test.go, line 357 at r8 (raw file):
// TestPeerNotResponding uses an invalid net.Conn instance and performs a // client-side handshake to test the case when the peer is not responding. func TestClientPeerNotResponding(t *testing.T) {
Should we do the same as a server?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @cesarghali and @davisgu)
security/s2a/internal/handshaker/handshaker.go, line 125 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Are we planning to enforce an upper limit on the number of concurrent handshakes that an application can make?
https://github.com/grpc/grpc-go/blob/master/credentials/alts/internal/handshaker/handshaker.go#L192
This will be handled by the handshaker service, and not the application. The same is true in C++ and Java.
(IIUC this will also be the case for ALTS, but due to the rollout period for the necessary changes to the ALTS handshaker service, this code is still in gRPC. This was the case for C++, and I'm guessing it's the same for Go.)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/handshaker/handshaker.go, line 28 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
No need to rename this for two reasons: it's renamed to the same name and there's no conflict of imported package names.
Done.
security/s2a/internal/handshaker/handshaker.go, line 37 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Please add a comment for all unexported members. Also, does this need to be unexported?
Done.
security/s2a/internal/handshaker/handshaker.go, line 124 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Remove "secure". TLS connections are by definition secure :)
Done.
security/s2a/internal/handshaker/handshaker.go, line 126 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Actually, you were right at first, it might be good to have a way to differentiate the side:
https://github.com/grpc/grpc-go/blob/master/credentials/alts/internal/handshaker/handshaker.go#L197
Done.
security/s2a/internal/handshaker/handshaker.go, line 192 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
We don't need to check this.
setUpSessionis a private function and we control how we call it. We should not call it with a nilreq.
Done.
security/s2a/internal/handshaker/handshaker.go, line 223 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This might not be 80 chars.
79! :)
security/s2a/internal/handshaker/handshaker.go, line 237 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
The
atfrom the next line can fit at the end of this line :)
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 34 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This doesn't seem to be 80 chars.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 63 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Same here.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 140 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Not 80 chars?
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 142 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Not 80 chars?
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 156 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Not 80 chars?
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 216 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Not 80 chars?
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 261 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Can we use
errgroup?
https://godoc.org/golang.org/x/sync/errgroup
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 303 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Same here.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 347 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
s/shouldn't work/should fail
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 351 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
should fail
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 357 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Should we do the same as a server?
fakeconn throws EOF instead of peernotresponding when server side. -- followed ALTS' tests.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r10.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/handshaker/handshaker.go, line 125 at r8 (raw file):
Previously, matthewstevenson88 wrote…
This will be handled by the handshaker service, and not the application. The same is true in C++ and Java.
(IIUC this will also be the case for ALTS, but due to the rollout period for the necessary changes to the ALTS handshaker service, this code is still in gRPC. This was the case for C++, and I'm guessing it's the same for Go.)
Ah yes I remember. Thanks for the info.
security/s2a/internal/handshaker/handshaker.go, line 126 at r8 (raw file):
Previously, davisgu wrote…
Done.
Do you think we might need to do like ALTS and add consts for ClientSide and ServerSide?
We don't need to do that in this PR, we can add it when we see the need but I was just curious.
security/s2a/internal/handshaker/handshaker_test.go, line 357 at r8 (raw file):
Previously, davisgu wrote…
fakeconn throws EOF instead of peernotresponding when server side. -- followed ALTS' tests.
SG.
security/s2a/internal/handshaker/handshaker_test.go, line 306 at r10 (raw file):
if err := errg.Wait(); err != nil { t.Errorf("Handshake returned error: %v", err)
Maybe say Client handshake failed:?
Sometimes people seeing an error would copy it and search for it in the code base. Handshake returned error will be found in two places.
Same below.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/handshaker/handshaker.go, line 125 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Ah yes I remember. Thanks for the info.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 357 at r8 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
SG.
Done.
security/s2a/internal/handshaker/handshaker_test.go, line 306 at r10 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Maybe say
Client handshake failed:?Sometimes people seeing an error would copy it and search for it in the code base.
Handshake returned errorwill be found in two places.Same below.
Done.
This change is