der: Error stacktraces

[dishmaker]

formats/der/src/decode.rs

Lines 26 to 28 in bd84063

	pub trait Decode<'a>: Sized + 'a {
	/// Type returned in the event of a decoding error.
	type Error: From<Error> + 'static;

I think associated Error would be useful inside Reader trait. ¹

Motivation

To prove usefulness, suppose we want to return annotated errors with .field stacktrace.

Example of such stacktrace from invalid PKCS#15:

Error: from_der_type CIOChoice

Caused by:
   0: .choice AuthObjects
   0: .choice Objects
   0: [0]
   0: (AuthenticationObjectChoice)
   0: .choice Pwd
   0: .type_attributes
   0: .pwd_reference

   1: incorrect length for INTEGER at DER byte 57

Because some ASN.1 DER definitions are really complicated, such error stacktrace is very helpful.
Field names in returned error are possible, because derive Sequence may generate .wrap_err_field("fieldname") calls.

Why not use Decode::Error?

Such errors may originate from x509-cert, as it is a part of PKCS#15:

CertificateChoice ::= CHOICE {
    x509Certificate CertificateObject {X509CertificateAttributes},
    -- ...
    spkiCertificate [1] CertificateObject {SPKICertificateAttributes},

Moreover, existing crates using der will benefit from such Decode error stacktrace feature, similar to my clarify Encode feature.

Footnotes

1. I mentioned that some time ago https://github.com/RustCrypto/formats/issues/1053#issuecomment-2393628486 ↩

[tarcieri]

As was mentioned in #1053, having an associated Error type on Decode/DecodeValue makes it possible to return format-specific errors. These can be well-typed e.g. enums which carry structured information about what went wrong, defined by downstream consumers of der in their own crates, as Decode/DecodeValue are actually traits impl'd by downstream consumers.

You are suggesting moving it to Reader, but Reader has an impl cardinality of 2 (SliceReader and PemReader) and both of those impls are in the der crate itself.

What you're describing doesn't sound like you actually want to leverage an associated error type? Why would it be different for SliceReader versus PemReader? Wouldn't they both use der::Error and at that point you don't need a separate associated type?

As an alternative to the well-typed errors an associated Error type enables, you are suggesting a stringly-typed field name (a 'static str I guess?) The two seem orthogonal to me: you can add a wrap_err_field method to der::Error and still keep the associated Error type. So it's a false dichotomy.

But a field name still provides far less information and flexibility than a proper format-specific error type. Enum variants can be used to relay more specific details about why processing of a particular field went wrong, e.g. perhaps it's trying to interpret an OCTET STRING as a particular cryptographic key, but the string is of the wrong length, or an OID identifies an unsupported algorithm. These are failures that can quite clearly be relayed using format-specific error types.

I'll also note format-specific error types have been requested by multiple people. See #1492 as one example.

[dishmaker]

but Reader has an impl cardinality of 2 (SliceReader and PemReader)

What you're describing doesn't sound like you actually want to leverage an associated error type? Why would it be different for SliceReader versus PemReader?

I use my own wrapper of SliceReader with error stacktraces.

/// [`SliceReader`] which tracks struct names and field names, where the error occured.
#[derive(Clone, Debug)]
pub struct SliceDebugReader<'a> {
    reader: SliceReader<'a>,
}
impl<'a> Reader<'a> for SliceDebugReader<'a> {
    type Error = StackTracingError;
}

you can add a wrap_err_field method to der::Error and still keep the associated Error type.

It would carry Vec<String> and be suboptimal.

use thiserror::Error;


#[derive(Error, Debug)]
pub enum StackTracingError {
    #[error(transparent)]
    Io(#[from] std::io::Error),

    #[error("{stack}")]
    Der {
        stack: Vec<String>,

        #[source]
        der: der::DerError,
    },
}

[tarcieri]

I use my own wrapper or SliceReader with error stacktraces.

That's not a justification for defining a custom error type on readers. It sounds like you just want more functionality out of the existing readers.

Are you expecting everyone to define their own bespoke SliceReader-like Reader types?

It would carry Vec and be suboptimal.

Please be more specific. What solution are you suggesting that doesn't involve a Vec<String> to report on nested fields and why does it need a custom error type per reader?

(I had already suggested 'static str since fields names are static, as it were, but you would still need a Vec to report on a series of nested field names)

[dishmaker]

Okay, I was too focused on a single solution and need to think that through.