Maybe we should add this to the style guide but can you keep the statics at the bottom, and preserve the previous import order?

final

final

should 0 be TLSEXT_STATUSTYPE_host_name?

SSL.setStatusType protects against null SSL* too ... can we just rely on the null there instead of doing it in both places?

should we instead unwrap and verify the NPE?

can we run this test with OPENSSL_REFCNT too? We need to be careful about releasing in this case of course.

consider specifying the timeout values here so the unit tests don't have to wait for the quite period etc...

can we just do assertTrue?

this is expected to be called in the junit thread right, if so can we just do assertTrue?

This test and testServerException produce the NPE described in #6336.

OpenSSL calls the verify() method (via the OCSP callback - see PR in netty-tcnative) and this test throws purposely an IllegalStateException to see what happens.

I'm expecting a DecoderException to bubble up (with the IllegalStateException as cause) but instead I'm getting DecoderException that was caused by a NPE.

see netty/netty-tcnative#215 ... does this account for checking of the runtime version which may be dynamically linked?

is it possible to add the commands required to generate this to https://github.com/netty/netty/blob/4.1/handler/src/test/resources/io/netty/handler/ssl/generate-certs.sh ?

That cert chain is actually not generated. It's the real cert chain for https://netty.io and it was simply pulled with Chrome's export feature.

add a comment to explain null is allowed and that it will free the existing callback if one exists.

nit: you mix 2 and 4 space indentation here ... can you make this consistent?

nit: package private?

consider adding a descriptive string: e.g socket was closed before response was received.

consider adding a descriptive error string for the exception

an one-time -> a one-time

also aren't you randomly generating the nonce on each build?

Yes, a new nonce is created for each build(). The comment is referring to the built object itself because it's tempting to cache that OCSPReq object.

make a define for this value?

😮

Consider adding a comment // TODO: use Netty 😉

can you link to the RFC or area where this ID is define? these magic IDs have a tendency to spread around without any reference to where they came from.

Can you clarify why having a self signed certificate is prohibitive and why it wouldn't be possible for the server to simulate querying the CA for the staple ... and just generate it on demand (via bc, openssl, etc...)?

there is a race condition here where callback may be used after freeOcspCallback is called on the callback. Consider the following:

