feat: improve hidden data handling

[AaronFeickert]

Some data, like cryptographic key material or passwords, needs special handling. Such data typically should:

• be zeroized when it goes out of scope
• never be displayed or written to debug logs
• not typically be accessed except by (mutable) reference
• have a strictly enforced type to avoid misuse

This PR updates the Hidden type to handle this kind of data in a generic way. Hidden types can be instantiated using any underlying type that implements Zeroize and an optional differentiated type created by a macro that enforces context and prevents misuse.

The hidden data is stored on the heap in a Box wrapper. This allows us to safely zeroize it on drop, and automatically prevents display and debug from revealing it unintentionally. Further, we only expose access to the data via immutable and mutable reference. The implementation supports Clone for cases where we need it, but does not support Copy.

We also update SafePassword to be an instantiation of the updated Hidden type. This refactor is transparent to callers, as its existing API is unchanged.

[jorgeantonio21]

Pretty cool !

LGTM

[CjS77]

I'm not convinced this is the right approach.

There's some debate going on in another fork of this repo on the same topic that I'll copy over here, when I have a spare minute.

We'll get there though.

[AaronFeickert]

Significantly updated. The latest commit instead updates Hidden to add generic type support, optional type enforcement, and zeroizing on drop. It also refactors SafePassword to use this, but in a way that maintains its API.

[brianp]

Something about the macro feels like an anti-pattern to me. From a usability perspective to have the marker trait applied I would want to throw it into derive. The use of hidden_label!() is offputting for a reason I can't quite put my finger on.

[AaronFeickert]

I'm not a huge fan of the approach, and I wish there were a better way to do what we want (enforced type differentiation) in Rust without resorting to replicating boilerplate code in a way that's likely to be done wrong. For what it's worth, @jorgeantonio21 suggested replacing the whole macro construction with a const str in the generic type definition, but Rust doesn't support it. I'm definitely open to a different approach if we can find one that works.

Possibly worth noting that a similar approach is used in both the new hashing and Schnorr signature APIs to enforce type differentiation and enable certain hash operations to do what we want.

[jorgeantonio21]

Perhaps add an example to docs. Something like

Suggested change

	/// Data that needs to be kept hidden, needs to be zeroized when it goes away, and is resistant to misuse
	/// Data that needs to be kept hidden, needs to be zeroized when it goes away, and is resistant to misuse.
	///
	/// The following will throw a compile error:
	///
	/// hidden_label!(MyFirstLabel);
	/// hidden_label!(MySecondLabel);
	///
	/// type HiddenType1 = Hidden<[u8; 3], MyFirstLabel>;
	/// type HiddenType2 = Hidden<[u8; 3], MySecondLabel>;
	///
	/// fn accept_only_hidden_type_2(hidden: HiddenType2) {
	/// println!("My dear hidden type 2");
	/// }
	///
	/// let hidden_1 = HiddenType1::hide([0u8, 1, 2]);
	/// accept_only_hidden_type_2(hidden_1);

[jorgeantonio21]

I am pretty satisfied with the current state. I left a few minor comments.

@brianp pointed out that the use of the hidden_label macro is not clear for a developer without the right context, there might be a way to satisfy our goal of type enforcement in a simpler way.

Nonetheless, I consider having type enforcement is a very important feature to have.

[jorgeantonio21]

Everything we discussed was addressed, I am in favor of it.

LGTM

[AaronFeickert]

Can someone enable CI workflows, so we can check for any problems there?

[jorgeantonio21]

LGTM :)

[AaronFeickert]

Updated to address some implementation concerns that primarily concern derived traits and trait bounds:

• Documentation is improved.
• Hidden now derives only Clone, Serialize, and Deserialize, and no longer has a Default trait bound on its underlying type.
• The hidden_type! macro generates a struct deriving only Clone, Debug, and Zeroize, and no longer has a Zeroize trait bound on its underlying type (it's enforced in Hidden).
• SafePassword now only derives Clone, Debug, Serialize, and Deserialize.

This means we can now use the macro with types that don't support serialization or defaults, which can include even basic types like [u8; 64] that we may need to use for derived keys. If we need serialization on such types, we can define them separately and derive the required traits.

It also means that the SafePassword API changes in a way that affects its tari use. Namely, that crate performs equality testing on SafePassword instances directly, which is probably bad form. This can be trivially changed (in only two locations) to testing on reveal() references, which seems much more idiomatic.

[CjS77]

Looks good. A few nits, and I suggest we remove serialisation support

[jorgeantonio21]

Would be good to have a description on why this is safe, given the way Box is treated by the compiler when dereferenced. Same for reveal.

[AaronFeickert]

From the analysis @CjS77 did on the compiled version, it's unclear precisely if/where this can go wrong. My understanding was that asterisk dereferencing of a Box<T> can perform a move, but need not do so in all cases. One apparent way to avoid this may be to replace any asterisk dereferencing with explicit calls to <Box<T> as Deref>::deref instead, but I have not tested this (and I'm not sure if the wrapped reference calls we use avoid this problem regardless).

[AaronFeickert]

All tari tests pass using this version of tari_utilities locally.

[AaronFeickert]

To mitigate dereference woes, added a new SafeArray<T, const N: usize> type that acts like an array by implementing AsRef<[T]> and AsMut<[T]>, but does not implement Copy.

Since it's intended for keys, also implements Default and ConstantTimeEq, and overrides Eq to be a constant-time operation too.

You'd use it by defining a hidden type in this manner: hidden_type!(CipherKey, SafeArray<u8, 32>)

Then you can use as_ref() or as_mut() to access it like an array reference, but it can't be dereferenced.

Comments welcome on this idea.

[CjS77]

I think this is as good as we can make it for now.

It would be interesting to see some performance benchmarks for secret key and ECC operations based on this (Vec<u8>) vs [u8; 32]

One day when #[sensitive] attributes can be passed to LLVM (something that is being discussed but not on any roadmap afaict), we can come and revisit this approach

[jorgeantonio21]

LGTM, I left a minor comment. Let's also run cargo fmt.

[AaronFeickert]

It would be interesting to see some performance benchmarks for secret key and ECC operations based on this (Vec<u8>) vs [u8; 32]

I wouldn't use this for elliptic curve SecretKey operations, as I'm sure that's an entirely different can of worms to open. But for hash-based derived keys, I would be surprised if it's much of a practical difference since those are used more sparingly (and the hash function computation time will surely dominate). Fortunately it would be easy to change the guts of SafeArray in a non-breaking way later if desired.

[AaronFeickert]

LGTM, I left a minor comment. Let's also run cargo fmt.

Yep, will run a few more proof-of-concept tests locally to ensure this will meet our needs, and then format to make the linter happy.

[AaronFeickert]

Updated to implement Deref and DerefMut with Target = [T], which makes it easier to use with the hashing API.

Would appreciate if someone can verify that you still can't dereference the underlying data. I ran into sizing errors while trying to do this.

Also would like comment on whether the SafeArray abstraction is too much effort. Presumably you can still dereference individual elements with an implicit copy happening? If it's just another abstraction that ends up causing more work for implementers while not sufficiently reducing risk, we could nix it.