Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Guarded trait for secrets protection guarantee #114

Closed

Conversation

therustmonk
Copy link
Contributor

An attempt to find a good solution for #112 (comment) they led me to this implementation (an alternative to #112) and curious conclusions.

Description:
This PR introduces a special Guarded trait that forces an implementation to:

  • always wrap a (secret) value to the GuardedWrapper (that implements Debug, but never prints the secret value)
  • implement Zeroize for the secret (and it always be called by the GuardedWrapper)

The GuardedWrapper has a special method .reveal() that returns a reference to a secret.
The beniefit of that approach: we have .reveal() calls everywhere we get access to a secret value and we could grep sources to see all the places the secret is revealed. Like .unwrap() approach, but for secrets. It's always clear now in reviewing PRs where we get the secret value

Another change is removing From<RistrettoSecretKey> for Scalar implementation, because Scalar implements Copy and easily copied in that method, but the copied value will never be zeroized. The original secret key is zeroized, becuase Drop will be called after the From implementation:

impl From<RistrettoSecretKey> for Scalar {
    fn from(k: RistrettoSecretKey) -> Self {
        k.0 // copied scalar `leaked` here and will never be cleared (zeroized)
        // `drop(k)` is happened here that will call `k.zeroize()` 
   }
}

The drawback of that approach: we have to Clone values instead of Copy, because we couldn't have both Copy and Drop at the same time.
The extra costs for cloning could be eliminated by some refactoring to using references to secrets instead of owning them.

@therustmonk therustmonk changed the title Hidden secret key 2 feat: Guarded trait for secrets protection guarantee Jul 8, 2022
@therustmonk therustmonk mentioned this pull request Jul 8, 2022
@CjS77
Copy link
Contributor

CjS77 commented Jul 8, 2022

I prefer the outcomes of #112 - even though it's a specific impl and not generally applicable to SecretKey.
This approach would require modifying every use of secret keys in the tari codebase to use GuardedSecret, right?

@therustmonk
Copy link
Contributor Author

@CjS77 yes, it requires a lot of changes in the main repository. #112 is not so strict, but is a much faster way to solve the problem.

@CjS77
Copy link
Contributor

CjS77 commented Jul 8, 2022

Cool. Let's abandon this approach, and go with #112

@therustmonk
Copy link
Contributor Author

Ok 👍

@therustmonk therustmonk closed this Jul 8, 2022
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.

2 participants