• adding a constraint (via javadoc APIs) on OcspCallbackExt that this use case must be supported (no negative behavior will result from calling callback.context() or using the resulting object after freeOcspCallback is called).
• adding a warning on this method that unexpected results may occur if the underlying object is freed.
• remove this method (doesn't look like its used currently).

IMO it would be more clear if the 0 value came from tcnative instead of implicitly defined at the Netty level.

Thinking about it more TlsExtStatusType is not very descriptive and I'm not sure if it would require state in the future to use if different options are supported through this interface. I get why BoringSSL decided to go with SSL_enable_ocsp_stapling ... can we just use this API naming convention in tcnative instead of OpenSSL's? @normanmaurer - WDYT?

Ack. Happy to change it into enableOcsp() as there is no use-case for the toggle functionality.

consider saving a reference to this exception ... just to ensure the same value is used in the comparison and string printout.

(general comment for this test)

I think we should revert this back to a plain JDK implementation (i.e. HttpURLConnection) and provide it just a short and simple example how to interact with the CA's responder as there's not much information about it on the Internet (or rather it's difficult to formulate the query as OCSP is foreign to many ppl.)

Once you know that it's a HTTP POST, with special content-type and accept headers and a special payload it's "trivial" to deduct a Netty client from it.

In general, in order to operate OCSP you need to build some infrastructure that goes beyond the Netty project. You need caches, an OCSP responder client (i.e. something like this class) and a strategy to manage and refresh all these staples that are only valid for < 10 days.

I am fine with that for the sake of an example, and assuming we can't or its too much hassle to implement a more self contained example #6158 (comment). My previous comment on this was meant to literally add something like // TODO: we use HttpURLConnection for the sake of demonstration, but you should consider using Netty's HTTP client instead.

update this comment.

Hum this relationship is unnatural. Typically the SSLContext provides the default configuration for creating an SSLEngine and the same relationship olds for SSL_CTX and SSL, and the SSL/SSLEngine never impacts the state of the SSL_CTX/SSLContext. Could we just instead have tcnative throw if the SSL_CTX has not been setup correctly? For example we could add a bool oscp_enabled to tcn_ssl_ctxt_t (in ssl_private.h). SSLContext#enableOcsp would set this state (even for BoringSSL) and then SSL#enableOcsp would check this state has been set. You can get the tcn_ssl_ctxt_t from the SSL* via SSL_get_app_data2.

Agree. Next PR fixes it. I've reduced it to:

SSLContext#enableOcsp
SSL#setOcspResponse
SSL#getOcspResponse

The SSL#enableOcsp call doesn't need to be exposed to the user.

nit: make a tertiary statement.

can this just be removed before the verification?

nit: remove empty line

nit: remove empty line

8192

@rkapsi is this good enough and also hold true for Libressl and Boringssl ?

TBH I don't know. I'm operating under the assumption they're forks of OpenSSL and OCSP should be present as of 1.0.0 and we have only tested 1.0.2+.

maybe try to run the test while using "netty-tcnative-boringssl-static" and see what happens :)

OK, LibreSSL works.

BoringSSL doesn't work (fails gracefully with my OCSP is not supported exception) but that is because... It's either getting built with OPENSSL_NO_OCSP or OPENSSL_VERSION_NUMBER is something very different than expected. Checking... 🤔

Confirm, BoringSSL gets built with OPENSSL_NO_OCSP.

Thanks for confirming @davidben.

@nmittler - We are working with @rkapsi to support both BoringSSL and OpenSSL (and LibreSSL should come for "free"). We are currently using both APIs to make this happen and it is just a matter of working out the kinks to make sure we can make the two different APIs (BoringSSL and OpenSSL) work consistently with one API (tcnative/netty) on top. I think we are very close to pulling this off (thanks to @rkapsi !)

In regards to the machinery: That is totally cool that BoringSSL doesn't support the heavy machinery because we don't use any of OpenSSL's either. All we want to do is blindly pass along byte strings. It's literally just that and BoringSSL's API does just that. Again, totally cool. Unfortunately it's defaulting to OPENSSL_NO_OCSP and I guess we're confused how to proceed. 🤔

I'm personally cool not supporting BoringSSL but that means I'll never be able to use it either because our lovely non-profit CA will not appreciate it being DDoS'd if our servers do handshakes without staples.

OPENSSL_NO_OCSP means we do not support OpenSSL's OCSP API. If we did not define it, loads of projects would fail to build because they, naturally, do not use BoringSSL's API. Like with any other project, your options are not to support OCSP stapling with BoringSSL (in which case OPENSSL_NO_OCSP, what you would have been checking anyway, Just Works and you need no additional work) or to add BoringSSL-specific code that does not look at OPENSSL_NO_OCSP. BoringSSL-specific code is obviously needed anyway for BoringSSL-specific API, so I don't see why having to ignore a preprocessor symbol which happens to have a similar name as the feature matters.

In case this is where the confusion comes from, BoringSSL's APIs has no relation to OPENSSL_NO_OCSP. They are available regardless.

We do not, in general, have individual toggles for all our features like OpenSSL does. Instead, we define a set of OPENSSL_NO_* values based on what makes portability simplest. These symbols do not affect what features BoringSSL exposes.

Agreed I think we have the tools we need and everything is ok :)

@rkapsi we just need to adjust the macros see netty/netty-tcnative#215 (comment) ... I will add more details here.

protected as the class is abstract

Making it protected would prevent ad-hoc instantiation and force the user to declare a class which can be a nuisance for prototyping/quick hackery.

// This won't be possible with a protected ctor
pipeline.addLast(new OcspClientHandler(engine) {
  @Override
  public boolean verify(...) {
    return true;
  }
});

@rkapsi - are you sure? I don't see why this would cause an issue, can you explain? OcspClientHandler is an anonymous type that should have access to protected things in the super class.

Operator error, disregard me.

consider using a do {} while(...) loop as it should not be called with a null Throwable

just return directly

2.0.1.Final-SNAPSHOT now :)

should we treat this as an error if called while in the wrong mode?

sgtm

@rkapsi - are you sure? I don't see why this would cause an issue, can you explain? OcspClientHandler is an anonymous type that should have access to protected things in the super class.

consider testing with both providers (OPENSSL_REFCNT) like other tests.