fix: address code review issues from PR #190

- Fix base64 concatenation bug in handleInitialRequest/Response
- Fix certificate signing bug in certificate_debug.go
- Fix TestPeerCertificateExchange with proper field encryption
- Improve error handling consistency with NewAuthError
- Fix variable naming for consistency

Author: rohenaz

auth/peer.go

@@ -494,15 +494,21 @@ func (p *Peer) handleInitialRequest(ctx context.Context, message *AuthMessage, s
 		Certificates: certs,
 	}
 
-	data := message.InitialNonce + session.SessionNonce
-	sigData, err := base64.StdEncoding.DecodeString(data)
+	// Decode the nonces first before concatenating
+	initialNonceBytes, err := base64.StdEncoding.DecodeString(message.InitialNonce)
 	if err != nil {
-		return NewAuthError("failed to prepare data to sign", err)
+		return NewAuthError("failed to decode initial nonce", err)
 	}
+	sessionNonceBytes, err := base64.StdEncoding.DecodeString(session.SessionNonce)
+	if err != nil {
+		return NewAuthError("failed to decode session nonce", err)
+	}
+	// Concatenate the decoded bytes
+	sigData := append(initialNonceBytes, sessionNonceBytes...)
 
 	keyID := fmt.Sprintf("%s %s", message.InitialNonce, session.SessionNonce)
 
