Fix PR comments: remove global vars, correct error formatting and godoc comments.

gssapi/WrapToken.go

@@ -34,7 +34,7 @@ From RFC 4121, section 4.2.6.2:
           6..7     RRC       Contains the "right rotation count" in big-
                              endian order, as described in section
                              4.2.5.
-          8..15    SND_SEQ   Sequence number field in clear text,
+          8..15    SndSeqNum   Sequence number field in clear text,
                              expressed in big-endian order.
           16..last Data      Encrypted data for Wrap tokens with
                              confidentiality, or plaintext data followed
@@ -51,55 +51,64 @@ Quick notes:
 	- When computing checksums, EC and RRC MUST be set to 0.
     - Wrap Tokens are not ASN.1 encoded.
 */
-var (
-	HdrLen              = 16 // Length of the Wrap Token's header
-	GSSWrapTokenID      = [2]byte{0x05, 0x04}
-	FillerByte     byte = 0xFF
-	ChecksumECRRC       = [4]byte{0x00, 0x00, 0x00, 0x00}
-	ENC                 = binary.BigEndian
+const (
+	HdrLen          = 16 // Length of the Wrap Token's header
+	FillerByte byte = 0xFF
 )
 
+// WrapToken represents a GSS API Wrap token, as defined in RFC 4121.
+// It contains the header fields, the payload and the checksum, and provides
+// the logic for converting to/from bytes plus computing and verifying checksums
 type WrapToken struct {
 	// const GSS Token ID: 0x0504
-	Flags byte // acceptor, sealed, acceptor subkey
+	Flags byte // contains three flags: acceptor, sealed, acceptor subkey
 	// const Filler: 0xFF
-	EC       uint16 // checksum length. big-endian
-	RRC      uint16 // right rotation count. big-endian
-	SND_SEQ  uint64 // sender's sequence number. big-endian
-	Payload  []byte // your data! :)
-	CheckSum []byte // authenticated checksum of { payload | header }
+	EC        uint16 // checksum length. big-endian
+	RRC       uint16 // right rotation count. big-endian
+	SndSeqNum uint64 // sender's sequence number. big-endian
+	Payload   []byte // your data! :)
+	CheckSum  []byte // authenticated checksum of { payload | header }
 }
 
-// Get them bytes!
+// Return the 2 bytes identifying a GSS API Wrap token
+func getGssWrapTokenId() *[2]byte {
+	return &[2]byte{0x05, 0x04}
+}
+
+// Marshal the WrapToken into a byte slice.
+// The payload should have been set and the checksum computed, otherwise an error is returned.
 func (wt *WrapToken) Marshal() ([]byte, error) {
 	if wt.CheckSum == nil {
-		return nil, errors.New("Checksum has not been set.")
+		return nil, errors.New("checksum has not been set")
 	}
 	if wt.Payload == nil {
-		return nil, errors.New("Payload has not been set.")
+		return nil, errors.New("payload has not been set")
 	}
 
 	pldOffset := HdrLen                    // Offset of the payload in the token
 	chkSOffset := HdrLen + len(wt.Payload) // Offset of the checksum in the token
 
 	bytes := make([]byte, chkSOffset+int(wt.EC))
-	copy(bytes[0:], GSSWrapTokenID[:])
+	copy(bytes[0:], getGssWrapTokenId()[:])
 	bytes[2] = wt.Flags
 	bytes[3] = FillerByte
-	ENC.PutUint16(bytes[4:6], wt.EC)
-	ENC.PutUint16(bytes[6:8], wt.RRC)
-	ENC.PutUint64(bytes[8:16], wt.SND_SEQ)
+	binary.BigEndian.PutUint16(bytes[4:6], wt.EC)
+	binary.BigEndian.PutUint16(bytes[6:8], wt.RRC)
+	binary.BigEndian.PutUint64(bytes[8:16], wt.SndSeqNum)
 	copy(bytes[pldOffset:], wt.Payload)
 	copy(bytes[chkSOffset:], wt.CheckSum)
 	return bytes, nil
 }
 
+// ComputeAndSetCheckSum uses the passed encryption key and key usage to compute the checksum over the payload and
+// the header, and sets the CheckSum field of this WrapToken.
+// If the payload has not been set or the checksum has already been set, an error is returned.
 func (wt *WrapToken) ComputeAndSetCheckSum(key types.EncryptionKey, keyUsage uint32) error {
 	if wt.Payload == nil {
-		return errors.New("Payload has not been set.")
+		return errors.New("payload has not been set")
 	}
 	if wt.CheckSum != nil {
-		return errors.New("Checksum has already been computed.")
+		return errors.New("checksum has already been computed")
 	}
 	chkSum, cErr := wt.ComputeCheckSum(key, keyUsage)
 	if cErr != nil {
@@ -109,10 +118,10 @@ func (wt *WrapToken) ComputeAndSetCheckSum(key types.EncryptionKey, keyUsage uin
 	return nil
 }
 
-// Compute and return the checksum of this token, computed using the passed key and key usage
+// ComputeCheckSum computes and returns the checksum of this token, computed using the passed key and key usage.
 // Conforms to RFC 4121 in that the checksum will be computed over { body | header },
 // with the EC and RRC flags zeroed out.
-// In the context of Kerberos Wrap tokens, mostly keyusage's GSSAPI_ACCEPTOR_SEAL (=22)
+// In the context of Kerberos Wrap tokens, mostly keyusage GSSAPI_ACCEPTOR_SEAL (=22)
 // and GSSAPI_INITIATOR_SEAL (=24) will be used.
 // Note: This will NOT update the struct's Checksum field.
 func (wt *WrapToken) ComputeCheckSum(key types.EncryptionKey, keyUsage uint32) ([]byte, error) {
@@ -122,7 +131,7 @@ func (wt *WrapToken) ComputeCheckSum(key types.EncryptionKey, keyUsage uint32) (
 	// Build a slice containing { payload | header }
 	checksumMe := make([]byte, HdrLen+len(wt.Payload))
 	copy(checksumMe[0:], wt.Payload)
-	copy(checksumMe[len(wt.Payload):], getChecksumHeader(wt.Flags, wt.SND_SEQ))
+	copy(checksumMe[len(wt.Payload):], getChecksumHeader(wt.Flags, wt.SndSeqNum))
 
 	encType, err := crypto.GetEtype(key.KeyType)
 	if err != nil {
@@ -135,78 +144,72 @@ func (wt *WrapToken) ComputeCheckSum(key types.EncryptionKey, keyUsage uint32) (
 func getChecksumHeader(flags byte, senderSeqNum uint64) []byte {
 	header := make([]byte, 16)
 	copy(header[0:], []byte{0x05, 0x04, flags, 0xFF, 0x00, 0x00, 0x00, 0x00})
-	ENC.PutUint64(header[8:], senderSeqNum)
+	binary.BigEndian.PutUint64(header[8:], senderSeqNum)
 	return header
 }
 
-// Compute the payload + header checksum with the provided key and usage,
-// and compare it to the checksum present in the token
+// VerifyCheckSum computes the token's checksum with the provided key and usage,
+// and compares it to the checksum present in the token.
+// In case of any failure, (false, Err) is returned, with Err an explanatory error.
 func (wt *WrapToken) VerifyCheckSum(key types.EncryptionKey, keyUsage uint32) (bool, error) {
 	computed, cErr := wt.ComputeCheckSum(key, keyUsage)
 	if cErr != nil {
 		return false, cErr
 	}
 	if !bytes.Equal(computed, wt.CheckSum) {
-		return false, errors.New(
-			fmt.Sprintf("Checksum mismatch. Computed: %s, Contained in token: %s",
-				hex.EncodeToString(computed), hex.EncodeToString(wt.CheckSum)))
+		return false, fmt.Errorf(
+			"checksum mismatch. Computed: %s, Contained in token: %s",
+			hex.EncodeToString(computed), hex.EncodeToString(wt.CheckSum))
 	}
 	return true, nil
 }
 
-// Parse a wrap token
-// if expectFromAcceptor is true, we expect the token to have been emitted by the gss acceptor,
-// and will check the according flag
-func UnmarshalWrapToken(b []byte, expectFromAcceptor bool) (*WrapToken, error) {
+// Unmarshal bytes into the corresponding WrapToken.
+// If expectFromAcceptor is true, we expect the token to have been emitted by the gss acceptor,
+// and will check the according flag, returning an error if the token does not match the expectation.
+func (wt *WrapToken) Unmarshal(b []byte, expectFromAcceptor bool) error {
 	// Check if we can read a whole header
 	if len(b) < 16 {
-		return nil, errors.New("bytes shorter than header length.")
+		return errors.New("bytes shorter than header length.")
 	}
 	// Is the Token ID correct?
-	if !bytes.Equal(GSSWrapTokenID[:], b[0:2]) {
-		return nil, errors.New(
-			fmt.Sprintf("Wrong Token ID. Expected %s, was %s",
-				hex.EncodeToString(GSSWrapTokenID[:]),
-				hex.EncodeToString(b[0:2])))
+	if !bytes.Equal(getGssWrapTokenId()[:], b[0:2]) {
+		return fmt.Errorf("wrong Token ID. Expected %s, was %s",
+			hex.EncodeToString(getGssWrapTokenId()[:]),
+			hex.EncodeToString(b[0:2]))
 	}
 	// Check the acceptor flag
 	flags := b[2]
 	isFromAcceptor := flags&0x01 == 1
 	if isFromAcceptor && !expectFromAcceptor {
-		return nil, errors.New("Unexpected acceptor flag is set. not expecting a token from the acceptor.")
+		return errors.New("unexpected acceptor flag is set: not expecting a token from the acceptor")
 	}
 	if !isFromAcceptor && expectFromAcceptor {
-		return nil, errors.New("Expected acceptor flag is not set. expecting a token from the acceptor, not the initiator.")
+		return errors.New("expected acceptor flag is not set: expecting a token from the acceptor, not the initiator")
 	}
 	// Check the filler byte
 	if b[3] != FillerByte {
-		return nil, errors.New(
-			fmt.Sprintf("Unexpected filler byte: expecting 0xFF, was %s ", hex.EncodeToString(b[3:4])))
+		return fmt.Errorf("unexpected filler byte: expecting 0xFF, was %s ", hex.EncodeToString(b[3:4]))
 	}
-	checksumL := ENC.Uint16(b[4:6])
+	checksumL := binary.BigEndian.Uint16(b[4:6])
 	// Sanity check on the checksum length
 	if int(checksumL) > len(b)-HdrLen {
-		return nil, errors.New(
-			fmt.Sprintf("Inconsistent checksum length. %d bytes to parse, checksum length is %d", len(b), checksumL))
-	}
-	rrc := ENC.Uint16(b[6:8])
-	seqNum := ENC.Uint64(b[8:16])
-	payload := b[16 : len(b)-int(checksumL)]
-	checksum := b[len(b)-int(checksumL):]
-	return &WrapToken{
-		Flags:    flags,
-		EC:       checksumL,
-		RRC:      rrc,
-		SND_SEQ:  seqNum,
-		Payload:  payload,
-		CheckSum: checksum,
-	}, nil
+		return fmt.Errorf("inconsistent checksum length: %d bytes to parse, checksum length is %d", len(b), checksumL)
+	}
+
+	wt.Flags = flags
+	wt.EC = checksumL
+	wt.RRC = binary.BigEndian.Uint16(b[6:8])
+	wt.SndSeqNum = binary.BigEndian.Uint64(b[8:16])
+	wt.Payload = b[16 : len(b)-int(checksumL)]
+	wt.CheckSum = b[len(b)-int(checksumL):]
+	return nil
 }
 
-// Build a new initiator token (acceptor flag will be set to 0) and compute the authenticated checksum.
+// NewInitiatorToken builds a new initiator token (acceptor flag will be set to 0) and computes the authenticated checksum.
 // Other flags are set to 0, and the RRC and sequence number are initialized to 0.
-// Note that in certain circumstances you may need to provide a sequence number that has been defined earlier,
-// this is currently not supported.
+// Note that in certain circumstances you may need to provide a sequence number that has been defined earlier.
+// This is currently not supported.
 func NewInitiatorToken(payload []byte, key types.EncryptionKey) (*WrapToken, error) {
 	encType, err := crypto.GetEtype(key.KeyType)
 	if err != nil {
@@ -216,10 +219,10 @@ func NewInitiatorToken(payload []byte, key types.EncryptionKey) (*WrapToken, err
 	token := WrapToken{
 		Flags: 0x00, // all zeroed out (this is a token sent by the initiator)
 		// Checksum size: lenth of output of the HMAC function, in bytes.
-		EC:      uint16(encType.GetHMACBitLength() / 8),
-		RRC:     0,
-		SND_SEQ: 0,
-		Payload: payload,
+		EC:        uint16(encType.GetHMACBitLength() / 8),
+		RRC:       0,
+		SndSeqNum: 0,
+		Payload:   payload,
 	}
 
 	if err := token.ComputeAndSetCheckSum(key, keyusage.GSSAPI_INITIATOR_SEAL); err != nil {

gssapi/WrapToken_test.go

@@ -33,12 +33,12 @@ func getSessionKey() types.EncryptionKey {
 func getChallengeReference() *WrapToken {
 	challenge, _ := hex.DecodeString(testChallengeFromAcceptor)
 	return &WrapToken{
-		Flags:    0x01,
-		EC:       12,
-		RRC:      0,
-		SND_SEQ:  binary.BigEndian.Uint64(challenge[8:16]),
-		Payload:  []byte{0x01, 0x01, 0x00, 0x00},
-		CheckSum: challenge[20:32],
+		Flags:     0x01,
+		EC:        12,
+		RRC:       0,
+		SndSeqNum: binary.BigEndian.Uint64(challenge[8:16]),
+		Payload:   []byte{0x01, 0x01, 0x00, 0x00},
+		CheckSum:  challenge[20:32],
 	}
 }
 
@@ -51,12 +51,12 @@ func getChallengeReferenceNoChksum() *WrapToken {
 func getResponseReference() *WrapToken {
 	response, _ := hex.DecodeString(testChallengeReplyFromInitiator)
 	return &WrapToken{
-		Flags:    0x00,
-		EC:       12,
-		RRC:      0,
-		SND_SEQ:  0,
-		Payload:  []byte{0x01, 0x01, 0x00, 0x00},
-		CheckSum: response[20:32],
+		Flags:     0x00,
+		EC:        12,
+		RRC:       0,
+		SndSeqNum: 0,
+		Payload:   []byte{0x01, 0x01, 0x00, 0x00},
+		CheckSum:  response[20:32],
 	}
 }
 
@@ -68,54 +68,71 @@ func getResponseReferenceNoChkSum() *WrapToken {
 
 func TestUnmarshal_Challenge(t *testing.T) {
 	challenge, _ := hex.DecodeString(testChallengeFromAcceptor)
-	decodedToken, err := UnmarshalWrapToken(challenge, true)
+	var wt WrapToken
+	err := wt.Unmarshal(challenge, true)
 	assert.Nil(t, err, "Unexpected error occurred.")
-	assert.Equal(t, getChallengeReference(), decodedToken, "Token not decoded as expected.")
+	assert.Equal(t, getChallengeReference(), &wt, "Token not decoded as expected.")
 }
 
 func TestUnmarshalFailure_Challenge(t *testing.T) {
 	challenge, _ := hex.DecodeString(testChallengeFromAcceptor)
-	decodedToken, err := UnmarshalWrapToken(challenge, false)
+	var wt WrapToken
+	err := wt.Unmarshal(challenge, false)
 	assert.NotNil(t, err, "Expected error did not occur: a message from the acceptor cannot be expected to be sent from the initiator.")
-	assert.Nil(t, decodedToken, "Token should not have been decoded")
+	assert.Nil(t, wt.Payload, "Token fields should not have been initialised")
+	assert.Nil(t, wt.CheckSum, "Token fields should not have been initialised")
+	assert.Equal(t, byte(0x00), wt.Flags, "Token fields should not have been initialised")
+	assert.Equal(t, uint16(0), wt.EC, "Token fields should not have been initialised")
+	assert.Equal(t, uint16(0), wt.RRC, "Token fields should not have been initialised")
+	assert.Equal(t, uint64(0), wt.SndSeqNum, "Token fields should not have been initialised")
 }
 
 func TestUnmarshal_ChallengeReply(t *testing.T) {
 	response, _ := hex.DecodeString(testChallengeReplyFromInitiator)
-	decodedResp, err := UnmarshalWrapToken(response, false)
+	var wt WrapToken
+	err := wt.Unmarshal(response, false)
 	assert.Nil(t, err, "Unexpected error occurred.")
-	assert.Equal(t, getResponseReference(), decodedResp, "Token not decoded as expected.")
+	assert.Equal(t, getResponseReference(), &wt, "Token not decoded as expected.")
 }
 
 func TestUnmarshalFailure_ChallengeReply(t *testing.T) {
 	response, _ := hex.DecodeString(testChallengeReplyFromInitiator)
-	decodedResp, err := UnmarshalWrapToken(response, true)
+	var wt WrapToken
+	err := wt.Unmarshal(response, true)
 	assert.NotNil(t, err, "Expected error did not occur: a message from the initiator cannot be expected to be sent from the acceptor.")
-	assert.Nil(t, decodedResp, "Token should not have been decoded.")
+	assert.Nil(t, wt.Payload, "Token fields should not have been initialised")
+	assert.Nil(t, wt.CheckSum, "Token fields should not have been initialised")
+	assert.Equal(t, byte(0x00), wt.Flags, "Token fields should not have been initialised")
+	assert.Equal(t, uint16(0), wt.EC, "Token fields should not have been initialised")
+	assert.Equal(t, uint16(0), wt.RRC, "Token fields should not have been initialised")
+	assert.Equal(t, uint64(0), wt.SndSeqNum, "Token fields should not have been initialised")
 }
 
 func TestChallengeChecksumVerification(t *testing.T) {
 	challenge, _ := hex.DecodeString(testChallengeFromAcceptor)
-	decodedToken, _ := UnmarshalWrapToken(challenge, true)
-	challengeOk, cErr := decodedToken.VerifyCheckSum(getSessionKey(), acceptorSeal)
+	var wt WrapToken
+	wt.Unmarshal(challenge, true)
+	challengeOk, cErr := wt.VerifyCheckSum(getSessionKey(), acceptorSeal)
 	assert.Nil(t, cErr, "Error occurred during checksum verification.")
 	assert.True(t, challengeOk, "Checksum verification failed.")
 }
 
 func TestResponseChecksumVerification(t *testing.T) {
 	reply, _ := hex.DecodeString(testChallengeReplyFromInitiator)
-	decodedReply, _ := UnmarshalWrapToken(reply, false)
-	replyOk, rErr := decodedReply.VerifyCheckSum(getSessionKey(), initiatorSeal)
+	var wt WrapToken
+	wt.Unmarshal(reply, false)
+	replyOk, rErr := wt.VerifyCheckSum(getSessionKey(), initiatorSeal)
 	assert.Nil(t, rErr, "Error occurred during checksum verification.")
 	assert.True(t, replyOk, "Checksum verification failed.")
 }
 
 func TestChecksumVerificationFailure(t *testing.T) {
 	challenge, _ := hex.DecodeString(testChallengeFromAcceptor)
-	decodedToken, _ := UnmarshalWrapToken(challenge, true)
+	var wt WrapToken
+	wt.Unmarshal(challenge, true)
 
 	// Test a failure with the correct key but wrong keyusage:
-	challengeOk, cErr := decodedToken.VerifyCheckSum(getSessionKey(), initiatorSeal)
+	challengeOk, cErr := wt.VerifyCheckSum(getSessionKey(), initiatorSeal)
 	assert.NotNil(t, cErr, "Expected error did not occur.")
 	assert.False(t, challengeOk, "Checksum verification succeeded when it should have failed.")
 
@@ -125,7 +142,7 @@ func TestChecksumVerificationFailure(t *testing.T) {
 		KeyValue: wrongKeyVal,
 	}
 	// Test a failure with the wrong key but correct keyusage:
-	wrongKeyOk, wkErr := decodedToken.VerifyCheckSum(badKey, acceptorSeal)
+	wrongKeyOk, wkErr := wt.VerifyCheckSum(badKey, acceptorSeal)
 	assert.NotNil(t, wkErr, "Expected error did not occur.")
 	assert.False(t, wrongKeyOk, "Checksum verification succeeded when it should have failed.")
 }
@@ -158,5 +175,5 @@ func TestMarshal_Failures(t *testing.T) {
 func TestNewInitiatorTokenSignatureAndMarshalling(t *testing.T) {
 	token, tErr := NewInitiatorToken([]byte{0x01, 0x01, 0x00, 0x00}, getSessionKey())
 	assert.Nil(t, tErr, "Unexepected error.")
-	assert.Equal(t, getResponseReference(), token)
+	assert.Equal(t, getResponseReference(), token, "Token failed to be marshalled to the expected bytes.")
 }