Add OCSP nonce functionality

[scolby33]

Will close #1045.

I intend to add as well:

• test OCSP request creation, with and without explicit nonce
• test OCSP response creation with and without explicit nonce
• test OCSP response creation, copying nonce from a request
• test OCSP nonce comparison
• some basic documentation for my changes

In the meantime, feedback on what I have written is welcome.

[sfackler]

This should take &OcspRequestRef, not OcspRequestRef.

[sfackler]

This function will need a custom return type, looking at the documentation.

[scolby33]

I've implemented this custom return. I don't love what I've done, especially how for the _ case I had to bury the ErrorStack down in my new enum. Please let me know what you think.

[scolby33]

Ought I derive Copy, Clone, PartialEq, Eq as well?

[scolby33]

Same as above: should Copy, Clone, PartialEq, Eq be added?

[scolby33]

I've pushed two commits based on your comments, @sfackler. I will be able to get to the remaining testing and adding documentation late on Monday. Thanks for helping me with this implementation so far!

[sfackler]

I'd have this be a bit simpler:

pub fn check_nonce(req: &OcspRequestRef, bs: &OcspBasicResponseRef) -> NonceCheck { ... }

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct NonceCheck(c_int);

impl NonceCheck {
    pub const REQUEST_ONLY: NonceCheck = NonceCheck(-1);
    pub const UNEQUAL: NonceCheck = NonceCheck(0);
    pub const EQUAL: NonceCheck = NonceCheck(1);
    pub const ABSENT: NonceCheck = NonceCheck(2);
    pub const RESPONSE_ONLY: NonceCheck = NonceCheck(3);
}

[scolby33]

I agree that my version is complicated, but it has the benefit of making the user handle the error case; I believe that failing to if (thing_that_returns_nonzero_on_error()) { goto uh_oh; } is a relatively common source of security bugs. In my version, the compiler will warn about an unused result or if the user chose to use unwrap()/? it'll panic/propagate if proper error handling isn't done.

I'm happy to use your version if you disagree, but I think my version gives a more ergonomic usage for the common case.

(Warning: pseudocode)

fn perform_ocsp_request(...) -> ... {
    let my_req = ocsp::OcspRequest::new().unwrap();
    let res = send_request(&my_req);

    ocsp::check_nonce(&my_req, &res)?
}

Versus

fn perform_ocsp_request(...) -> ... {
    let my_req = ocsp::OcspRequest::new().unwrap();
    let res = send_request(&my_req);

    match ocsp::check_nonce(&my_req, &res) {
        ocsp::NonceCheck::REQUEST_ONLY => Err(()),
        ocsp::NonceCheck::UNEQUAL => Err(()),
        ocsp::NonceCheck::EQUAL => Ok(()),
        ...
    }
}

It only moves the work of handling the error up a level, but at least it's impossible to avoid handling it somewhere.

Looking forward to hearing your thoughts on this.

[sfackler]

How unambiguous is the error/ok decision in this context?

We can always tag the return type here #[must_use] just like Result.

[scolby33]

That's a good point. The OpenSSL docs make a reasonable case that at least a 0 return value should be an unambiguous error signal:

The return values of OCSP_check_nonce() can be checked to cover each case. A positive return value effectively indicates success: nonces are both present and match, both absent or present in the response only. A non-zero return additionally covers the case where the nonce is present in the request only: this will happen if the responder doesn't support nonces. A zero return value indicates present and mismatched nonces: this should be treated as an error condition.

https://www.openssl.org/docs/man1.1.0/man3/OCSP_check_nonce.html

I like the idea of using #[must_use] for this at least, but I could see an argument that -1 at least shouldn't exactly be an error. Maybe a combination of our approaches like

pub fn check_nonce(req: &OcspRequestRef, bs: &OcspBasicResponseRef) -> Result<NonceCheck, SomeErrorNotSureWhat> { ... }

impl NonceCheck {
    pub const REQUEST_ONLY: NonceCheck = NonceCheck(-1);
    pub const EQUAL: NonceCheck = NonceCheck(1);
    pub const ABSENT: NonceCheck = NonceCheck(2);
    pub const RESPONSE_ONLY: NonceCheck = NonceCheck(3);
}

[scolby33]

Sorry for letting this sit for so long. I'm going to do some work on the testing parts of this implementation while we work out the best return type for the OCSP_check_nonce() function.

[scolby33]

Actually, for the moment, I think I'm going to set this aside again.