Implement record write #49
Conversation
There was a problem hiding this comment.
Thanks for the PR Davis! I've left some comments.
Reviewable status: 0 of 2 files reviewed, 20 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/record/record.go, line 45 at r1 (raw file):
// consist of several records, the last of which could be incomplete. unusedBuf []byte // outRecordsBuf is a buffer used to contain outgoing TLS records before
s/contain/store
security/s2a/internal/record/record.go, line 132 at r1 (raw file):
// TODO: Implement this. n = len(b) numOfFrames := int(math.Ceil(float64(len(b))) / float64(tlsRecordMaxPlaintextSize))
s/numOfFrames/numOfRecords
security/s2a/internal/record/record.go, line 133 at r1 (raw file):
n = len(b) numOfFrames := int(math.Ceil(float64(len(b))) / float64(tlsRecordMaxPlaintextSize)) totalSize := len(b) + numOfFrames*tlsRecordHeaderSize
Each record has more overhead than just the header - it will have the header, plus the record type byte, plus the tag. So there will always be (for our ciphersuites) 17 bytes of overhead, not 5.
security/s2a/internal/record/record.go, line 133 at r1 (raw file):
n = len(b) numOfFrames := int(math.Ceil(float64(len(b))) / float64(tlsRecordMaxPlaintextSize)) totalSize := len(b) + numOfFrames*tlsRecordHeaderSize
Please use a more descriptive name than totalSize. For example, totalNumOfRecordBytes.
security/s2a/internal/record/record.go, line 154 at r1 (raw file):
appData = appData[payloadLen:] payload := append(appData, byte(23))
Please define constants for 23 and 3. (You can reuse the constants from Ryan's PR.)
Even better, these constants should be in hex form (e.g. 0x17 rather than 23).
security/s2a/internal/record/record.go, line 154 at r1 (raw file):
appData = appData[payloadLen:] payload := append(appData, byte(23))
Please make a new function writeTlsRecord that takes 2 parameters: b and byte recordType. Then, your Write function will just be 1 line: write(b, 23).
This flexibility is needed so that the record protocol can encrypt e.g. a key update message.
security/s2a/internal/record/record.go, line 174 at r3 (raw file):
for len(partialB) > 0 { // Construct the payload consisting of app data and record type. payloadLen := len(partialB)
I don't think this is the payload length? This is just the length of the application data in the payload, and it ignores the record type byte and the tag.
security/s2a/internal/record/record.go, line 191 at r3 (raw file):
outRecordsBufIndex += payloadLen + len(buff) } nn, err := p.Conn.Write(p.outRecordsBuf[:outRecordsBufIndex])
Why nn?
security/s2a/internal/record/record.go, line 191 at r3 (raw file):
outRecordsBufIndex += payloadLen + len(buff) } nn, err := p.Conn.Write(p.outRecordsBuf[:outRecordsBufIndex])
Do we need to reset outRecordsBuf in any way?
security/s2a/internal/record/record.go, line 200 at r3 (raw file):
func (p *conn) encryptPayload (b, header []byte) ([]byte, error) {
Should there be the space between encryptPayload and (b?
security/s2a/internal/record/record.go, line 200 at r3 (raw file):
func (p *conn) encryptPayload (b, header []byte) ([]byte, error) {
Please add a comment for this function.
security/s2a/internal/record/record.go, line 208 at r3 (raw file):
} func buildHeader(b []byte) ([]byte){
Please add a comment for this function.
Also, please add a name for the output, e.g. header.
security/s2a/internal/record/record.go, line 208 at r3 (raw file):
} func buildHeader(b []byte) ([]byte){
Please add a check to ensure that len(b) is not too large.
security/s2a/internal/record/record.go, line 210 at r3 (raw file):
func buildHeader(b []byte) ([]byte){ payloadLen := make([]byte, 2) binary.BigEndian.PutUint16(payloadLen, uint16(len(b) + 17))
Why 17? Please make this a constant. (And/or re-use the constants from Ryan's PR.)
security/s2a/internal/record/record.go, line 211 at r3 (raw file):
payloadLen := make([]byte, 2) binary.BigEndian.PutUint16(payloadLen, uint16(len(b) + 17)) return append([]byte{23, 3, 3}, payloadLen...)
Why is there ... after payloadLen?
security/s2a/internal/record/record_test.go, line 189 at r3 (raw file):
} func TestBuildHeader(t *testing.T) {
Please test some additional edge cases, e.g. when len(b) is too large or len(b) = 0.
security/s2a/internal/record/record_test.go, line 219 at r3 (raw file):
desc: "AES-128-GCM-SHA256 with no padding", ciphersuite: s2apb.Ciphersuite_AES_128_GCM_SHA256, inTrafficSecret: testutil.Dehex("6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b"),
Shouldn't this be outTrafficSecret?
security/s2a/internal/record/record_test.go, line 220 at r3 (raw file):
ciphersuite: s2apb.Ciphersuite_AES_128_GCM_SHA256, inTrafficSecret: testutil.Dehex("6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b"), completedRecords: [][]byte{
Please call these records rather than completedRecords.
security/s2a/internal/record/record_test.go, line 523 at r3 (raw file):
TLSVersion: s2apb.TLSVersion_TLS1_3, InTrafficSecret: tc.inTrafficSecret, OutTrafficSecret: tc.inTrafficSecret,
Why are we setting the in traffic secret equal to the out traffic secret? Can we call tc.inTrafficSecret simply trafficSecret?
security/s2a/internal/record/record_test.go, line 533 at r3 (raw file):
t.Errorf("c.Write(plaintext) = (err=nil) = %v, want %v", got, want) } if n!= len(inPlaintext) {
Please check that whatever was written is equal to the correct TLS record (i.e. we need to use the completedRecords part of the test vector).
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 20 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/record/record.go, line 45 at r1 (raw file):
Previously, matthewstevenson88 wrote…
s/contain/store
Done.
security/s2a/internal/record/record.go, line 132 at r1 (raw file):
Previously, matthewstevenson88 wrote…
s/numOfFrames/numOfRecords
Done.
security/s2a/internal/record/record.go, line 133 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Each record has more overhead than just the header - it will have the header, plus the record type byte, plus the tag. So there will always be (for our ciphersuites) 17 bytes of overhead, not 5.
Done.
security/s2a/internal/record/record.go, line 133 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please use a more descriptive name than
totalSize. For example,totalNumOfRecordBytes.
Done.
security/s2a/internal/record/record.go, line 154 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please make a new function
writeTlsRecordthat takes 2 parameters:bandbyte recordType. Then, yourWritefunction will just be 1 line:write(b, 23).This flexibility is needed so that the record protocol can encrypt e.g. a key update message.
Done.
security/s2a/internal/record/record.go, line 154 at r1 (raw file):
Previously, matthewstevenson88 wrote…
Please define constants for
23and3. (You can reuse the constants from Ryan's PR.)Even better, these constants should be in hex form (e.g.
0x17rather than23).
Done.
security/s2a/internal/record/record.go, line 191 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Why
nn?
Done.
security/s2a/internal/record/record.go, line 200 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Should there be the space between
encryptPayloadand(b?
Done.
security/s2a/internal/record/record.go, line 200 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment for this function.
Done.
security/s2a/internal/record/record.go, line 208 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment for this function.
Also, please add a name for the output, e.g.
header.
Done.
security/s2a/internal/record/record.go, line 211 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Why is there
...afterpayloadLen?
appending array to array requires '...' right?
security/s2a/internal/record/record_test.go, line 189 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please test some additional edge cases, e.g. when
len(b)is too large orlen(b) = 0.
Done.
security/s2a/internal/record/record_test.go, line 219 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Shouldn't this be
outTrafficSecret?
Done.
security/s2a/internal/record/record_test.go, line 220 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please call these
recordsrather thancompletedRecords.
Done.
There was a problem hiding this comment.
I took a quick look through the PR since our work for this overlaps a lot, and I left some comments!
Reviewable status: 0 of 2 files reviewed, 26 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/record/record.go, line 133 at r1 (raw file):
Previously, davisgu wrote…
Done.
Could we define a constant rather than using a raw value?
security/s2a/internal/record/record.go, line 154 at r1 (raw file):
Previously, davisgu wrote…
Done.
It was kept as 23 rather than 0x17 as per Cesar's suggestion. Because it is referred to as 23 in the RFC rather than 0x17. C++ also defines the constant as 23 rather than 0x17.
security/s2a/internal/record/record.go, line 66 at r4 (raw file):
// tlsApplicationData is the application data type of the TLS 1.3 record // header. tlsApplicationData = 23
Can we undo this change? It was intentionally changed to 23 rather than 0x17.
security/s2a/internal/record/record.go, line 76 at r4 (raw file):
// tlsHandshakeNewSessionTicket is the prefix of a handshake new session // ticket message of TLS 1.3. tlsHandshakeNewSessionTicket = 4
Same here.
security/s2a/internal/record/record.go, line 79 at r4 (raw file):
// tlsHandshakeKeyUpdatePrefix is the prefix of a handshake key update // message of TLS 1.3. tlsHandshakeKeyUpdatePrefix = 24
Same here too.
security/s2a/internal/record/record.go, line 314 at r4 (raw file):
} func (p *conn) writeTlsRecord(b []byte, recordType byte) (n int, err error) {
Can we rename this writeTLSRecord? I believe TLS should be in all caps.
security/s2a/internal/record/record_test.go, line 523 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Why are we setting the in traffic secret equal to the out traffic secret? Can we call
tc.inTrafficSecretsimplytrafficSecret?
I think this is actually part of my Read implementation tests. I put this because OutTrafficSecret doesn't matter for this test, but it needed to be populated with something so that NewConn doesn't fail.
It could be replaced with something like
OutTrafficSecret: make([]byte, len(tc.inTrafficSecret))
if we want. Or just trafficSecret as you suggest.
security/s2a/internal/record/record_test.go, line 1052 at r4 (raw file):
desc string ciphersuite s2apb.Ciphersuite trafficSecret []byte
Could we run gofmt to make sure that the variables line up properly?
security/s2a/internal/record/record_test.go, line 1169 at r4 (raw file):
ciphersuite: s2apb.Ciphersuite_AES_128_GCM_SHA256, trafficSecret: testutil.Dehex("6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b"), records: [][]byte{
I don't think the tests for Write should be splitting the records. Rather, they should be splitting the inPlaintexts.
To split the inPlaintexts, you would need the plaintexts to be larger than 2^14.
It might be worth refactoring your writeTlsRecord to also take in a maxSize argument. Write can always pass maxPlaintextSize but for your tests, you would be able to pass a smaller value. And then you can have 2 separate tests. TestConnWrite and TestWriteTLSVersion
@matthewstevenson88 do you also have thoughts on this?
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 26 unresolved discussions (waiting on @davisgu and @matthewstevenson88)
security/s2a/internal/record/record_test.go, line 1169 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
I don't think the tests for Write should be splitting the records. Rather, they should be splitting the
inPlaintexts.To split the
inPlaintexts, you would need the plaintexts to be larger than 2^14.It might be worth refactoring your
writeTlsRecordto also take in amaxSizeargument.Writecan always passmaxPlaintextSizebut for your tests, you would be able to pass a smaller value. And then you can have 2 separate tests.TestConnWriteandTestWriteTLSVersion@matthewstevenson88 do you also have thoughts on this?
Also, I think it might be worth renaming inPlaintexts to just plaintexts and records to outRecords, and moving the outRecords below plaintexts.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 26 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/record/record.go, line 174 at r3 (raw file):
Previously, matthewstevenson88 wrote…
I don't think this is the payload length? This is just the length of the application data in the payload, and it ignores the record type byte and the tag.
Done.
security/s2a/internal/record/record.go, line 191 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Do we need to reset
outRecordsBufin any way?
we reset the index of pointer to 0 at the beginning of the loop -- but resetting it shouldnt do any harm. Done
security/s2a/internal/record/record.go, line 208 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please add a check to ensure that
len(b)is not too large.
Done.
security/s2a/internal/record/record.go, line 210 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Why
17? Please make this a constant. (And/or re-use the constants from Ryan's PR.)
Done.
security/s2a/internal/record/record.go, line 314 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Can we rename this
writeTLSRecord? I believe TLS should be in all caps.
Done.
security/s2a/internal/record/record_test.go, line 523 at r3 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
I think this is actually part of my Read implementation tests. I put this because
OutTrafficSecretdoesn't matter for this test, but it needed to be populated with something so thatNewConndoesn't fail.It could be replaced with something like
OutTrafficSecret: make([]byte, len(tc.inTrafficSecret))if we want. Or just
trafficSecretas you suggest.
Done. (used trafficSecret -- more generalized for both in n out traffic secret vars)
security/s2a/internal/record/record_test.go, line 1052 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Could we run
gofmtto make sure that the variables line up properly?
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 26 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/record/record.go, line 66 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Can we undo this change? It was intentionally changed to 23 rather than 0x17.
Done.
security/s2a/internal/record/record.go, line 76 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Same here.
Done.
security/s2a/internal/record/record.go, line 79 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Same here too.
Done.
There was a problem hiding this comment.
Thanks Davis! I've added some additional comments to record.go, but I have held off looking at the tests until the tests are working and passing.
Reviewable status: 0 of 2 files reviewed, 32 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/record/record.go, line 133 at r1 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Could we define a constant rather than using a raw value?
+1
security/s2a/internal/record/record.go, line 154 at r1 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
It was kept as 23 rather than 0x17 as per Cesar's suggestion. Because it is referred to as 23 in the RFC rather than 0x17. C++ also defines the constant as 23 rather than 0x17.
+1. Please ignore what I said.
security/s2a/internal/record/record.go, line 208 at r3 (raw file):
Previously, davisgu wrote…
Done.
I think the condition should be len(b) > tlsRecordMaxPayloadSize.
security/s2a/internal/record/record.go, line 53 at r5 (raw file):
// single TLS 1.3 record. This is the maximum size of the plaintext plus the // record type byte and 16 bytes of the tag. tlsRecordMaxPayloadSize = tlsRecordMaxPlaintextSize + 17
When you add a constant for 17, please also change 17 here to use the constant.
security/s2a/internal/record/record.go, line 184 at r5 (raw file):
copy(unusedBuf, o.UnusedBuf) } else { unusedBuf = make([]byte, 0, 2*tlsRecordMaxPlaintextSize-1)
Why was 2*tlsRecordMaxPlaintextSize - 1 chosen?
security/s2a/internal/record/record.go, line 309 at r5 (raw file):
} // Write encrypts, frames, and writes bytes from b to the underlying connection.
Please add more detail. For example:
-Mention that b should contain application data
-Mention that it divides b up into pieces of size tlsRecordMaxPlaintextSize, they are each packaged into a TLS 1.3 record of type "application data", which is then written to the wire
-Mention what occurs in some important error cases
security/s2a/internal/record/record.go, line 315 at r5 (raw file):
} // writeTLSRecord accepts bytes from b, and writes the data according to the
Please add more detail, in a similar manner to what will be written for Write.
security/s2a/internal/record/record.go, line 317 at r5 (raw file):
// writeTLSRecord accepts bytes from b, and writes the data according to the // recordType and max payload size to the underlying connection. func (p *conn) writeTLSRecord(b []byte, recordType byte, maxSize int) (n int, err error) {
Can we replace maxSize with a more descriptive name? For example, maxPlaintextBytesPerRecord.
security/s2a/internal/record/record.go, line 319 at r5 (raw file):
func (p *conn) writeTLSRecord(b []byte, recordType byte, maxSize int) (n int, err error) { n = len(b) // Calculate the output buffer size.
Is this comment still relevant?
security/s2a/internal/record/record.go, line 323 at r5 (raw file):
totalNumOfRecordBytes := len(b) + numOfRecords*17 partialBSize := len(b) if totalNumOfRecordBytes > outBufSize {
What is outBufSize? Is it defined at this stage?
security/s2a/internal/record/record.go, line 325 at r5 (raw file):
if totalNumOfRecordBytes > outBufSize { totalNumOfRecordBytes = outBufSize partialBSize = outBufSize / maxSize * tlsRecordMaxPayloadSize
Please add parentheses to this line for clarity.
security/s2a/internal/record/record.go, line 343 at r5 (raw file):
dataLen = tlsRecordMaxPayloadSize } buff := partialB[:dataLen]
Why do we need this variable? Are we able to just use partialB[:dataLen] instead?
security/s2a/internal/record/record.go, line 352 at r5 (raw file):
} // Encrypt the payload using header as aad. encrypted, err := p.encryptPayload(payload, newHeader)
Please use a more specific variable name, e.g. encryptedPayload.
security/s2a/internal/record/record.go, line 359 at r5 (raw file):
outRecordsBufIndex += dataLen + len(buff) } partialWritten, err := p.Conn.Write(p.outRecordsBuf[:outRecordsBufIndex])
What do you mean by partialWritten? If the appropriate variable name would be too long, please add a comment.
security/s2a/internal/record/record.go, line 368 at r5 (raw file):
} // encryptPayload takes in b as the payload and feeds it into the ADEED crypter
Please don't reference the AEAD crypter here, since we are using the half connection.
security/s2a/internal/record/record.go, line 379 at r5 (raw file):
// buildHeader takes in b as the payload and builds the header for it. func buildHeader(b []byte, recordType byte) (header []byte, err error) {
This function does not need to have a variable recordType.
security/s2a/internal/record/record.go, line 383 at r5 (raw file):
return nil, errors.New("plaintext length exceeds max size") } dataLen := make([]byte, tlsRecordHeaderPayloadLengthSize)
Please call this something more descriptive, e.g. payloadLengthInHex.
security/s2a/internal/record/record.go, line 386 at r5 (raw file):
// Write the length of the ciphertext, which is length of the payload, // record type byte, and 16 bytes of tag. binary.BigEndian.PutUint16(dataLen, uint16(len(b)+tlsRecordTypeSize+16))
Please use whatever constant is used for 17 instead of tlsRecordTypeSize + 16.
Alternatively, we can have 16 be a "tag size" constant, and define the "payload overhead" as the "tag size" constant plus tlsRecordTypeSize.
security/s2a/internal/record/record.go, line 387 at r5 (raw file):
// record type byte, and 16 bytes of tag. binary.BigEndian.PutUint16(dataLen, uint16(len(b)+tlsRecordTypeSize+16)) return append([]byte{recordType, tlsLegacyRecordVersion, tlsLegacyRecordVersion}, dataLen...), nil
The first byte of the header is always equal to 23, regardless of what the actual record type of the record is. (This is because a TLS 1.3 record of any type is disguised as a TLS 1.2 "application data" record, for backwards compatibility reasons.)
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 32 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/record/record.go, line 191 at r3 (raw file):
Previously, davisgu wrote…
we reset the index of pointer to 0 at the beginning of the loop -- but resetting it shouldnt do any harm. Done
Done.
security/s2a/internal/record/record.go, line 53 at r5 (raw file):
Previously, matthewstevenson88 wrote…
When you add a constant for
17, please also change 17 here to use the constant.
Done.
security/s2a/internal/record/record.go, line 184 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Why was
2*tlsRecordMaxPlaintextSize - 1chosen?
Done. removed? not sure how to "optimize unusedBuf w pre-allocation"
security/s2a/internal/record/record.go, line 309 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add more detail. For example:
-Mention thatbshould contain application data
-Mention that it dividesbup into pieces of sizetlsRecordMaxPlaintextSize, they are each packaged into a TLS 1.3 record of type "application data", which is then written to the wire
-Mention what occurs in some important error cases
Done.
security/s2a/internal/record/record.go, line 315 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add more detail, in a similar manner to what will be written for
Write.
Done.
security/s2a/internal/record/record.go, line 317 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Can we replace
maxSizewith a more descriptive name? For example,maxPlaintextBytesPerRecord.
Done.
security/s2a/internal/record/record.go, line 319 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Is this comment still relevant?
Done.
security/s2a/internal/record/record.go, line 323 at r5 (raw file):
Previously, matthewstevenson88 wrote…
What is
outBufSize? Is it defined at this stage?
defined in constants -- initial write buffer size
security/s2a/internal/record/record.go, line 325 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please add parentheses to this line for clarity.
Done.
security/s2a/internal/record/record.go, line 343 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Why do we need this variable? Are we able to just use
partialB[:dataLen]instead?
partialB gets redefined after wards -- gotta store partialB[:dataLen] somewhere.
security/s2a/internal/record/record.go, line 352 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please use a more specific variable name, e.g.
encryptedPayload.
Done.
security/s2a/internal/record/record.go, line 359 at r5 (raw file):
Previously, matthewstevenson88 wrote…
What do you mean by
partialWritten? If the appropriate variable name would be too long, please add a comment.
Done.
security/s2a/internal/record/record.go, line 368 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please don't reference the AEAD crypter here, since we are using the half connection.
Done.
security/s2a/internal/record/record.go, line 379 at r5 (raw file):
Previously, matthewstevenson88 wrote…
This function does not need to have a variable
recordType.
Done.
security/s2a/internal/record/record.go, line 383 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please call this something more descriptive, e.g.
payloadLengthInHex.
Done.
security/s2a/internal/record/record.go, line 386 at r5 (raw file):
Previously, matthewstevenson88 wrote…
Please use whatever constant is used for
17instead oftlsRecordTypeSize + 16.Alternatively, we can have
16be a "tag size" constant, and define the "payload overhead" as the "tag size" constant plustlsRecordTypeSize.
Done.
security/s2a/internal/record/record.go, line 387 at r5 (raw file):
Previously, matthewstevenson88 wrote…
The first byte of the header is always equal to 23, regardless of what the actual record type of the record is. (This is because a TLS 1.3 record of any type is disguised as a TLS 1.2 "application data" record, for backwards compatibility reasons.)
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 32 unresolved discussions (waiting on @matthewstevenson88 and @Ryanfsdf)
security/s2a/internal/record/record_test.go, line 533 at r3 (raw file):
Previously, matthewstevenson88 wrote…
Please check that whatever was written is equal to the correct TLS record (i.e. we need to use the
completedRecordspart of the test vector).
Done.
security/s2a/internal/record/record_test.go, line 1169 at r4 (raw file):
Previously, Ryanfsdf (Ryan Kim) wrote…
Also, I think it might be worth renaming
inPlaintextsto justplaintextsandrecordstooutRecords, and moving theoutRecordsbelowplaintexts.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)
security/s2a/internal/record/record.go, line 79 at r14 (raw file):
Previously, davisgu wrote…
Done. replaced outBufSize in Write with effectiveMaxRecordSize
Please change the value of outSize to tlsRecordHeaderSize + tlsRecordMaxPayloadSize.
This way p.outRecordsBuf will be initialized so it can hold exactly 1 max sized TLS 1.3 record. This is analogous to what ALTS does.
security/s2a/internal/record/record.go, line 348 at r14 (raw file):
Previously, davisgu wrote…
Done. (?) renamed outBufSize to effectiveMaxRecordSize. I dont think outBufMaxSize cant be a constant since maxPlaintextBytesPerRecord is variable
The point of outBufMaxSize is that it is independent of everything, I think. Please make outBufMaxSize a constant at the top of the file (equal to 16*outBufSize), similar to what ALTS does.
security/s2a/internal/record/record.go, line 347 at r15 (raw file):
effectiveMaxRecordSize := maxPlaintextBytesPerRecord + p.overheadSize outBufMaxSize := 16 * effectiveMaxRecordSize totalNumOfRecordBytes := len(b) + int(math.Ceil(float64(len(b))/float64(maxPlaintextBytesPerRecord)))*p.overheadSize
Please add a comment to the top of the function that says: The argument maxPlaintextBytesPerRecord MUST be greater than zero.
I think quite a few things will go awry if maxPlaintextBytesPerRecord is zero or negative.
security/s2a/internal/record/record.go, line 350 at r15 (raw file):
// maxNumPlaintextBytesInBuf is the maximum number of plaintext bytes we // can put into record buffer of size outBufMaxSize.
s/record/a
security/s2a/internal/record/record_test.go, line 1299 at r15 (raw file):
ciphersuite: s2apb.Ciphersuite_AES_128_GCM_SHA256, trafficSecret: testutil.Dehex("6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b"), plaintext: make([]byte, 1426),
Please add a comment explaining where the numbers 1426 and 32798 come from.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @matthewstevenson88 and @Ryanfsdf)
security/s2a/internal/record/record.go, line 347 at r15 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment to the top of the function that says:
The argument maxPlaintextBytesPerRecord MUST be greater than zero.I think quite a few things will go awry if
maxPlaintextBytesPerRecordis zero or negative.
Done.
security/s2a/internal/record/record.go, line 350 at r15 (raw file):
Previously, matthewstevenson88 wrote…
s/record/a
Done.
security/s2a/internal/record/record_test.go, line 1299 at r15 (raw file):
Previously, matthewstevenson88 wrote…
Please add a comment explaining where the numbers
1426and32798come from.
Done.
There was a problem hiding this comment.
Approved, with 1 nit. Thanks Davis!
Please add Cesar for Go readability.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @davisgu and @Ryanfsdf)
security/s2a/internal/record/record.go, line 80 at r16 (raw file):
// outBufSize is the initial write buffer size in bytes. outBufSize = tlsRecordHeaderSize + tlsRecordMaxPayloadSize // outBufMaxSize is the maximum write buffer size in bytes.
nit: s/write buffer/outRecordsBuf
security/s2a/internal/record/record_test.go, line 1219 at r10 (raw file):
Previously, matthewstevenson88 wrote…
Let's come back to this. I replied in another comment.
This can now be resolved.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved
security/s2a/internal/record/record.go, line 80 at r16 (raw file):
Previously, matthewstevenson88 wrote…
nit: s/write buffer/outRecordsBuf
Done!
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 18 unresolved discussions (waiting on @cesarghali and @davisgu)
security/s2a/internal/record/record.go, line 52 at r17 (raw file):
// tlsRecordTypeSize is the size in bytes of the TLS 1.3 record type. tlsRecordTypeSize = 1 // tlsTagSize is the size in bytes of the following three ciphersuites:
the size in bytes of the tag of the following...
security/s2a/internal/record/record.go, line 77 at r17 (raw file):
) const (
Any reason we have three const sections? If they're for organizing consts of different parts of the code, we need comments above each section so future code changes won't mix the consts or add new consts in the wrong place.
security/s2a/internal/record/record.go, line 81 at r17 (raw file):
outBufSize = tlsRecordHeaderSize + tlsRecordMaxPayloadSize // outBufMaxSize is the maximum outRecordsBuf size in bytes. outBufMaxSize = 16 * outBufSize
This doesn't seem to be used.
security/s2a/internal/record/record.go, line 321 at r17 (raw file):
} // Write divides b into segments of size maxPlaintextBytesPerRecord, builds a
s/write/writeTLSRecord
security/s2a/internal/record/record.go, line 324 at r17 (raw file):
// TLS 1.3 record (of type recordType) from each segment, and sends the record // to the peer. It returns the number of plaintext bytes that were successfully // sent to the peer. maxPlaintextBytesPerRecord MUST be greater than zero.
maxPlaintextBytesPerRecord is not checked to be greater than zero. Should we add that?
security/s2a/internal/record/record.go, line 332 at r17 (raw file):
if effectiveMaxPayloadSize > tlsRecordMaxPayloadSize { return 0, errors.New("Effective payload size exceeds TLS max payload size") }
A few comments about the above few lines:
- Based on the const definitions and the calculation, it seems that
effectiveMaxPayloadSizewill always be equal totlsRecordMaxPlaintextSize. So we do we need the check? - Why do we need to pass
maxPlaintextBytesPerRecordas a parameter if we always pass the const value. If it's used to test multiple records in a single write, I don't think 2^14 is a large buffer to build in a test.
The reason I'm mentioning this is because I'm a bit confused about the the many "max size" variables, especially the "effective" one.
security/s2a/internal/record/record.go, line 341 at r17 (raw file):
} // We can ignore the return value of Write, since plaintext bytes
This comment is a bit confusing. If you want to comment on something, it is why we're returning 0, and not we're ignoring the return value of Write.
The reason why we're ignoring it is because we return the number of payload bytes that are written without any overhead (header+tag). This value is always 0 here while what Write returns is the overhead.
security/s2a/internal/record/record.go, line 347 at r17 (raw file):
} effectiveMaxRecordSize := maxPlaintextBytesPerRecord + p.overheadSize
Should we use effectiveMaxPayloadSize instead of maxPlaintextBytesPerRecord?
Reiterating my above comment that having all these "max size" is confusing.
security/s2a/internal/record/record.go, line 349 at r17 (raw file):
effectiveMaxRecordSize := maxPlaintextBytesPerRecord + p.overheadSize outBufMaxSize := 16 * effectiveMaxRecordSize totalNumOfRecordBytes := len(b) + int(math.Ceil(float64(len(b))/float64(maxPlaintextBytesPerRecord)))*p.overheadSize
Please define a variable for this int(math.Ceil(float64(len(b))/float64(maxPlaintextBytesPerRecord))) separately, maybe call it numOfRecords.
security/s2a/internal/record/record.go, line 353 at r17 (raw file):
// maxNumPlaintextBytesInBuf is the maximum number of plaintext bytes we // can put into a buffer of size outBufMaxSize. maxNumPlaintextBytesInBuf := (outBufMaxSize / effectiveMaxRecordSize) * maxPlaintextBytesPerRecord
outBufMaxSize / effectiveMaxRecordSize is always 16 here. Why are we calculating it again?
security/s2a/internal/record/record.go, line 357 at r17 (raw file):
if len(p.outRecordsBuf) < totalNumOfRecordBytes { p.outRecordsBuf = make([]byte, totalNumOfRecordBytes)
Since we're writing every record to p.Conn separately, there's no need to allocate the entire write buffer. We can allocate outRecordsBuf to the max record that we're expecting to work with and reuse it, similar to ALTS.
Also in this case, this variable can be called outRecordBuf (one record).
security/s2a/internal/record/record.go, line 369 at r17 (raw file):
outRecordsBufIndex, partialB, err = p.buildRecord(partialB, recordType, outRecordsBufIndex, maxPlaintextBytesPerRecord) if err != nil { return bStart, err
Please add a comment to explain why we're returning bStart here.
security/s2a/internal/record/record.go, line 395 at r17 (raw file):
dataLen = maxPlaintextSize } buff := make([]byte, dataLen)
This will allocate for every record we build. We should avoid that. Instead, we should allocate outRecordsBuf once per
security/s2a/internal/record/record.go, line 398 at r17 (raw file):
copy(buff, plaintext[:dataLen]) plaintext = plaintext[dataLen:] // TODO(gud): Investigate whether we should preallocate 16 bytes at the
This is probably moot after changing the code to allocate outRecordsBuf once and writing to it.
security/s2a/internal/record/record.go, line 413 at r17 (raw file):
return outRecordsBufIndex, plaintext, err } record := append(header, encryptedPayload...)
This will allocate and copy. We should avoid that. Using outRecordsBuf would solve this. See my comments below.
security/s2a/internal/record/record.go, line 414 at r17 (raw file):
} record := append(header, encryptedPayload...) copy(p.outRecordsBuf[outRecordsBufIndex:], record)
We should be able to avoid this copy by writing to outRecordsBuf directly.
security/s2a/internal/record/record.go, line 422 at r17 (raw file):
// (unencrypted) payload. func (p *conn) encryptPayload(payload, header []byte) ([]byte, error) { encrypted := make([]byte, len(payload)+tlsTagSize)
We're allocating this buffer for every record we're encrypting. Can we use a outRecordsBuf that we allocate once Write call similar to what ALTS does?
security/s2a/internal/record/record.go, line 440 at r17 (raw file):
// of the ciphertext, record type byte, and tag. binary.BigEndian.PutUint16(payloadLengthInHex, uint16(payloadSize+tlsTagSize)) return append([]byte{tlsApplicationData, tlsLegacyRecordVersion, tlsLegacyRecordVersion}, payloadLengthInHex...), nil
This also allocates a buffer. We should be able to write to outRecordsBuf directly.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 18 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/record/record.go, line 52 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
the size in bytes of the tag of the following...
Done.
security/s2a/internal/record/record.go, line 77 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Any reason we have three
constsections? If they're for organizing consts of different parts of the code, we need comments above each section so future code changes won't mix the consts or add new consts in the wrong place.
Done.
security/s2a/internal/record/record.go, line 81 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This doesn't seem to be used.
Done.
security/s2a/internal/record/record.go, line 321 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
s/write/writeTLSRecord
Done.
security/s2a/internal/record/record.go, line 324 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
maxPlaintextBytesPerRecordis not checked to be greater than zero. Should we add that?
Done.
security/s2a/internal/record/record.go, line 332 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
A few comments about the above few lines:
- Based on the const definitions and the calculation, it seems that
effectiveMaxPayloadSizewill always be equal totlsRecordMaxPlaintextSize. So we do we need the check?- Why do we need to pass
maxPlaintextBytesPerRecordas a parameter if we always pass the const value. If it's used to test multiple records in a single write, I don't think 2^14 is a large buffer to build in a test.The reason I'm mentioning this is because I'm a bit confused about the the many "max size" variables, especially the "effective" one.
effectiveMaxPayloadSize is built from maxPlaintextBytesPerRecord, so its not always equal to tlsRecordMaxPlaintextSize. also, we included this as a parameter so we can split up, build, and compare test records that we borrow from the java test cases. thoughts?
security/s2a/internal/record/record.go, line 341 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This comment is a bit confusing. If you want to comment on something, it is why we're returning
0, and not we're ignoring the return value ofWrite.The reason why we're ignoring it is because we return the number of payload bytes that are written without any overhead (header+tag). This value is always 0 here while what
Writereturns is the overhead.
Done.
security/s2a/internal/record/record.go, line 347 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Should we use
effectiveMaxPayloadSizeinstead ofmaxPlaintextBytesPerRecord?Reiterating my above comment that having all these "max size" is confusing.
Done.
security/s2a/internal/record/record.go, line 349 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Please define a variable for this
int(math.Ceil(float64(len(b))/float64(maxPlaintextBytesPerRecord)))separately, maybe call itnumOfRecords.
Done.
security/s2a/internal/record/record.go, line 353 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
outBufMaxSize / effectiveMaxRecordSizeis always 16 here. Why are we calculating it again?
having just 16 * maxPlaintextBytesPerRecord will bc unclear where 16 came from -- using outBufMaxSize/effectiveMaxRecordSize is better. thoughts?
security/s2a/internal/record/record.go, line 357 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Since we're writing every record to
p.Connseparately, there's no need to allocate the entire write buffer. We can allocateoutRecordsBufto the max record that we're expecting to work with and reuse it, similar to ALTS.Also in this case, this variable can be called
outRecordBuf(one record).
Done.
security/s2a/internal/record/record.go, line 369 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Please add a comment to explain why we're returning
bStarthere.
Done.
security/s2a/internal/record/record.go, line 395 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This will allocate for every record we build. We should avoid that. Instead, we should allocate
outRecordsBufonce per
Done.
security/s2a/internal/record/record.go, line 398 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This is probably moot after changing the code to allocate
outRecordsBufonce and writing to it.
Done.
security/s2a/internal/record/record.go, line 413 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This will allocate and copy. We should avoid that. Using
outRecordsBufwould solve this. See my comments below.
Done.
security/s2a/internal/record/record.go, line 414 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
We should be able to avoid this copy by writing to
outRecordsBufdirectly.
Done.
security/s2a/internal/record/record.go, line 422 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
We're allocating this buffer for every record we're encrypting. Can we use a
outRecordsBufthat we allocate onceWritecall similar to what ALTS does?
Done.
security/s2a/internal/record/record.go, line 440 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This also allocates a buffer. We should be able to write to
outRecordsBufdirectly.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 18 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/record/record.go, line 395 at r17 (raw file):
Previously, davisgu wrote…
Done.
edit: I'm not sure it is possible, since buff is used to hold the plaintext splice and has to append recordType to it without overwriting the rest of the plaintext. is there any way to do that?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali and @davisgu)
security/s2a/internal/record/record.go, line 332 at r17 (raw file):
Previously, davisgu wrote…
effectiveMaxPayloadSize is built from maxPlaintextBytesPerRecord, so its not always equal to tlsRecordMaxPlaintextSize. also, we included this as a parameter so we can split up, build, and compare test records that we borrow from the java test cases. thoughts?
I don't think we need to feed the max size. When the code is running, the size will always be 2^14. We're introducing a change in the code to facilitate testing. This makes the code more complex and harder to read/follow. I suggest removing maxPlaintextBytesPerRecord and adjust the tests accordingly. Also, as I said before, 2^14 is not a large buffer to build in tests.
My comment about effectiveMaxPayloadSize being always equal to tlsRecordMaxPlaintextSize was referring to the production code and not tests.
Also, if we really want to adjust the max const value in the tests, we can make it a var and change its value in the test cases that need that.
security/s2a/internal/record/record.go, line 86 at r18 (raw file):
const ( // These are TLS 1.3 handshake-specific constants
Please add a period at the end of the comment sentence.
security/s2a/internal/record/record.go, line 352 at r18 (raw file):
// Write the bytes stored in outRecordBuf to p.Conn. Since we return // the number of plaintext bytes written without overhead, we will // always return 0 while Write returns the entire record length.
s/Write/p.Conn.Write
for clarity.
security/s2a/internal/record/record.go, line 366 at r18 (raw file):
recordEndIndex, err := p.buildRecord(partialB, recordType, maxPlaintextBytesPerRecord) if err != nil { // Return the amount of bytes written previously.
s/previously/so far, i.e., prior to the error
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali and @davisgu)
security/s2a/internal/record/record.go, line 353 at r17 (raw file):
Previously, davisgu wrote…
having just 16 * maxPlaintextBytesPerRecord will bc unclear where 16 came from -- using outBufMaxSize/effectiveMaxRecordSize is better. thoughts?
Done.
security/s2a/internal/record/record.go, line 86 at r18 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Please add a period at the end of the comment sentence.
Done.
security/s2a/internal/record/record.go, line 352 at r18 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
s/Write/p.Conn.Write
for clarity.
Done.
security/s2a/internal/record/record.go, line 366 at r18 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
s/previously/so far, i.e., prior to the error
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/record/record.go, line 332 at r17 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
I don't think we need to feed the max size. When the code is running, the size will always be 2^14. We're introducing a change in the code to facilitate testing. This makes the code more complex and harder to read/follow. I suggest removing
maxPlaintextBytesPerRecordand adjust the tests accordingly. Also, as I said before, 2^14 is not a large buffer to build in tests.My comment about
effectiveMaxPayloadSizebeing always equal totlsRecordMaxPlaintextSizewas referring to the production code and not tests.Also, if we really want to adjust the max const value in the tests, we can make it a var and change its value in the test cases that need that.
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r19.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali and @davisgu)
security/s2a/internal/record/record.go, line 395 at r17 (raw file):
Previously, davisgu wrote…
edit: I'm not sure it is possible, since buff is used to hold the plaintext splice and has to append recordType to it without overwriting the rest of the plaintext. is there any way to do that?
outRecordBuf is already allocated, so why can't we write in it directly? We write the head bytes, then the encrypted plaintext, then the type. Would that work? If it makes things easier, you can move the code in buildHeader here.
security/s2a/internal/record/record.go, line 325 at r19 (raw file):
func (p *conn) writeTLSRecord(b []byte, recordType byte) (n int, err error) { // Set p.outRecordBuf to the size of a single record. p.outRecordBuf = make([]byte, tlsRecordMaxSize)
This buffer is already allocated to the same size when conn is initialize. No need to reallocate it here.
security/s2a/internal/record/record.go, line 357 at r19 (raw file):
// error, calculate the total number of plaintext bytes of complete // records successfully written to the peer and return it. numberOfBytesWrittenToPeer, err := p.Conn.Write(p.outRecordBuf[:recordEndIndex])
s/numberOfBytesWrittenToPeer/nn
The variable name is too long and looking at this line of code, it is clear that it is the number of bytes written to peer :)
security/s2a/internal/record/record.go, line 373 at r19 (raw file):
// Construct the payload, which consists of application data and record type. dataLen := len(plaintext) if dataLen > tlsRecordMaxPlaintextSize {
I think we can drop this condition for the following reasons:
plaintextis at mosttlsRecordMaxPlaintextSizein length.- If it is longer, we're basically truncating the end of the plaintext buffer but I don't see any code to address that.
security/s2a/internal/record/record.go, line 398 at r19 (raw file):
// encryptPayload returns the TLS 1.3 record built from header and the // (unencrypted) payload. func (p *conn) encryptPayload(payload, header []byte) ([]byte, error) {
This function is a simple wrapper of p.outConn.Encrypt, the code can be moved directly to where it is called.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/record/record.go, line 325 at r19 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This buffer is already allocated to the same size when
connis initialize. No need to reallocate it here.
Done.
security/s2a/internal/record/record.go, line 357 at r19 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
s/numberOfBytesWrittenToPeer/nn
The variable name is too long and looking at this line of code, it is clear that it is the number of bytes written to peer :)
Done.
security/s2a/internal/record/record.go, line 373 at r19 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
I think we can drop this condition for the following reasons:
plaintextis at mosttlsRecordMaxPlaintextSizein length.- If it is longer, we're basically truncating the end of the plaintext buffer but I don't see any code to address that.
Done.
security/s2a/internal/record/record.go, line 398 at r19 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This function is a simple wrapper of
p.outConn.Encrypt, the code can be moved directly to where it is called.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r20.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali and @davisgu)
security/s2a/internal/record/record.go, line 330 at r10 (raw file):
Previously, matthewstevenson88 wrote…
After looking at the ALTS code, it is written in such a way that it does not allow one to send zero-length plaintext messages. However, TLS 1.3 explicitly allows this. For that reason, I think it is fine to have a separate if statement to handle this case, even though it duplicates a little bit of code.
@cesarghali WDYT?
IIUC, I doesn't seem that we can use the code below for this special case because the for loop will not execute when len(b) == 0, so Write will not be called. In that case, it's fine to keep this code.
security/s2a/internal/record/record.go, line 371 at r20 (raw file):
dataLen := len(plaintext) copy(p.outRecordBuf[tlsRecordHeaderSize:tlsRecordHeaderSize + dataLen],plaintext[:dataLen])
I think you can simplify this:
copy(p.outRecordBuf[tlsRecordHeaderSize:], plaintext)
security/s2a/internal/record/record.go, line 373 at r20 (raw file):
copy(p.outRecordBuf[tlsRecordHeaderSize:tlsRecordHeaderSize + dataLen],plaintext[:dataLen]) p.outRecordBuf[tlsRecordHeaderSize + dataLen] = recordType payload := p.outRecordBuf[tlsRecordHeaderSize:tlsRecordHeaderSize + dataLen + 1]
Please add a comment at the end of the line: // 1 is for the recordType
security/s2a/internal/record/record.go, line 381 at r20 (raw file):
// Encrypt the payload using header as aad. encryptedPayload, err := p.outConn.Encrypt(p.outRecordBuf[tlsRecordHeaderSize : len(payload)+tlsTagSize][:0], payload, header)
Do we need len(payload)+tlsTagSize? Can we use p.outRecordBuf[tlsRecordHeaderSize:]?
Also, do we need to clear the slice using [:0] (I'm assuming that's the goal of this)? The The ciphertext should always be larger than the playload for two reasons: (1) the tag, and (2) padded bytes.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/record/record.go, line 371 at r20 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
I think you can simplify this:
copy(p.outRecordBuf[tlsRecordHeaderSize:], plaintext)
Done.
security/s2a/internal/record/record.go, line 373 at r20 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Please add a comment at the end of the line:
// 1 is for the recordType
Done.
security/s2a/internal/record/record.go, line 381 at r20 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
Do we need
len(payload)+tlsTagSize? Can we usep.outRecordBuf[tlsRecordHeaderSize:]?Also, do we need to clear the slice using
[:0](I'm assuming that's the goal of this)? The The ciphertext should always be larger than the playload for two reasons: (1) the tag, and (2) padded bytes.
Done. [:0] is required, since we do not want the zero bytes
There was a problem hiding this comment.
Reviewed 1 of 1 files at r21.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @davisgu)
security/s2a/internal/record/record.go, line 323 at r21 (raw file):
// to the peer. It returns the number of plaintext bytes that were successfully // sent to the peer. maxPlaintextBytesPerRecord MUST be greater than zero. func (p *conn) writeTLSRecord(b []byte, recordType byte) (n int, err error) {
We probably don't need this function anymore. Its code can be moved to Write.
security/s2a/internal/record/record_test.go, line 1068 at r21 (raw file):
} func TestBuildInvalidHeader(t *testing.T) {
This should be combined with the above test under TestBuildHeader.
security/s2a/internal/record/record_test.go, line 1109 at r21 (raw file):
} func TestConnWrite(t *testing.T) {
TestWrite
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @cesarghali)
security/s2a/internal/record/record.go, line 323 at r21 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
We probably don't need this function anymore. Its code can be moved to
Write.
Done. (kept as is)
security/s2a/internal/record/record_test.go, line 1068 at r21 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
This should be combined with the above test under
TestBuildHeader.
Done.
security/s2a/internal/record/record_test.go, line 1109 at r21 (raw file):
Previously, cesarghali (Cesar Ghali) wrote…
TestWrite
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r19, 1 of 1 files at r22.
Reviewable status:complete! all files reviewed, all discussions resolved
This change is