Fix default value for SSL::Context default_verify_param

[waj]

This is a fix for #5266 but it might require a review from someone with better understanding of x509.

We've just been debugging this issue with @ggiraldez and found that it seems like the verify parameters are inverted for Client and Server.

GitHub certificates has both key usages (client and server) and that's why they pass the verification, but Google new certificate has only "server" usage.

We'd like to actually know if this verification must be enabled by default on every context. Otherwise, just the validity of the certificate is checked.

@jhass what do you think?

[will]

Adding this as a monkey patch to my app did work to let me use google oauth without the insecure context workarounds:

abstract class OpenSSL::SSL::Context
  class Client < Context
    def initialize(method : LibSSL::SSLMethod = Context.default_method)
      super(method)

      self.verify_mode = OpenSSL::SSL::VerifyMode::PEER
      {% if LibSSL::OPENSSL_102 %}
        self.default_verify_param = "ssl_server"
        {% end %}
    end
  end

  class Server < Context
    def initialize(method : LibSSL::SSLMethod = Context.default_method)
      super(method)

      add_options(OpenSSL::SSL::Options::CIPHER_SERVER_PREFERENCE)
      {% if LibSSL::OPENSSL_102 %}
        self.default_verify_param = "ssl_client"
        {% end %}

      set_tmp_ecdh_key(curve: LibCrypto::NID_X9_62_prime256v1)
    end
  end
end

[jhass]

Looks good to me. I'm unable to unearth any useful docs on this right now and I don't remember how I figured this out initially, so probably reading openssl source code -> likely that I just missed this :)

[ysbaddaden]

I suppose it's fine. Any reference anywhere about that?

[waj]

@ysbaddaden No, I couldn't find a good reference about this. I didn't test the server part and I wonder what it's actually verifying there? I guess is the client certificate in case there is one. Can client requests still be made without a client certificate?

[ysbaddaden]

Documentation is available in the x509 man page under CERTIFICATE EXTENSIONS:
https://www.openssl.org/docs/manmaster/man1/x509.html

SSL certificates may have a "purpose" to add another level of trust and verification to the certificate. For example a certificate may be trusted as a SSL Client (or SSL Server) and should be refused when it acts for another purposes.

I assume the X509 context verification is meant to verify the received certificate, which means a server will verify a client certificate, and a client will verify a server certificate. I assume we got the purposes wrong? The OpenSSL API makes it confusing, since we set the purpose on the SSL context (client/server) but must set an opposite value (server/client)...

[ysbaddaden]

Maybe we should document this, and properly explain in the commit message (since the value is weird, a git blame will help).

[RX14]

Can verify this fix.

[RX14]

I agree that a bit of docs should be added though.

[matiasgarciaisaia]

@waj @RX14 @ysbaddaden @jhass I've updated the commit's message to reflect why are we changing this, with the relevant OpenSSL/x509 RFC docs linked.

I've also rebased this to be included in a future 0.24.2 release.

Do you think this is OK? Any ideas on how to clarify further?

[ysbaddaden]

👍

[RX14]

Ensure this is cherry-picked into master please.

[RX14]

This is still not cherry-picked into master. This is why I dislike all these weird release branches.