Panic in native decryption

[cole-miller]

Hi, thanks so much for the library!

At Zed we've seen panics in production originating from this unwrap, via Item::secret:

oo7/client/src/crypto/native.rs

Line 47 in 1bba4ad

.unwrap()

Apparently the encrypted secret read from DBus is sometimes found to be corrupt. To avoid crashing the app in this case we're currently wrapping Item::secret in catch_unwind (zed-industries/zed#21617 )---would you be open to updating oo7 to return an error in this situation instead? I'd be happy to put together a PR if so.

[A6GibKm]

If there are valid cases where the data is corrupt then it makes sense to return a Result, however I would like to understand first why they are failing in the first place.

[bilelmoussaoui]

Indeed. It would be nice to figure out what is the root of the issue as i havent seen it before given all the other apps using the library.

So I would like to know if the app is sandboxed or not, if it is using the dbus backend, what is the server implementation.

I think oo7 should provide enough logs using tracing, so might be worth digging into that and possibly adding more logs if needed.

[cole-miller]

Thanks for the feedback. I agree it would be great to know more about what's going on here; I'll look into adding a bit of telemetry to gather more information and let you know if I find anything. Right now I don't think our panic reports contain enough information to know which oo7 backend is in use, etc.

I do think that from a design standpoint returning an error instead of panicking is the correct choice here. The Secret is loaded from an external source (DBus or the filesystem) that oo7 doesn't entirely control, so finding that the raw loaded data is not valid according to the PKCS7 padding rules doesn't (necessarily) indicate that oo7 has a bug, and doesn't call for crashing the application to avoid operating in an incorrect state. With regard to gathering information about the root cause, that won't be any harder (for Zed at least) if the panic is replaced by an error return.

[A6GibKm]

The Secret is loaded from an external source (DBus or the filesystem)

Do you know who stored such secret in the filesystem?

[bilelmoussaoui]

I opened a PR in #172 that would avoid us from panicking at runtime which is not great. Although, i won't consider it as a fix for this issue as we really need to understand why.