-	arg := wallet.CreateSignatureArgs{
+	args := wallet.CreateSignatureArgs{
 		EncryptionArgs: wallet.EncryptionArgs{
 			ProtocolID: wallet.Protocol{
 				// SecurityLevel set to 2 (SecurityLevelEveryAppAndCounterparty) as specified in BRC-31 (Authrite)
@@ -515,13 +521,12 @@ func (p *Peer) handleInitialRequest(ctx context.Context, message *AuthMessage, s
 				Counterparty: message.IdentityKey,
 			},
 		},
-		// Sign the certificate request data, as in TypeScript
 		Data: sigData,
 	}
 
-	sigResult, err := p.wallet.CreateSignature(ctx, arg, "")
+	sigResult, err := p.wallet.CreateSignature(ctx, args, "")
 	if err != nil {
-		return fmt.Errorf("failed to sign initial response: %w", err)
+		return NewAuthError("failed to sign initial response", err)
 	}
 
 	response.Signature = sigResult.Signature.Serialize()
@@ -545,16 +550,21 @@ func (p *Peer) handleInitialResponse(ctx context.Context, message *AuthMessage,
 		return ErrSessionNotFound
 	}
 
-	data := message.InitialNonce + session.SessionNonce
-
-	sigData, err := base64.StdEncoding.DecodeString(data)
+	// Decode the nonces first before concatenating
+	initialNonceBytes, err := base64.StdEncoding.DecodeString(message.InitialNonce)
+	if err != nil {
+		return NewAuthError("failed to decode initial nonce", err)
+	}
+	sessionNonceBytes, err := base64.StdEncoding.DecodeString(session.SessionNonce)
 	if err != nil {
-		return NewAuthError("failed to prepare data to sign", err)
+		return NewAuthError("failed to decode session nonce", err)
 	}
+	// Concatenate the decoded bytes
+	sigData := append(initialNonceBytes, sessionNonceBytes...)
 
 	signature, err := ec.ParseSignature(message.Signature)
 	if err != nil {
-		return fmt.Errorf("failed to parse signature: %w", err)
+		return NewAuthError("failed to parse signature", err)
 	}
 
 	verifyResult, err := p.wallet.VerifySignature(ctx, wallet.VerifySignatureArgs{
@@ -612,13 +622,13 @@ func (p *Peer) handleInitialResponse(ctx context.Context, message *AuthMessage,
 			utilsRequestedCerts,
 		)
 		if err != nil {
-			return fmt.Errorf("invalid certificates: %w", err)
+			return NewAuthError("invalid certificates", err)
 		}
 
 		for _, callback := range p.onCertificateReceivedCallbacks {
 			err := callback(senderPublicKey, message.Certificates)
 			if err != nil {
-				return fmt.Errorf("certificate received callback error: %w", err)
+				return NewAuthError("certificate received callback error", err)
 			}
 		}
 	}
@@ -638,7 +648,7 @@ func (p *Peer) handleInitialResponse(ctx context.Context, message *AuthMessage,
 	if len(message.RequestedCertificates.Certifiers) > 0 || len(message.RequestedCertificates.CertificateTypes) > 0 {
 		err = p.sendCertificates(ctx, message)
 		if err != nil {
-			return fmt.Errorf("failed to send requested certificates: %w", err)
+			return NewAuthError("failed to send requested certificates", err)
 		}
 	}
 
@@ -712,7 +722,7 @@ func (p *Peer) handleCertificateRequest(ctx context.Context, message *AuthMessag
 	// Try to parse the signature
 	signature, err := ec.ParseSignature(message.Signature)
 	if err != nil {
-		return fmt.Errorf("failed to parse signature: %w", err)
+		return NewAuthError("failed to parse signature", err)
 	}
 
 	// Verify signature
@@ -740,7 +750,7 @@ func (p *Peer) handleCertificateRequest(ctx context.Context, message *AuthMessag
 	if len(message.RequestedCertificates.Certifiers) > 0 || len(message.RequestedCertificates.CertificateTypes) > 0 {
 		err = p.sendCertificates(ctx, message)
 		if err != nil {
-			return fmt.Errorf("failed to send requested certificates: %w", err)
+			return NewAuthError("failed to send requested certificates", err)
 		}
 	}
 
@@ -776,7 +786,7 @@ func (p *Peer) handleCertificateResponse(ctx context.Context, message *AuthMessa
 	// Try to parse the signature
 	signature, err := ec.ParseSignature(message.Signature)
 	if err != nil {
-		return fmt.Errorf("failed to parse signature: %w", err)
+		return NewAuthError("failed to parse signature", err)
 	}
 
 	// Verify signature
@@ -865,7 +875,7 @@ func (p *Peer) handleGeneralMessage(ctx context.Context, message *AuthMessage, s
 	// Try to parse the signature
 	signature, err := ec.ParseSignature(message.Signature)
 	if err != nil {
-		return fmt.Errorf("failed to parse signature: %w", err)
+		return NewAuthError("failed to parse signature", err)
 	}
 
 	// Verify signature

auth/peer_test.go

@@ -405,7 +405,6 @@ func (t *LoggingMockTransport) OnData(callback func(context.Context, *AuthMessag
 
 // TestPeerCertificateExchange tests certificate request and exchange
 func TestPeerCertificateExchange(t *testing.T) {
-	t.Skip("Needs a fix for DecryptFields - probably something in test setup need to be adjusted")
 
 	var certType = tu.GetByte32FromString("testCertType")
 	requiredField := "testField"
@@ -451,13 +450,26 @@ func TestPeerCertificateExchange(t *testing.T) {
 		return &wallet.VerifySignatureResult{Valid: true}, nil
 	}
 
-	// Create raw certificates with proper base64 encoding using our helper
+	// Generate a symmetric key for field encryption
+	fieldSymmetricKeyBytes := make([]byte, 32)
+	for i := range fieldSymmetricKeyBytes {
+		fieldSymmetricKeyBytes[i] = byte(i) // Deterministic for testing
+	}
+	fieldSymmetricKey := ec.NewSymmetricKey(fieldSymmetricKeyBytes)
+	
+	// Encrypt the field value
+	plainFieldValue := []byte("decrypted field value")
+	encryptedFieldBytes, err := fieldSymmetricKey.Encrypt(plainFieldValue)
+	require.NoError(t, err)
+	encryptedFieldValue := base64.StdEncoding.EncodeToString(encryptedFieldBytes)
+
+	// Create raw certificates with encrypted fields
 	aliceCertRaw := wallet.Certificate{
 		Type:               certType,
 		SerialNumber:       tu.GetByte32FromString("serial1"),
 		Subject:            aliceSubject,
 		Certifier:          bobSubject,
-		Fields:             map[string]string{requiredField: "fieldValue"},
+		Fields:             map[string]string{requiredField: encryptedFieldValue},
 		RevocationOutpoint: tu.OutpointFromString(t, "a755810c21e17183ff6db6685f0de239fd3a0a3c0d4ba7773b0b0d1748541e2b.0"),
 	}
 
@@ -466,7 +478,7 @@ func TestPeerCertificateExchange(t *testing.T) {
 		SerialNumber:       tu.GetByte32FromString("serial2"),
 		Subject:            bobSubject,
 		Certifier:          aliceSubject,
-		Fields:             map[string]string{requiredField: "fieldValue"},
+		Fields:             map[string]string{requiredField: encryptedFieldValue},
 		RevocationOutpoint: tu.OutpointFromString(t, "a755810c21e17183ff6db6685f0de239fd3a0a3c0d4ba7773b0b0d1748541e2b.1"),
 	}
 
@@ -510,28 +522,29 @@ func TestPeerCertificateExchange(t *testing.T) {
 	logger.Printf("DEBUG: Alice cert signature: %x", aliceCert.Signature)
 	logger.Printf("DEBUG: Bob cert signature: %x", bobCert.Signature)
 
-	// Create mock keyring results - also properly encoded
-	fieldValueBase64 := base64.StdEncoding.EncodeToString([]byte("key-for-field"))
+	// Mock keyring - in real usage this would be the symmetric key encrypted for the verifier
+	// For testing, we just need valid encrypted data that MockDecrypt can "decrypt" to fieldSymmetricKeyBytes
+	encryptedSymmetricKey := base64.StdEncoding.EncodeToString(fieldSymmetricKeyBytes)
 	aliceWallet.MockProveCertificate = func(ctx context.Context, args wallet.ProveCertificateArgs, originator string) (*wallet.ProveCertificateResult, error) {
 		return &wallet.ProveCertificateResult{
-			KeyringForVerifier: map[string]string{requiredField: fieldValueBase64},
+			KeyringForVerifier: map[string]string{requiredField: encryptedSymmetricKey},
 		}, nil
 	}
 	bobWallet.MockProveCertificate = func(ctx context.Context, args wallet.ProveCertificateArgs, originator string) (*wallet.ProveCertificateResult, error) {
 		return &wallet.ProveCertificateResult{
-			KeyringForVerifier: map[string]string{requiredField: fieldValueBase64},
+			KeyringForVerifier: map[string]string{requiredField: encryptedSymmetricKey},
 		}, nil
 	}
 
-	// Configure wallet mocks for Decrypt to make DecryptFields work
+	// MockDecrypt returns the symmetric key for field decryption
 	aliceWallet.MockDecrypt = func(ctx context.Context, args wallet.DecryptArgs, originator string) (*wallet.DecryptResult, error) {
 		return &wallet.DecryptResult{
-			Plaintext: []byte("decrypted field value"),
+			Plaintext: fieldSymmetricKeyBytes,
 		}, nil
 	}
 	bobWallet.MockDecrypt = func(ctx context.Context, args wallet.DecryptArgs, originator string) (*wallet.DecryptResult, error) {
 		return &wallet.DecryptResult{
-			Plaintext: []byte("decrypted field value"),
+			Plaintext: fieldSymmetricKeyBytes,
 		}, nil
 	}
 

auth/utils/certificate_debug.go

@@ -82,7 +82,7 @@ func SignCertificateWithWalletForTest(ctx context.Context, cert wallet.Certifica
 		return encodedCert, fmt.Errorf("failed to get identity key of signer: %w", err)
 	}
 
-	cert.Certifier = publicKeyResult.PublicKey
+	encodedCert.Certifier = publicKeyResult.PublicKey
 
 	certObj, err := certificates.FromWalletCertificate(&encodedCert)
 	if err != nil {