Added support to ignore invalid cert common name

[Jinming-Hu]

We need this feature when using storage library in Azure Storage server-side codebase. TLS certificate used for testing is self-signed. Service needs to be accessed via multiple hostnames including localhost, so we need to be able to ignore CN mismatch errors.

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

• C++ Guidelines
• Doxygen docs
• Unit tests
• No unwanted commits/changes
• Descriptive title/description
  • PR is single purpose
  • Related issue listed
• Comments in source
• No typos
• Update changelog
• Not work-in-progress
• External references or docs updated
• Self review of PR done
• Any breaking changes?

[LarryOsterman]

QQ: Where is the libcurl version of this fix? Or does Libcurl already ignore invalid CN names?

[Jinming-Hu]

QQ: Where is the libcurl version of this fix? Or does Libcurl already ignore invalid CN names?

It turns out libcurl will ignore all these kinds of errors when verify peer is disabled.

WinHTTP, however, has more granular control. https://learn.microsoft.com/en-us/windows/win32/winhttp/option-flags#winhttp_option_security_flags

[LarryOsterman]

QQ: Where is the libcurl version of this fix? Or does Libcurl already ignore invalid CN names?

It turns out libcurl will ignore all these kinds of errors when verify peer is disabled.

WinHTTP, however, has more granular control. https://learn.microsoft.com/en-us/windows/win32/winhttp/option-flags#winhttp_option_security_flags

There are global options now for disabling peer validation, so if you're ok with disabling peer validation on curl, why not also disable it globally?

In general, I'm really not a fan of per-transport options, because enabling them forces the customer to manually create their own transport object for the pipeline (which then forces them to make curl vs winhttp decisions, etc). So I would much prefer to see new transport features be available for all of them.

[Jinming-Hu]

There are global options now for disabling peer validation,

Are you referring to TransportOptions? There's no option to disable peer validation in there. In other words, if customer wants to disable peer validation, he has to specify a transport (curl or winhttp).

That's what I always think. Am I wrong?

[LarryOsterman]

There are global options now for disabling peer validation,

Are you referring to TransportOptions? There's no option to disable peer validation in there. In other words, if customer wants to disable peer validation, he has to specify a transport (curl or winhttp).

That's what I always think. Am I wrong?

@#$ - for some reason, I thought I'd added them with the websocket work.

IMHO, this would be better set as a global option to disable peer validation that would then be modified to the appropriate HTTP transport (so for WInHTTP, it would set the existing IgnoreUnknownCertificateAuthority and the new IgnoreInvalidCertificateCommonName field, for curl it would set SslVerifyPeer to false.

That way customers aren't forced to conditionally compile their code depending on what HTTP transport they are using.

My rule of thumb for these kinds of things is: "If the same functionality is implemented by more than one transport, it should be in TransportOptions, if the option is only relevant to a single transport, it should be in per-transport options".

[Jinming-Hu]

IMHO, this would be better set as a global option to disable peer validation that would then be modified to the appropriate HTTP transport (so for WInHTTP, it would set the existing IgnoreUnknownCertificateAuthority and the new IgnoreInvalidCertificateCommonName field, for curl it would set SslVerifyPeer to false.

Done!

[Jinming-Hu]

@LarryOsterman Can you take a look at the updated changes, to ensure you're happy with the it, especially the variable name?

[LarryOsterman]

My only concern is that the semantics of SslVerifyPeer is backwards from the other options (and yes, I know that the CURL option is SslVerifyPeer - that makes sense in the CURL context since this is exactly the CURL option name. But this setting applies to all transports.

[Jinming-Hu]

@LarryOsterman Can you review this PR again?

[LarryOsterman]

Looks great to me. Thank you for your patience.

[ahsonkhan]

Just making sure (because it seems to be ok), but @LarryOsterman would we need to update the connection key logic that we use for the connection pooling/caching? It is meant to be unique for each set of options (based on SslVerifyPeer being set).

azure-sdk-for-cpp/sdk/core/azure-core/src/http/curl/curl.cpp

Line 1293 in e2a1b99

key.append(options.SslVerifyPeer ? "1" : "0");

[LarryOsterman]

Good question: Based on my reading, the SslVerifyPeer option is already included in the connection key logic - see line 1284 of GetConnectionKey.