Skip to content

Conversation

@batuhan
Copy link
Contributor

@batuhan batuhan commented Jan 2, 2026

No description provided.

Removed the CGo-based Juicebox SDK under pkg/juicebox and replaced it with a new pure Go implementation in pkg/juiceboxgo. Updated keybackup.go to use the new juiceboxgo package and types. Updated go.mod and go.sum to add new dependencies required by the Go implementation.
Introduces zerolog-based debug logging to the realm client for request/response tracing and error reporting. Refactors request URL construction to append '/req' to base addresses. Implements custom CBOR marshaling for SecretsRequest to match Juicebox protocol expectations. Adds CBOR struct tags to noise handshake types for correct serialization.
Implements Ed25519 signature verification for OPRF public keys in the client recovery flow. Refactors Noise handshake request structure to wrap handshake requests in a dedicated type, aligning with Rust enum variants. Also ensures mix_hash is called with an empty prologue in the Noise handshake.
Added comprehensive test files for OPRF, PIN hashing, and Shamir secret sharing to verify compatibility with Rust SDK. Improved PIN recovery error handling in login flow to allow user retries and display remaining guesses. Added debug logging to Juicebox client for key recovery and OPRF phases. Ensured realm sorting in config for consistent share indices. Introduced Rust-compatible Duration type for session lifetimes.
Copilot AI review requested due to automatic review settings January 2, 2026 16:30
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR replaces the FFI-based Juicebox SDK implementation with a pure Go implementation for PIN-protected secret recovery. The change eliminates the C dependency and implements all cryptographic operations (OPRF, Shamir secret sharing, Noise protocol) directly in Go.

Key changes:

  • Implements pure Go cryptographic primitives using go-ristretto for elliptic curve operations
  • Adds complete Juicebox recovery protocol with 3-phase secret retrieval
  • Replaces C FFI bindings with native Go HTTP client and CBOR serialization

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 16 comments.

