[server] Switch to native lib for results

[ua741]

Description

Tests

 go test ./pkg/utils/crypto -v   

[claude]

Code Review - PR #7430: Switch to Native Lib for Results

Summary

This PR completes the migration from libsodium to native Go crypto libraries by switching the return values from comparison functions to use the native implementation. This is a critical step in eliminating the external libsodium dependency.

---

✅ Strengths

1. Consistent Migration Pattern: The changes follow a clear pattern of switching from libsodium to native implementations across three crypto operations (encrypt, decrypt, hash).

2. Improved Error Handling: The PR adds proper error propagation where it was previously missing:

   • crypto.go:42,64,84: Now returns errors instead of just logging them
   • crypto.go:50: Uses stacktrace.Propagate(err, ...) for proper error context

3. Enhanced Validation: Added nil check for native encryption cipher (crypto.go:44-46), preventing potential nil pointer issues.

4. Comprehensive Testing: The PR description indicates tests are in place (go test ./pkg/utils/crypto -v).

---

🐛 Critical Bug

Line crypto.go:50 - Incorrect Error Variable

if base64.StdEncoding.EncodeToString(nativeEnc.Cipher) != base64.StdEncoding.EncodeToString(encryptedEmailBytes) ||
    base64.StdEncoding.EncodeToString(nativeEnc.Nonce) != base64.StdEncoding.EncodeToString(nonce) {
    logrus.WithField("op", "crypto_compare").Error("encryption mismatch between libsodium and native implementation")
    return ente.EncryptionResult{}, stacktrace.Propagate(err, "encryption mismatch")  // ⚠️ BUG: 'err' is nil here
}

Issue: The code tries to propagate err, but at this point in the code, err is nil (it was last checked on line 39-42 and would have returned if non-nil). This will cause stacktrace.Propagate to receive a nil error.

Fix: Use stacktrace.NewError() instead:

return ente.EncryptionResult{}, stacktrace.NewError("encryption mismatch")

This same pattern is correctly used on lines 67 and 87 for similar mismatch cases.

---

🔒 Security Considerations

1. Cryptographic Compatibility: The migration assumes the native implementation (GetEncryptedTokenNative) is cryptographically compatible with libsodium's sealed box format. Based on sealed_box_test.go:63-71, this compatibility should be verified by tests.

2. No Gradual Rollout: The PR removes the email-based feature flag (if strings.Contains(email, "@ente.io")) in userauth.go:487-492, meaning all users will immediately use the native implementation. This is appropriate if compatibility has been verified.

3. Authentication Token Security: The changes affect token encryption for:

   • Two-factor authentication (twofactor.go:186)
   • Email verification (userauth.go:487)
   • Account deletion challenges (user_delete.go:48)

   These are critical security flows, so thorough testing is essential.

---

⚡ Performance Considerations

1. Elimination of Comparison Overhead: By returning native results directly instead of running both implementations and comparing, this PR should improve performance by ~2x for these operations.

2. Dependency Reduction: Removing reliance on libsodium CGO calls should reduce CGO overhead and improve build times.

---

📝 Recommendations

1. Fix the critical bug on line 50 in crypto.go (use NewError instead of Propagate(err, ...)).

2. Consider adding metrics/logging to track the migration's success in production, especially for the critical authentication flows.

3. Verify nonce validation: Line 48 adds nonce validation to the encryption comparison. This is good, but ensure the test suite validates that nonces are handled correctly in all cases.

4. Future cleanup: Once this PR is merged and proven stable in production, consider removing the libsodium comparison code entirely and the GetEncryptedToken function (keeping only the native versions).

---

🎯 Conclusion

This is a well-structured PR that completes an important migration. The approach of validating native implementations against libsodium before switching is sound. The PR should not be merged until the critical bug on line 50 is fixed, but otherwise the changes are clean and follow good practices.

Recommendation: Request changes to fix the error handling bug, then approve after fix.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

[chatgpt-codex-connector]

Return error on native/libsodium encryption mismatch

When the libsodium and native encryption results differ, the code logs the mismatch and returns stacktrace.Propagate(err, "encryption mismatch"). At this point err is nil because the native call succeeded, so the propagated error is nil and callers see an empty EncryptionResult while thinking the operation succeeded. If the implementations ever diverge—the condition this check is trying to detect—upstream token generation will silently proceed with a zero-value cipher. Wrap a real error (e.g. stacktrace.NewError(...)) so mismatches surface as failures.

