Revise errors for CertificateSigningRequestParams::from_der

[dwhjames]

• Introduce specific error for CSR signature verification
• Make error name more specific for unsupported CSR extensions

---

Following up from

• Expand rustdoc for CertificateSigningRequestParams::from_der #386

[djc]

It would be nice to better understand the motivation/use case here.

[dwhjames]

It would be nice to better understand the motivation/use case here.

@djc, I’m assuming the original introduction of Error::RingUnspecified is for

rcgen/rcgen/src/key_pair.rs

Lines 651 to 656 in 453bcb5

	#[cfg(feature = "crypto")]
	impl<T> ExternalError<T> for Result<T, ring_error::Unspecified> {
	fn _err(self) -> Result<T, Error> {
	self.map_err(|_| Error::RingUnspecified)
	}
	}

where a catch-all error case of the underlying crypto library is lifted into the rcgen error enumeration.

And later it was re-used in this case

• 116df85#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R508

It would be clearer in code that uses this library to have error variant names that map in a self-evident way to the expected error conditions expected in parsing and verifying a CSR.

Some, but not all, existing error variants include contextual hints in the name (…InCsr or …CertificationRequest are existing examples).

[djc]

It would be nice to better understand the motivation/use case here.

You still haven't really answered this question. What problem did you run into that you are trying to solve?

[dwhjames]

It would be clearer in code that uses this library to have error variant names that map in a self-evident way to the expected error conditions expected in parsing and verifying a CSR.

Some, but not all, existing error variants include contextual hints in the name (…InCsr or …CertificationRequest are existing examples).

match CertificateSigningRequestParams::from_der(csr_der) {
    Ok(csr_params) => …,
    Err(rcgen::Error::RingUnspecified) => …,
    Err(rcgen::Error::UnsupportedExtension) => …,
    Err(_) => …,
}

1. Would you agree that even in the immediate context of CertificateSigningRequestParams::from_der, RingUnspecified is not an intuitive name for the error condition that caused its return?
2. Would you agree that if the error handling for UnsupportedExtension is not immediately in this scope, but an enclosing scope, it would not be obvious what error condition is being handled?

[djc]

Submitted an alternative in #390.

[dwhjames]

It sounds like you agree on 1? Do you also agree on 2? Are you open to a more specific name for the rejection of an extension in a CSR?

[djc]

It sounds like you agree on 1? Do you also agree on 2? Are you open to a more specific name for the rejection of an extension in a CSR?

I'm on the fence because I think the documentation for the variant and error string (Display representation) are pretty clear that it's about the CSR -- therefore I'm not sure changing this to a different name (in a semver-compatible release, deprecating the old variant) is worth it. @cpu thoughts?

[dwhjames]

I'm on the fence because I think the documentation for the variant and error string (Display representation) are pretty clear that it's about the CSR

Certainly those parts are, but that doesn’t immediately help clarity in source code.

match something_that_interally_calls_from_der(…) {
    Ok(…) => …,
    Err(rcgen::Error::UnsupportedExtension) => …,
    Err(_) => …,
}

My contention, from above, is that if the error handling for UnsupportedExtension is not immediately in the lexical scope of invoking from_der, but instead an enclosing scope, it would not be obvious what error condition is being handled.

The current recourse is to add comments asking the reader to jump to documentation on the error value or the from_der function.

[cpu]

therefore I'm not sure changing this to a different name (in a semver-compatible release, deprecating the old variant) is worth it. @cpu thoughts?

I don't feel strongly but agree that duplicating the error types under a new name + deprecating the old ones doesn't feel like it's worthwhile in this case. Maybe better to file an issue tagged with a label to just outright fix the name in the next semver breaking release.