Skip to content

Conversation

@AbhiMgowda
Copy link

Description

need to unify the secure randomness in Boundary behind the use of the SecureRandomReader field, passing it in through options where we today use crypto/rand directly.
No changes to security controls or cryptographic behavior:

  • Uses the same crypto/rand.Reader underneath
  • Salt generation remains cryptographically identical
  • No changes to access control, logging pipelines, or audit trails
  • This is a pure refactoring for API clarity; the security posture is unchanged
    Testing
    All existing tests pass:
  • ✅ internal/auth/password package tests (12.67s)
  • ✅ internal/securerandom package tests (0.94s)
    Files Changed
  • internal/securerandom/random.go — Added public Reader() accessor function
  • internal/auth/password/argon2.go — Updated to use securerandom.Reader()

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've documented the impact of any changes to security controls.
    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

@AbhiMgowda AbhiMgowda changed the title reactor: Update Password use of crypto/rand to SecureRandomReader refactor: Update Password use of crypto/rand to SecureRandomReader Nov 18, 2025
@AbhiMgowda AbhiMgowda marked this pull request as ready for review November 20, 2025 08:54
@AbhiMgowda AbhiMgowda requested a review from a team as a code owner November 20, 2025 08:54
@AbhiMgowda AbhiMgowda self-assigned this Nov 20, 2025
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This is not the change I expected to see. The change we need to make is to pass in the existing secure random reader into the context where random data is needed, not just create a new wrapper around crypto/rand. Could you take another look at the document I prepared in the JIRA ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants