Skip to content

refactor(server-ng): move password crypto into server_common#3562

Open
jayakasadev wants to merge 1 commit into
apache:masterfrom
jayakasadev:phase6-crypto-server-common
Open

refactor(server-ng): move password crypto into server_common#3562
jayakasadev wants to merge 1 commit into
apache:masterfrom
jayakasadev:phase6-crypto-server-common

Conversation

@jayakasadev

@jayakasadev jayakasadev commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What

Moves the password crypto helpers (hash_password, verify_password, generate_secret) out of core/server into the shared server_common crate, and repoints server-ng at the new home.

Why

core/server-ng (the new Viewstamped-Replication server that is meant to replace the legacy core/server) still reaches into core/server via use server::*. That welds the next-gen server to the very crate it is supposed to deprecate, so server-ng cannot compile or ship without dragging in dead code.

#3315 tracks unpicking those ties one cluster at a time: each shared piece moves into server_common (the legitimate shared crate, not deprecated), server-ng is repointed at it, and the work continues until server-ng can drop server = { workspace = true } entirely. This PR cuts the crypto slice: server-ng imported server::streaming::utils::crypto, a compile-time edge on the legacy crate. Moving the helper to server_common gives both servers a shared home with no dependency on server.

Changes

  • crypto.rs moved to server_common (rename, zero body changes)
  • server_common
    • pub mod crypto;
    • argon2 and rand added — crypto's only dependencies, previously pulled in by server
  • server
    • argon2 and rand dropped — crypto was their only consumer
    • keeps a backwards-compatible re-export at streaming::utils::crypto, so its in-crate call sites (http_server, shard/execution, shard/system/users, user.rs) are unchanged
  • server-ng
    • auth.rs and users.rs now use server_common::crypto

Scope

One slice of #3315. On its own it does not let server-ng drop the server dependency — other use server::* clusters (logging, segment storage, consumer offsets, shard allocator, root-user bootstrap, fs utils) still remain. It removes one of them and gives the root-user-bootstrap phase a clean foundation to build on.

Verification

  • cargo fmt --all, cargo sort --no-format --workspace — clean
  • cargo clippy --all-features --all-targets -- -D warnings on server, server_common, server-ng — clean
  • cargo machete — no unused deps
  • cargo check/buildserver, server_common, server-ng build
  • cargo test -p server_common — 64 passed
  • rg 'streaming::utils::crypto' core/server-ng/src — no matches

No behavior change: existing password hashes still verify; legacy server builds via the re-export. Rebased onto latest master; once_cell is not introduced (consistent with #3524's once_cell → std migration).

Refs #3315.

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Jun 24, 2026
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.06%. Comparing base (307fdb1) to head (5a84ad2).

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #3562       +/-   ##
=============================================
- Coverage     74.07%   61.06%   -13.01%     
  Complexity      937      937               
=============================================
  Files          1249     1247        -2     
  Lines        128248   117573    -10675     
  Branches     104116    93487    -10629     
=============================================
- Hits          94994    71791    -23203     
- Misses        30219    42585    +12366     
- Partials       3035     3197      +162     
Components Coverage Δ
Rust Core 58.28% <ø> (-16.43%) ⬇️
Java SDK 62.44% <ø> (ø)
C# SDK 71.40% <ø> (-0.66%) ⬇️
Python SDK 88.88% <ø> (ø)
PHP SDK 84.29% <ø> (ø)
Node SDK 91.35% <ø> (ø)
Go SDK 40.14% <ø> (ø)
Files with missing lines Coverage Δ
core/server-ng/src/auth.rs 0.00% <ø> (ø)
core/server-ng/src/users.rs 0.00% <ø> (ø)
core/server_common/src/crypto.rs 100.00% <ø> (ø)

... and 231 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jayakasadev jayakasadev force-pushed the phase6-crypto-server-common branch from eb32f00 to 6b41184 Compare June 24, 2026 21:41
server-ng pulled password hashing and verification from the legacy server
crate, forcing a compile-time dependency on the binary it is meant to
replace. The crypto module is three free functions over argon2 and rand
with no server-internal state, so it relocates cleanly to the shared
server_common crate.

server keeps the symbols at their original module path via a re-export, so
its own call sites are untouched. The argon2 and rand dependencies move with
the code, since crypto was their only consumer in server.

Refs apache#3315.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jayakasadev jayakasadev force-pushed the phase6-crypto-server-common branch from 6b41184 to 5a84ad2 Compare June 26, 2026 17:10
@jayakasadev jayakasadev reopened this Jun 26, 2026
@github-actions github-actions Bot removed the S-waiting-on-review PR is waiting on a reviewer label Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant