-
Notifications
You must be signed in to change notification settings - Fork 83
Redacted + improved privacy of private keys #404
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
|
||
| const RedactedType: unique symbol = Symbol.for('@mysten/redacted'); | ||
| export interface Redacted<T> { | ||
| [RedactedType]: T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might not work well across sdk versions. I think when we introduced symbols for Transaction and/or SuiClient properties it caused some issues. I'm not sure how unique symbol type works, and maybe thats the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think you're right. I just was trying to hide any public API from this. I can just tack on a string property or something though.
| /** | ||
| * Get the secret key for this keypair, with the value redacted so that it cannot be logged. | ||
| */ | ||
| abstract getSecretKeyRedacted(): Redacted<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can just be a concrete implementation right?
Description
Taking inspiration from how Effect handles Redacted values, this introduces our own
redactedfunction, which creates a value that is stored via a weakmap on a global registry. This has some neat properties:I originally wanted this to be exposed in a create-only way, so that outside of the package you couldn't read reacted values, but I realized that we probably want methods like
getSecretKeyto return redacted values by default.While I was here, I realized we were using fake private for the data in keypairs, which kind of scares the shit out of me because if you log a keypair (which IMO seems like an operation that should be plenty safe) it means that all of the secret values are exposed:

I made this a true private which shouldn't be a breaking change unless people are depending on undocumented properties. I thought about changing this to store as a redacted internally, but just using true privates felt like a better solution here.
Test plan
I will test it + update docs before merging.