Useful? React with 👍 / 👎.

[claude]

Code Review for PR #7430

Thank you for this PR migrating to the native cryptographic implementation! This is an important step toward removing the libsodium dependency.

✅ Positive Aspects

1. Clear migration path: Completes the transition from libsodium to native Go crypto libraries, excellent for maintainability and reduces CGO dependencies.

2. Consistent approach: Systematically replaces GetEncryptedToken with GetEncryptedTokenNative across all call sites.

3. Verification logic: Properly validates that native encryption matches libsodium before switching.

4. Error handling improvement: Adds proper error propagation instead of just logging errors.

🐛 Potential Issues

1. Backward Compatibility Concerns (CRITICAL)

The encryptWithNonce function now returns native encryption results instead of libsodium results. Questions to verify:

• Are the native and libsodium implementations 100% interoperable for decryption?
• Have you verified this with production data or staging?
• Is there existing encrypted data that needs migration?

Recommendation: Add integration tests that verify old libsodium-encrypted data can be decrypted with native implementation.

2. Nonce Validation (Line 48)

The nonce comparison check seems unnecessary since we pass the same nonce to both implementations. Consider removing or adding a comment explaining why it's necessary.

3. Error Messages (Lines 50, 67, 87)

Minor issue: Extra space in error message and could be more actionable.

⚡ Performance Considerations

The code still calls BOTH libsodium AND native implementations for every operation (2x computational cost).

Recommendation:

1. Short term: Acceptable for validation during migration
2. Medium term: Add a configuration flag to skip libsodium comparison once confidence is high
3. Long term: Remove libsodium code entirely

🔒 Security Concerns

This PR changes the cryptographic implementation used in production. The native implementation appears correct, but crypto changes deserve extra caution:

Recommendations:

1. Add test vectors with known inputs/outputs from libsodium
2. Deploy to staging first with monitoring
3. Consider gradual rollout
4. Add metrics to detect decryption failures

The code now returns errors instead of logging when native crypto fails. Add monitoring/alerting for these new error paths.

📝 Missing Information

PR description is minimal. Please add:

• Why this change is being made
• What testing was performed beyond unit tests
• Whether this was tested in staging/production
• Expected impact and rollback plan

📋 Summary

Severity:

• 🔴 Critical: Verify backward compatibility with existing encrypted data
• 🟡 Medium: Performance overhead from dual implementation
• 🟢 Low: Minor error message formatting

Recommendation: Approve with conditions - needs verification of backward compatibility testing, staging deployment plan, and monitoring for crypto failures.

Great work on the native implementation! Code quality is good, but crypto changes deserve extra caution. 🔐

[claude]

Pull Request Review: Switch to native lib for results

Summary

This PR completes the migration from libsodium to Go's native crypto libraries (golang.org/x/crypto) by switching all production code paths to use the native implementations. Previously, the code was comparing outputs but still returning libsodium results; now it exclusively uses the native implementation.

---

✅ Strengths

1. Clean Migration Strategy: The PR successfully removes the email-specific rollout logic (if strings.Contains(email, "@ente.io")) and completes the gradual migration that was clearly in progress.

2. Improved Error Handling: The changes in crypto.go now properly propagate errors from the native implementation instead of silently logging and falling back to libsodium results. This is a significant improvement:

   // Before: logged error but continued with libsodium
   if nativeErr != nil {
       logrus.WithField("op", "crypto_compare").WithError(nativeErr).Error("native decryption failed")
   }
   return result, nil  // returned libsodium result
   
   // After: properly propagates the error
   if nativeErr != nil {
       logrus.WithField("op", "crypto_compare").WithError(nativeErr).Error("native decryption failed")
       return "", stacktrace.Propagate(nativeErr, "native decryption failed")
   }

3. Consistent API Usage: All three call sites (twofactor.go, user_delete.go, userauth.go) are updated consistently to use GetEncryptedTokenNative.

4. Strong Test Coverage: The test file shows comprehensive coverage including compatibility tests, error cases, and edge cases.

---

🔍 Code Quality & Best Practices

Good:

• Error handling is consistent and uses the stacktrace library throughout
• Validation logic (nil checks, length checks) is properly added
• The comparison logic now validates both cipher and nonce match

Observations:

