Open mentalisttraceur opened 1 year ago
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
?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.
For examples of comments that a person would find useful when trying to understand these protocol message contents from scratch, you can look at https://github.com/segmentio/kafka-go/pull/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.)
So I was looking at
newAuthenticatorChksum
, and I suggest improving it like this:For me the main benefit is clarity: I think it becomes much more obvious that if
gssapi.ContextFlagDeleg
is included one or more times inflags
, 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
anda[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.)