Suggested Improvements to newAuthenticatorChksum

[mentalisttraceur]

So I was looking at newAuthenticatorChksum, and I suggest improving it like this:

func newAuthenticatorChksum(flags []int) []byte {
	a := make([]byte, 24)
	binary.LittleEndian.PutUint32(a[:4], 16)
+	f := uint32(0)
	for _, i := range flags {
-		if i == gssapi.ContextFlagDeleg {
-			x := make([]byte, 28-len(a))
-			a = append(a, x...)
-		}
-		f := binary.LittleEndian.Uint32(a[20:24])
		f |= uint32(i)
-		binary.LittleEndian.PutUint32(a[20:24], f)
	}
+	if f&gssapi.ContextFlagDeleg != 0 {
+		a = append(a, 0, 0, 0, 0)
+	}
+	binary.LittleEndian.PutUint32(a[20:24], f)
 	return a
}

For me the main benefit is clarity: I think it becomes much more obvious that if gssapi.ContextFlagDeleg is included one or more times in flags, the checksum just gets four zero'd bytes appended to the end.

You could also see it as an optimization benefit. (Does the compiler realize that the serialization round trip between f and a[20:24] doesn't have to happen each loop? Does the compiler realize that it could possibly save a copy of a copy-on-write page of memory if it waits to write the flag bits until after it knows to extend the checksum slice by four bytes? Well, with these changes it doesn't even have to.)

(Normally I'd just make a PR with such suggested changes and discuss it there, but your contributing guidelines suggest opening an issue first.)

[mentalisttraceur]

Another thing I'd like to do is have the code be more explicitly clear about exactly what it's doing with the delegation flag/fields and why (but I don't fully understand it so I can't suggest the best comment for that yet)...

1. It handles the "delegation" flag by adding the two uint16 fields: delegation option and delegation length, both set to zero, but it's not clear to me why this is the correct behavior?
   • I haven't yet figured out what the delegation option field is supposed to contain, but the "(=1)" in RFC 4121 section 4.1.1 seems to maybe suggest that it must be hardcoded to 1?
   • I'm... somewhat confused about what it "means" to have delegation "on" without any actual delegated credential. Why is a zero-length delegated credential better than to just ignore/clear the delegation flag?
2. Neither code nor comment actually say what's going on: I was lucky enough to already know and remember enough of the RFC 4121 that I was able to connect the dots to the delegation fields after some re-reading and thinking.

Also, I'd like to make the code more self-describing, at least by renaming a to checksum, f to flagBits, and i to flag.

func newAuthenticatorChecksum(flags []int) []byte {
	checksum = make([]byte, 24)
	binary.LittleEndian.PutUint32(checksum[0:], channelBindingLength)
	flagBits := uint32(0)
	for _, flag := range flags {
		flagBits |= uint32(flag)
	}
	if flagBits&gssapi.ContextFlagDeleg != 0 {
		// delegation fields, if present, are blank/unused:
 		checksum = append(checksum, 0, 0, 0, 0)
	}
	// channel binding information is left blank/unused
	binary.LittleEndian.PutUint32(checksum[flagsOffset:], flagBits)
	return checksum
}

In an earlier edit of this comment, I also wanted to add some comments or comment-like code to help explain all those magic numbers (24, 16, 4, 28, ...) and what each part of the checksum was, but on further thought I don't like how that was turning out.

[mentalisttraceur]

For examples of comments that a person would find useful when trying to understand these protocol message contents from scratch, you can look at segmentio/kafka-go#598, where I implemented SASL's "GSSAPI" mechanism for Kafka.

(In that PR I ended up implementing a sizeable chunk of what gokrb5 currently has in the spnego package. I don't remember if gokrb5 already had it in spnego at the time when I implemented it, and if so I don't remember if I knew of it and if so then why I didn't try to reuse it. I think probably at the time I didn't know what patterns to look for, so until I implemented it myself, I wouldn't have recognized the similarities between what the spnego code did and what the SASL GSSAPI method needed.)