Node auth error messages - take 3 #397
Conversation
go-cosmwasm/src/error/rust.rs
Outdated
| @@ -35,6 +35,12 @@ pub enum Error { | |||
| #[cfg(feature = "backtraces")] | |||
| backtrace: snafu::Backtrace, | |||
| }, | |||
| #[snafu(display("Error calling the enclave: {}", msg))] | |||
There was a problem hiding this comment.
This isn't an error calling the enclave specifically. Also if this is going to be printed to the log we can probably just print "{}", msg since it will be an informative error
There was a problem hiding this comment.
Also this is the 3rd variant of the name EnclaveErr in the project. Maybe name this something else? (sgx-vm EnclaveErr and EnclaveError in ffi/types)
There was a problem hiding this comment.
Ok, so just writing is as "{}", msg is probably alright.
I was debating creating another variant SdkError for cases of sgx_status_t like i did in VmError, for which this message is accurate, and then change the message on this variant to something like "Error returned from the enclave", but i felt that might be excessive.
As for the naming, the name shows up in a lot of contexts, but that's because different contexts use different error types, and all of them needed a new variant for errors that come from the enclave. I agree that the naming duplication isn't great, but for each of those contexts separately i felt like that was the most appropriate name.
In any case, if you mix them up, the compiler will let you know.
There was a problem hiding this comment.
So why not call it GoEnclaveErr or something? Duplicate names will cause all sorts of fun when trying to use find and replace, or find in path (not to mention the potential Expected EnclaveErr but got EnclaveErr good times)
There was a problem hiding this comment.
It's not even in Go yet 😅
But we can name it GoCwEnclaveError or something to signal the layer it's in.
| /// cbindgen:prefix-with-name | ||
| #[repr(C)] | ||
| #[derive(Debug, Display, PartialEq, Eq)] | ||
| pub enum NodeAuthResult { |
There was a problem hiding this comment.
Do you think we could add errors for specific isvEnclaveQuoteStatus? Like, return "failed to perform registration. Invalid quote status "KEY_REVOKED" " or something
There was a problem hiding this comment.
hmm... can you point me to the place in the code where this error originates? I'd probably have to fiddle with 1-3 functions to bubble this specific case out to the context where i instantiate variants of this type.
Note that the way i use this type is really high level, and unrelated to how most of the related code just uses sgx_status_t
There was a problem hiding this comment.
If it's not something you can do in 15 minutes or less we can leave it for some other time
There was a problem hiding this comment.
Yeah, this will be somewhat involved.
I'm not sure I understand what exactly all the structs and enums mean, and the specific error you're referring to happens a couple layers into functions that obfuscate the specific errors that happen. If I change that, I'd need to add a couple of error types, and modify a bunch of other functions that use the same functions that I needed to change in the first place.
…ssages-3-electric-manatee
This PR makes errors in node authentication display more useful errors for users that encounter them.
closes #376
To reviewer: The technique shown here to generate nicer error messages can and should be applied to
EnclaveError.