For proxy #218
For proxy #218
Conversation
bssl-compat/CMakeLists.txt
Outdated
| SET (CMAKE_CXX_COMPILER "clang++") | ||
| # dcillera - commented out as they're declared in function "cmake" of Bazel BUILD file | ||
| # SET (CMAKE_C_COMPILER "clang") | ||
| # SET (CMAKE_CXX_COMPILER "clang++") |
There was a problem hiding this comment.
Just delete these lines rather than commenting out
bssl-compat/CMakeLists.txt
Outdated
| @@ -118,6 +119,7 @@ add_library(bssl-compat STATIC | |||
| source/RSA_decrypt.cc | |||
| source/RSA_encrypt.cc | |||
| source/RSA_generate_key_ex.cc | |||
| source/RSA_padding_add_PKCS1_PSS_mgf1.cc | |||
There was a problem hiding this comment.
You added this extra function to support the cryptomb and qat extensions, but you subsequently disabled those extensions (because private key method providers are not supported yet). Therefore, you should remove this function.
| @@ -24,6 +24,7 @@ uncomment.sh "$1" --comment -h \ | |||
| --uncomment-func-decl RSA_add_pkcs1_prefix \ | |||
| --uncomment-func-decl RSA_public_key_from_bytes \ | |||
| --uncomment-func-decl RSA_private_key_from_bytes \ | |||
| --uncomment-func-decl RSA_padding_add_PKCS1_PSS_mgf1 \ | |||
There was a problem hiding this comment.
Remove RSA_padding_add_PKCS1_PSS_mgf1
There was a problem hiding this comment.
Remove RSA_padding_add_PKCS1_PSS_mgf1
| @@ -182,7 +182,9 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c | |||
| // even request client certs. So, instead, we should configure a callback to skip | |||
| // validation and always supply the callback to boring SSL. | |||
| SSL_CTX_set_custom_verify(ctx, verify_mode, customVerifyCallback); | |||
| #ifdef ENABLE_REVERIFY_ENFORCE_RSA // Disabled as not implememnted in the bSSL layer | |||
There was a problem hiding this comment.
Simply use #if 0 with a comment to explain.
The ENABLE_REVERIFY_ENFORCE_RSA guard name is confusing. It conflates the "reverify" from SSL_CTX_set_reverify_on_resume() function, and the "enforce rsa" from SSL_set_enforce_rsa_key_usage() & SSL_was_key_usage_invalid() functions.
If you really want to use a guard macro then maybe something more "mechanical" like BSSL_COMPAT_HAVE_<function> would be more appropriate i.e. BSSL_COMPAT_HAVE_SSL_CTX_set_reverify_on_resume in this case.
bssl-compat/source/bio_meth_map.cpp
Outdated
| @@ -102,7 +102,10 @@ static ossl_BIO_METHOD *bio_method_new(const BIO_METHOD *bsslMethod) { | |||
| ossl.ossl_BIO_meth_set_callback_ctrl(osslMethod, nullptr); | |||
| } | |||
| else { | |||
| bssl_compat_fatal("BIO_METHOD::callback_ctrl is not supported"); | |||
| // Simulate a segfault | |||
There was a problem hiding this comment.
I know it's obvious, but just so it doesn't slip through the cracks, delete this null pointer deference code, and reinstate the bssl_compat_fatal(...) call.
bssl-compat/BUILD
Outdated
| @@ -15,7 +15,12 @@ cmake( | |||
| visibility = ["//visibility:public"], | |||
| generate_crosstool_file = False, | |||
| tags = ["requires-network"], | |||
| env = { "GOCACHE" : "/tmp" }, | |||
| env = { "GOCACHE" : "/tmp", | |||
There was a problem hiding this comment.
I don't think we need the GOCACHE environment variable any more. It was needed when the bssl-compat build included a full sub-build of BoringSSL (which used Go) but that is no longer the case, so it should be removed.
bssl-compat/BUILD
Outdated
| env = { "GOCACHE" : "/tmp" }, | ||
| env = { "GOCACHE" : "/tmp", | ||
| "CMAKE_C_COMPILER" : "clang", | ||
| "CMAKE_CXX_COMPILER" : "clang++", |
There was a problem hiding this comment.
When invoking cmake, bazel already passes CC & CXX in the environment, which is enough for cmake to do its thing. Therefore, CMAKE_C_COMPILER & CMAKE_CXX_COMPILER can be removed.
There was a problem hiding this comment.
This resulted necessary when building proxy (CMake couldn't find compilers) . However I 'll try again to confirm this.
There was a problem hiding this comment.
Without CMAKE_C_COMPILER or CMAKE_CXX_COMPILER in bssl-compat/BUILD, I confirmed the following builds still work for me locally:
- Your
dcillera/envoy-openssl[proxy-envoy-openssl]branch, in thequay.io/maistra-dev/maistra-builder:2.6builder. - Your
dcillera/proxy[for-proxy]branch, in the upstream builder.
Signed-off-by: Dario Cillerai <dcillera@redhat.com>
…set_enforce_rsa_key_usage Signed-off-by: Dario Cillerai <dcillera@redhat.com>
| @@ -569,14 +571,14 @@ void ContextImpl::logHandshake(SSL* ssl) const { | |||
| #error "Delete preprocessor check below; no longer needed" | |||
| #endif | |||
|
|
|||
| #if BORINGSSL_API_VERSION >= 18 | |||
| #if 0 | |||
There was a problem hiding this comment.
Could you reinstate the original upstream code, and then add an additional #if 0 with a comment, just around the SSL_was_key_usage_invalid() call. That way, the intent is clearer, and we are modifying upstream code less by only making a small addition of the #if 0.
Signed-off-by: Dario Cillerai <dcillera@redhat.com>
Envoy-openssl with some commits needed for Maistra Proxy
PR created to test CPAAS procedure in Proxy.
Signed-off-by: Dario CILLERAI dcillera@redhat.com