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

Implement DebugSecret for vectors of debuggable types #458

Closed
ionut-arm opened this issue Jun 30, 2020 · 5 comments
Closed

Implement DebugSecret for vectors of debuggable types #458

ionut-arm opened this issue Jun 30, 2020 · 5 comments

Comments

@ionut-arm
Copy link

Hi,

Currently DebugSecret is implemented only for Vec<S>, where S must have DebugSecret implemented on it as well (link) - however only arrays of types with Debug have that trait, so only Vec<[T]> will get DebugSecret implemented.

I was wondering if it's possible to have DebugSecret implemented on Vec<S: Debug> as well/instead.

@tony-iqlusion
Copy link
Member

tony-iqlusion commented Jul 2, 2020

Thanks for reporting this.

For context: one of the goals of this crate (which could definitely be better documented) is preventing accidental secret leakage through mechanisms such as Debug logging.

To enforce that concern, DebugSecret::debug_secret presently returns a &static str.

I think you're right that having DebugSecret contingent on Vec<S> where S: DebugSecret is not ideal. I think instead of asking the downstream type to opt into DebugSecret, instead the type signature for DebugSecret::debug_secret could be changed to use const fn, which would uphold the same invariant of no data-dependent debug logging, but in a way that's more flexible than a &'static str.

By leveraging const fn, a DebugSecret impl on Vec<S> could incorporate the core::any::type_name of S.

Is that what you'd ultimately like for debugging information? If so, it seems possible.

@ionut-arm
Copy link
Author

That definitely sounds like a sensible solution that would solve our problem.

Is that what you'd ultimately like for debugging information? If so, it seems possible.

Actually, it's not really debugging that I'm interested in, but serialization. However, Serialize and Deserialize are tied to DebugSecret, and thus my request :) I should've mentioned that previously.

We're trying to protect a secret while working with it in memory, but to also be able to serialize it at the right point. Don't know if that would be better achieved through other means, like exposing the contents of the secret and then serializing, but it feels more error-prone.

@tony-iqlusion
Copy link
Member

You might take a look at the docs for SerializableSecret:

https://docs.rs/secrecy/0.6.0/secrecy/trait.SerializableSecret.html

...which, upon further inspection, does not appear to be properly used in the bounds for the Serialize impl on Secret (!)

To prevent accidental exfiltration of secrets, SerializableSecret was supposed to act as a marker trait, and one deliberately not impl'd for SecretVec.

The alternatives are:

  1. Make your own newtype that wraps Vec<_> (of whatever type you're interested in)
  2. Use serde's serialize_with as noted in the SerializableSecret docs.

All that said, it seems this crate could use a little work and documentation improvements. Hopefully I'll have time to make a pass over it in the next few days to clear some of that up and fix the issues brought up here.

@ionut-arm
Copy link
Author

Thanks for pointing that out! For some reason I was completely blind to the existence of SerializableSecret in the crate...

I'll probably go down the route of using serialize_with, seems more idiomatic.

tarcieri pushed a commit that referenced this issue Jul 8, 2020
NOTE: addresses #458

The `SerializableSecret` trait was added in #262, however the
`Serialize` (as well as `Deserialize`) impls were (unintentionally)
bounded on `DebugSecret`.

This commit removes the `DebugSecret` bound on the `Deserialize` impl,
adds the intended `SerializableSecret` on the `Serialize` impl, and
improves the documentation for the `SerializableSecret` trait.
tony-iqlusion added a commit that referenced this issue Jul 8, 2020
NOTE: addresses #458

The `SerializableSecret` trait was added in #262, however the
`Serialize` (as well as `Deserialize`) impls were (unintentionally)
bounded on `DebugSecret`.

This commit removes the `DebugSecret` bound on the `Deserialize` impl,
adds the intended `SerializableSecret` on the `Serialize` impl, and
improves the documentation for the `SerializableSecret` trait.
@tony-iqlusion
Copy link
Member

#463 should address the issues with the trait bounds: serde serialization is no longer bounded by DebugSecret.

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

No branches or pull requests

2 participants