Show a summary per file
File Description
pkg/juiceboxgo/types/types.go Core type definitions for realms, keys, and secrets
pkg/juiceboxgo/types/errors.go Error types including RecoverError with guess tracking
pkg/juiceboxgo/secretsharing/shamir.go Shamir secret sharing with Lagrange interpolation
pkg/juiceboxgo/oprf/oprf.go OPRF protocol implementation with Ristretto255
pkg/juiceboxgo/oprf/dleq.go DLEQ proof verification for OPRF correctness
pkg/juiceboxgo/pin/hash.go Argon2id-based PIN hashing
pkg/juiceboxgo/crypto/encrypt.go ChaCha20-Poly1305 encryption and key derivation
pkg/juiceboxgo/noise/client.go Noise_NK protocol for hardware realm communication
pkg/juiceboxgo/requests/requests.go CBOR request/response types for realm API
pkg/juiceboxgo/realm/client.go HTTP client for realm communication with session management
pkg/juiceboxgo/client.go Main recovery client implementing 3-phase protocol
pkg/juiceboxgo/config.go Configuration parsing with realm sorting
pkg/connector/login.go Integration with login flow including PIN retry logic
pkg/connector/keybackup.go Updated to use pure Go client
pkg/juicebox/* Removed FFI-based implementation files

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +406 to +412
minGuesses := uint16(0xFFFF)
for _, g := range allGuessesRemaining {
if g < minGuesses {
minGuesses = g
}
}
return UnlockKey{}, ErrInvalidPin(minGuesses)
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimum guesses calculation iterates through all guesses to find the minimum, but it initializes minGuesses to 0xFFFF which could be misleading if allGuessesRemaining is empty. Consider checking if the slice is empty first to avoid returning a meaningless maximum value.

Copilot uses AI. Check for mistakes.
Comment on lines +212 to +237
// Check if this is an invalid PIN error that allows retry
var recoverErr *juiceboxgo.RecoverError
if errors.As(err, &recoverErr) && recoverErr.GuessesRemaining != nil {
guessesLeft := *recoverErr.GuessesRemaining
if guessesLeft > 0 {
// Return the same step with error message to allow retry
return &bridgev2.LoginStep{
Type: bridgev2.LoginStepTypeUserInput,
StepID: LoginStepJuiceboxPIN,
Instructions: fmt.Sprintf(
"**Invalid PIN.** You have %d guesses remaining.\n\nEnter your 4-digit PIN to recover your encryption keys from Juicebox.",
guessesLeft,
),
UserInputParams: &bridgev2.LoginUserInputParams{
Fields: []bridgev2.LoginInputDataField{
{
Type: bridgev2.LoginInputFieldTypePassword,
ID: "pin",
Name: "PIN",
Description: "4-digit PIN for key recovery",
},
},
},
}, nil
}
// No guesses remaining - user is locked out
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling for invalid PIN retry logic checks for GuessesRemaining != nil but doesn't verify that the RecoverError itself wraps an actual invalid PIN error. The code should also check that recoverErr.Reason indicates an invalid PIN to avoid treating other errors (like transient network errors) as retry-able PIN attempts.

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +176
// Start initiates an NK handshake with the server.
// serverStaticPublic is the server's known static public key (32 bytes).
// payloadPlaintext is the optional payload to send (may be empty, but won't have forward secrecy).
func Start(serverStaticPublic []byte, payloadPlaintext []byte) (*Handshake, *HandshakeRequest, error) {
if len(serverStaticPublic) != 32 {
return nil, nil, ErrHandshakeFailed
}

// Generate client ephemeral key pair
var clientEphemeralSecret, clientEphemeralPublic [32]byte
if _, err := rand.Read(clientEphemeralSecret[:]); err != nil {
return nil, nil, err
}
curve25519.ScalarBaseMult(&clientEphemeralPublic, &clientEphemeralSecret)

// Initialize h and ck
h := blake2s.Sum256([]byte(protocolName))
ck := h

// mix_hash(prologue) - empty prologue
h = mixHash(h, []byte{})
// mix_hash(serverStaticPublic)
h = mixHash(h, serverStaticPublic)
// mix_hash(clientEphemeralPublic)
h = mixHash(h, clientEphemeralPublic[:])

// DH(clientEphemeral, serverStatic)
var serverPub [32]byte
copy(serverPub[:], serverStaticPublic)
sharedSecret, err := curve25519.X25519(clientEphemeralSecret[:], serverPub[:])
if err != nil {
return nil, nil, err
}

// mix_key(sharedSecret)
var cipher *cipherState
ck, cipher = mixKey(ck, sharedSecret)

// Encrypt payload with AD = h
ciphertext, err := cipher.encryptWithAD(payloadPlaintext, h[:])
if err != nil {
return nil, nil, err
}

// mix_hash(ciphertext)
h = mixHash(h, ciphertext)

handshake := &Handshake{
clientEphemeralSecret: clientEphemeralSecret,
h: h,
ck: ck,
}

request := &HandshakeRequest{
ClientEphemeralPublic: clientEphemeralPublic[:],
PayloadCiphertext: ciphertext,
}

return handshake, request, nil
}

// Finish completes the handshake with the server's response.
// Returns the Transport for further communication and the decrypted response payload.
func (hs *Handshake) Finish(response *HandshakeResponse) (*Transport, []byte, error) {
if len(response.ServerEphemeralPublic) != 32 {
return nil, nil, ErrHandshakeFailed
}

h := hs.h
ck := hs.ck

// mix_hash(serverEphemeralPublic)
h = mixHash(h, response.ServerEphemeralPublic)

// DH(clientEphemeral, serverEphemeral)
var serverPub [32]byte
copy(serverPub[:], response.ServerEphemeralPublic)
sharedSecret, err := curve25519.X25519(hs.clientEphemeralSecret[:], serverPub[:])
if err != nil {
return nil, nil, err
}

// mix_key(sharedSecret)
var cipher *cipherState
ck, cipher = mixKey(ck, sharedSecret)

// Decrypt payload with AD = h
plaintext, err := cipher.decryptWithAD(response.PayloadCiphertext, h[:])
if err != nil {
return nil, nil, ErrDecryption
}

// Split to get Transport (client role)
transport := split(ck, roleClient)

return transport, plaintext, nil
}
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Noise protocol implementation lacks test coverage. This is a security-critical component that handles handshake and encrypted transport. Tests should verify correct handshake completion, encryption/decryption, and nonce handling to prevent security vulnerabilities.

Copilot uses AI. Check for mistakes.
Comment on lines +57 to +63
// ErrInvalidPin creates an error for an invalid PIN with the remaining guesses.
func ErrInvalidPin(guessesRemaining uint16) error {
return &RecoverError{
Reason: errors.New("invalid PIN"),
GuessesRemaining: &guessesRemaining,
}
}
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message construction doesn't include the embedded guesses_remaining value when it's present. The RecoverError.Error() method returns the guesses count, but the ErrInvalidPin function creates a new error with message "invalid PIN" which is then wrapped. This could make error messages less informative for users trying to understand how many guesses they have left.

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +114
// Recover retrieves a PIN-protected secret from the realms.
func (c *Client) Recover(ctx context.Context, pinBytes Pin, userInfo UserInfo) (Secret, error) {
// Phase 1: Query version from realms
c.logger.Debug().Msg("Starting recovery phase 1")
version, realms, err := c.recoverPhase1(ctx)
if err != nil {
return nil, err
}
c.logger.Debug().Str("version", string(version[:])).Int("realms", len(realms)).Msg("Phase 1 complete")

// Hash PIN with Argon2
hashResult := pin.HashPIN(pinBytes, pin.HashingMode(c.config.PinHashingMode), [16]byte(version), userInfo)

// DEBUG: Log PIN hash results for comparison with Rust
c.logger.Debug().
Hex("access_key", hashResult.AccessKey[:]).
Hex("encryption_key_seed", hashResult.EncryptionKeySeed[:]).
Msg("PIN hash complete")

// Phase 2: OPRF evaluation
c.logger.Debug().Msg("Starting recovery phase 2")
unlockKey, err := c.recoverPhase2(ctx, version, realms, hashResult.AccessKey)
if err != nil {
return nil, err
}
c.logger.Debug().Msg("Phase 2 complete")

// Phase 3: Retrieve encrypted secret
c.logger.Debug().Msg("Starting recovery phase 3")
secret, err := c.recoverPhase3(ctx, version, realms, unlockKey, hashResult.EncryptionKeySeed)
if err != nil {
return nil, err
}
c.logger.Debug().Int("secret_len", len(secret)).Msg("Phase 3 complete - recovery successful")

return secret, nil
}
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main client logic in client.go (585 lines) lacks test coverage. This is critical functionality for PIN-protected secret recovery that involves multiple phases of cryptographic operations, network requests, and error handling. Consider adding tests for the Recover method and the phase1/phase2/phase3 recovery functions to ensure correctness.

Copilot uses AI. Check for mistakes.

func randomSessionID() types.SessionID {
var buf [4]byte
rand.Read(buf[:])
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The randomSessionID function doesn't check the error from rand.Read. While crypto/rand.Read rarely fails, ignoring the error could lead to using an uninitialized or partially-initialized session ID in edge cases, which could be a security issue.

Suggested change
rand.Read(buf[:])
if _, err := rand.Read(buf[:]); err != nil {
panic(fmt.Errorf("crypto/rand.Read failed: %w", err))
}

Copilot uses AI. Check for mistakes.
Comment on lines +360 to +366
for key, groupResults := range grouped {
if len(groupResults) >= threshold {
selectedResults = groupResults
unlockKeyCommitment = key.commitment
break
}
}
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to phase 3, phase 2 recovery selects the first group that meets the threshold without deterministic ordering. The break statement exits after finding any qualifying group, which could lead to non-deterministic behavior if multiple groups meet the threshold. Consider sorting groups or using consistent selection logic.

Copilot uses AI. Check for mistakes.
Eliminates the Juicebox SDK git submodule, its FFI header, and all Rust build steps from the project. Updates .gitignore, Dockerfile, and build scripts to remove references to the SDK and Rust artifacts, simplifying the build process to Go-only.
Aligned struct fields for improved readability in multiple files. Updated logger initialization to use Stringer for realm_id and replaced hardcoded HTTP method strings with http.MethodPost in realm client. No functional changes introduced.
@batuhan batuhan merged commit 0a95356 into xchat Jan 5, 2026
7 of 11 checks passed
@batuhan batuhan deleted the batuhan/xchat/juicebox branch January 5, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants