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

fix: don't print a secret key by default #112

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

therustmonk
Copy link
Contributor

Fixes for tari-project/tari#3822
Secret keys will never be printed until they are revealed explicitly with the .reveal() method call.

@therustmonk therustmonk marked this pull request as ready for review July 6, 2022 16:11
@CjS77
Copy link
Contributor

CjS77 commented Jul 6, 2022

Untested ACK. LGTM!

@CjS77
Copy link
Contributor

CjS77 commented Jul 6, 2022

Could you make this a blanket impl for T where T: SecretKey? Then any other Secret key impls get this for free

@therustmonk
Copy link
Contributor Author

I've converted it back to draft, because an alternative solution available here: #114
We should compare them.

In any case we can't have a Debug trait implementation for another trait (SecretKey), but we can add a special wrapper that will guarantee safe behavior.

@hansieodendaal
Copy link
Contributor

Interesting/nice use of the RevealedSecretKey struct.

On a side note (@CjS77 @stringhandler @deniskolodin), maybe I am missing the plot here? Do we not want to hide the secrets so it cannot be printed at all? Or is the idea that we go back and clean up all uses of .reveal() before mainnet?

@CjS77
Copy link
Contributor

CjS77 commented Jul 11, 2022

The latter.
The idea is to prevent accidental printing of secret keys anywhere.
This will happen in unexpected places.
For example Foo is a struct that derives Debug, and there's an identity field in Foo that also impls Debug. Let's say there's a secret key inside identity.
Now you're writing a function that deals with Foo. And you log some message about Foo during debugging.
You might not notice that you're printing out the secret key. And because it's a few levels deep, code searches might not turn it up either during routine security checks.
With this change, there's no problem, because the key field will automatically be redacted.
By requiring a call to reveal, the programmer is explicitly asking to provide the string representation of the secret key.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM

@stringhandler stringhandler merged commit 45c5728 into tari-project:main Jul 22, 2022
aviator-app bot pushed a commit to tari-project/tari that referenced this pull request Aug 8, 2022
Description
---
The update for the upcoming `tari-crypto` updates:
- tari-project/tari-crypto#117
- tari-project/tari-crypto#112

Motivation and Context
---


How Has This Been Tested?
---
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.

None yet

4 participants