• The encryptWithNonce, Decrypt, and GetHash functions still perform both libsodium and native operations for comparison purposes. Consider documenting whether this is temporary (for monitoring during rollout) or permanent (for ongoing validation).

---

⚠️ Potential Issues & Concerns

1. Breaking Change Without Deprecation (Medium Priority)

The old GetEncryptedToken function is not marked as deprecated and remains in the codebase. Consider:

• Adding a deprecation comment
• Or removing it entirely if no other code uses it
• Run: git grep "GetEncryptedToken[^N]" server/ to verify no other usages exist

2. Comparison Logic Still Executes Both Paths (Low-Medium Priority)

In crypto.go:34-52, the code still runs both libsodium AND native encryption, compares them, but now only returns the native result. This means:

• Performance: Double encryption overhead on every call
• Complexity: Maintenance burden of keeping both implementations

Recommendation: If validation is complete and native implementation is proven correct:

// Option A: Remove comparison entirely and just use native
func encryptWithNonce(data string, encryptionKey []byte, nonce []byte) (ente.EncryptionResult, error) {
    return encryptWithNonceNative(data, encryptionKey, nonce)
}

// Option B: Add feature flag for comparison
if compareEnabled {
    // comparison logic
}
return nativeEnc, nil

Same applies to Decrypt (lines 55-69) and GetHash (lines 72-89).

3. Missing Typo Fix (Low Priority)

Line 50 in crypto.go:

return ente.EncryptionResult{}, stacktrace.NewError("encryption mismatch  nonce or cipher not same")
//                                                                    ^^  double space

4. Security: No Migration Path for Existing Data (Context Needed)

If this change affects how tokens are encrypted/decrypted, ensure:

• ✅ The native implementation is cryptographically compatible with libsodium (tests suggest it is)
• ❓ Existing encrypted tokens can still be decrypted (need confirmation if any tokens are persisted)
• ❓ No in-flight tokens will break during deployment

Question: Are the encrypted tokens ephemeral (session-only) or persisted? If persisted, do old libsodium-encrypted tokens need to be decrypted?

5. Test Execution Not Shown (Low Priority)

The PR description mentions running tests:

go test ./pkg/utils/crypto -v

But doesn't show the output. For reviewer confidence, consider including test results in the PR description.

---

🔒 Security Considerations

Good:

1. ✅ Native implementation uses established, well-audited libraries (golang.org/x/crypto)
2. ✅ Sealed box implementation matches libsodium's algorithm (see comments in crypto_native.go:86-87)
3. ✅ Nonce derivation is deterministic and matches libsodium (BLAKE2b-192)
4. ✅ Test coverage includes tampering detection

Verify:

• ✅ The native implementation was likely already tested extensively (given the gradual rollout strategy)
• ❓ Has this been deployed to a subset of users already (the @ente.io check suggests yes)?

---

🚀 Performance Considerations

1. Current state: Functions perform ~2x crypto operations (libsodium + native)
2. After removing comparison: Expected 50% performance improvement
3. Recommendation: Add benchmarks to quantify the improvement:

   go test -bench=. ./pkg/utils/crypto

---

📋 Recommendations

High Priority:

1. Search for any remaining usages of GetEncryptedToken (without "Native") and verify it can be removed or deprecated

Medium Priority:
2. Consider removing the comparison logic in encryptWithNonce, Decrypt, and GetHash to eliminate double computation

• Or document why it must remain (e.g., monitoring in production)

3. Verify deployment strategy: Can this be deployed safely without breaking in-flight or persisted tokens?

Low Priority:
4. Fix double space typo in error message (line 50)
5. Add benchmark results to PR description
6. Document the migration timeline: Is comparison logic temporary or permanent?

---

📝 Questions for Author

1. Is the dual execution (libsodium + native) with comparison still needed, or can we remove the libsodium code paths now?
2. Are there any persisted encrypted tokens that need migration, or are all tokens ephemeral?
3. Has this change been tested in staging/production with a subset of users?
4. What's the plan for removing the old GetEncryptedToken function?

---

✅ Conclusion

This is a well-executed migration PR that completes the transition to native Go crypto. The changes are focused, tested, and improve error handling. The main suggestion is to consider removing the now-redundant comparison logic for performance, or documenting why it must remain.

Recommendation: Approve with minor suggestions - the code is safe to merge, with the improvements noted being optional optimizations rather than blockers.

Great work on the careful, gradual migration strategy! 🎉