Use the platform native APIs with C++ SSL transport #2063

pepone · 2024-04-19T16:37:24Z

This PR adds the ability to programmatically configure the C++ SSL transport using the platform native API.

This PR adds two structs for configuring the SSL transport, Ice::SSL::ClientAuthenticationOptions and Ice::SSL::ServerAuthenticationOptions.

I considered two important use cases for this PR:

User must be able to provide its own validation logic using the platform native API (OpenSSL, Schannel, SecureTransport)
User must be able to easy provide credentials for a new connection without restarting.

The new configuration API also allow for using the native API for setting lower level details.

The PR reworks the SSL engines to create the ClientAuthenticationOptions and ServerAuthenticationOptions based on the IceSSL properties, when the user doesn't explicitly provide its own options. This shows that the new option structs are enough to replace the IceSSL properties currently supported.

Things to consider for follow up PRs:

Rework IceSSL/configuration, and add more tests for the new API.
Move IceSSL -> to Ice::SSL in all languages
Remove the C++ Certificate API, it is currently used to expose the peer certificate chain in IceSSL::ConnectionInfo
I was thinking of also removing info.certs and rely on custom validation callbacks to obtain the peer certificates, but I think we use this in a Glacier2 and IceGrid implementations to fill SSLInfo forwarder to session managers.

cpp/include/Ice/SSL.h

cpp/include/Ice/SSLConnectionInfo.h

cpp/src/Ice/ObjectAdapterFactory.cpp

cpp/src/Ice/ObjectAdapterI.h

bernardnormier · 2024-04-21T14:06:14Z

cpp/src/IceSSL/SSLEngine.h

@@ -26,6 +27,7 @@ namespace IceSSL

        Ice::LoggerPtr getLogger() const;
        Ice::PropertiesPtr getProperties() const;
+        Ice::InitializationData getInitializationData() const;


It's not clear to me who is the client for this public exported class.

Is it a public / user-documented API even though there is no doc-comment.

We should move this to IceInternal, it is only used by the SSL transport implementation.

bernardnormier · 2024-04-21T14:07:49Z

cpp/include/Ice/SSL.h

+#    undef SECURITY_WIN32
+#endif
+
+namespace Ice::SSL


Is the idea to rename IceSSL -> Ice::SSL in another PR?

Yes, and do the same in C#/Java replacing the top-level IceSSL namespace with Ice::SSL.

cpp/include/Ice/ClientAuthenticationOptions.h

cpp/include/Ice/ServerAuthenticationOptions.h

pepone · 2024-04-24T17:04:25Z

cpp/include/Ice/ServerAuthenticationOptions.h

+        /**
+         * The server's certificate chain.
+         */
+        CFArrayRef serverCertificateChain;


This is the simplest when you want to always use the same certificate chain.

It is used to call SSLSetCertificate before the handshake starts.

pepone · 2024-04-24T17:07:53Z

cpp/include/Ice/ServerAuthenticationOptions.h

+         * @param host The server's host name.
+         * @return The server's certificate chain.
+         */
+        std::function<CFArrayRef(const std::string& host)> serverCertificateSelectionCallback;


This callback allows selecting the certificate before SSL handshake starts, host parameter should probably be the adapter name, a bit more convenient way when you want to be able to upgrade the certificate without restart.

pepone · 2024-04-24T17:09:56Z

cpp/include/Ice/ServerAuthenticationOptions.h

+         * A callback that is called before a new SSL handshake starts. The callback can be used to set additional SSL
+         * context parameters.
+         */
+        std::function<void(SSLContextRef)> sslNewSessionCallback;


You can also set the certificates from this callback, but the other certificate properties simplify it, and the use case for this callback is for more advanced or less used settings for which we don't provide a property.

pepone · 2024-04-24T17:17:45Z

cpp/include/Ice/ServerAuthenticationOptions.h

+        std::function<int(int ok, X509_STORE_CTX*, const IceSSL::ConnectionInfoPtr& info)>
+            clientCertificateVerificationCallback;


The implementation wraps this callback and uses it as the OpenSSL verification callback. The signature is the same with the additional ConnectionInfo. The info param is required for the trust verifier, but I plan to remove this dependency in a follow-up PR.

The wrapper saves exceptions thrown from the callback, and rethrows them from the transceiver initialization.

pepone · 2024-04-24T17:22:01Z

cpp/include/Ice/ServerAuthenticationOptions.h

+         * A callback that is called before a new SSL handshake starts. The callback can be used to set additional SSL
+         * parameters.
+         */
+        std::function<void(::SSL* ssl, const std::string& host)> sslNewSessionCallback;


This allows configuring the SSL session before the handshake starts, the SSL object is created from the provided SSL_CTX.

Need to think about the defaults we should use here, or if we can count on OpenSSL defaults being always appropriate.

While more of the parameters can be set in both SSL_CTX and SSL, a few like SNI are only available for SSL object, that is why we use to callbacks.

pepone · 2024-04-24T19:00:02Z

cpp/include/Ice/ServerAuthenticationOptions.h

+        /**
+         * The server's SSL context, this objects holds configuration relevant the SSL session establishment.
+         */
+        SSL_CTX* serverSslContext;


A callback would be more convenient here too.

externl

Looks good to me! I may have been a bit overzealous with nullptr comments, please ignore any wrong ones :)

externl · 2024-05-02T17:53:45Z

.devcontainer/Dockerfile

@@ -9,7 +9,7 @@ RUN set -eux \
    && sudo dpkg -i packages-microsoft-prod.deb \
    && rm packages-microsoft-prod.deb \
    && sudo apt update \
-    && sudo apt-get install -y python3 python3-dev python3-passlib ruby-full \


Is gdb necessary?

I use it to debug the tests, I think it is fine for a development container.

externl · 2024-05-02T18:11:51Z

cpp/include/Ice/ClientAuthenticationOptions.h

+         * @remarks The trusted root certificates are used by both the default validation callback, and by custom
+         * validation callback set in clientCertificateValidationCallback.
+         *
+         * The application must ensure that the certificate store remains valid during the setup of the Communicator. It


Should setup be initialization?

Suggested change

* The application must ensure that the certificate store remains valid during the setup of the Communicator. It

* The application must ensure that the certificate store remains valid during the setup of the Communicator. It

I think that's actually obvious and not necessary to document. initialize() is a synchronous function. While it runs, you should not free the arguments you passed via initData.

What about the second part about releasing the certificate store?

externl · 2024-05-02T18:13:46Z

cpp/include/Ice/ClientAuthenticationOptions.h

+         *
+         * @remarks This callback is invoked by the SSL transport for each new outgoing connection before starting the
+         * SSL handshake to determine the appropriate client certificate chain. The callback should return a CFArrayRef
+         * that represents the client's certificate chain, or NULL if no certificate chain should be used for the


nullptr?

Suggested change

* that represents the client's certificate chain, or NULL if no certificate chain should be used for the

* that represents the client's certificate chain, or nullptr if no certificate chain should be used for the

I have to check if nullptr works with CFArrayRef

I thought that nullptr as a drop in replacement for 0 or NULL.

externl · 2024-05-02T18:16:36Z

cpp/include/Ice/ClientAuthenticationOptions.h

cpp/include/Ice/ServerAuthenticationOptions.h

cpp/src/IceSSL/SSLEngine.cpp

cpp/src/IceSSL/SecureTransportTransceiverI.cpp

bernardnormier

I like the API but I am concerned about the generated C++ API reference. How is it going to work with all these ifdef?

When you look-up Ice::Ssl::ClientAutenticationOptions::clientCertificateSelectionCallback, which platform-specific doc-comment do you see? All of them?

It would be clearer for the API reference to have different names, like:

#if defined (ICE_SCHANNEL)
struct SchannelClientAuthenticationOptions
{
    // lots of Schannel-specific doc-comment
   ...
};

using ClientAuthenticationOptions = SchannelClientAuthenticationOptions; // alias for portable code

#elif defined(ICE_SECURE_TRANSPORT)
...

cpp/include/Ice/Certificate.h

bernardnormier · 2024-05-03T01:11:54Z

cpp/include/Ice/ClientAuthenticationOptions.h

+    {
+#if defined(_WIN32)
+        /**
+         * A callback that allows selecting the client's SSL certificate based on the target server host name.


This target server host name is the host name in the endpoint you're using to establish the connection?

If yes, I think something like "based on the hostname or IP address in the endpoint used to establish the connection" would be clearer.

Yes, it is.

cpp/include/Ice/ClientAuthenticationOptions.h

bernardnormier · 2024-05-03T01:16:21Z

cpp/include/Ice/ClientAuthenticationOptions.h

+         * server's certificate chain is validated against these certificates; otherwise, the system's default root
+         * certificates are used.
+         *
+         * @remarks The trusted root certificates are used by both the default validation callback, and by custom


are used by both the default validation callback and by the custom validation ... (no comma, and the).

Did you mean serverCertificateValidationCallback?

Since these fields are not sorted in alphabetical order, I would put this trustedRootCertificates next to serverCertificateValidationCallback.

Also, do we document what the default validation callback is?

bernardnormier · 2024-05-03T01:19:17Z

cpp/include/Ice/ClientAuthenticationOptions.h

+         * @remarks The trusted root certificates are used by both the default validation callback, and by custom
+         * validation callback set in clientCertificateValidationCallback.
+         *
+         * The application must ensure that the certificate store remains valid during the setup of the Communicator. It


I think that's actually obvious and not necessary to document. initialize() is a synchronous function. While it runs, you should not free the arguments you passed via initData.

cpp/include/Ice/ServerAuthenticationOptions.h

cpp/include/Ice/ClientAuthenticationOptions.h

cpp/include/Ice/ServerAuthenticationOptions.h

Co-authored-by: Joe George <joe@externl.com>

pepone · 2024-05-03T09:16:05Z

It would be clearer for the API reference to have different names, like:

That makes sense, assuming we can still use the alias in ObjectAdapter/InitializationData declarations.

bernardnormier

Looks good!

cpp/src/Ice/ObjectAdapterI.cpp

cpp/src/Ice/ObjectAdapterI.h

pepone added 3 commits April 17, 2024 15:25

Use ssl native API

e299cf6

Checkpoint

6b9286f

Checkpoint

ddb1fb4

pepone requested review from bernardnormier and externl April 19, 2024 16:37

bernardnormier reviewed Apr 21, 2024

View reviewed changes

pepone added 2 commits April 21, 2024 20:43

SChannel fixes

c36953a

SChannel certificate validation fixes

f176d7d

pepone force-pushed the ssl-native branch from e65163b to f176d7d Compare April 22, 2024 14:26

pepone marked this pull request as draft April 22, 2024 14:32

externl reviewed Apr 22, 2024

View reviewed changes

cpp/include/Ice/ClientAuthenticationOptions.h Outdated Show resolved Hide resolved

pepone added 6 commits April 23, 2024 16:50

SecureTransport fixes

b18f2f5

Add TLS 1.2 protocol name

be82bae

OpenSSL fixes

c877a81

Cleanup

ed9711b

Merge remote-tracking branch 'origin/main' into ssl-native

319ac21

Cleanup

ae94cd4

externl reviewed Apr 24, 2024

View reviewed changes

cpp/include/Ice/ServerAuthenticationOptions.h Outdated Show resolved Hide resolved

pepone commented Apr 24, 2024

View reviewed changes

pepone added 6 commits April 25, 2024 12:01

Merge remote-tracking branch 'origin/main' into ssl-native

9d13c39

Checkpoint

2dfc388

Additional fixes

1e48e73

Fix clang format

02ac138

Doc comment fixes

d61d27f

Script language fixes

3588bd4

pepone added 6 commits April 30, 2024 18:00

Merge remote-tracking branch 'origin/main' into ssl-native

0bec18e

SecureTransport updates

89d2d1b

Improve OpenSSL usage doc comments

aec11f5

Retain/Release SecureTransport trusted roots

1607c42

OpenSSL bug with bogus _sslCtx reference

bd70794

SChannel updates

7c8290f

pepone requested review from externl and bernardnormier May 2, 2024 14:40

externl approved these changes May 2, 2024

View reviewed changes

bernardnormier reviewed May 3, 2024

View reviewed changes

pepone and others added 6 commits May 3, 2024 10:29

Update cpp/src/IceSSL/OpenSSLTransceiverI.cpp

ab5d208

Co-authored-by: Joe George <joe@externl.com>

Update cpp/src/IceSSL/SecureTransportTransceiverI.cpp

2a6c820

Co-authored-by: Joe George <joe@externl.com>

Update cpp/src/IceSSL/SecureTransportTransceiverI.cpp

4326856

Co-authored-by: Joe George <joe@externl.com>

Update cpp/src/IceSSL/SChannelEngine.cpp

063dbc8

Co-authored-by: Joe George <joe@externl.com>

Update cpp/src/IceSSL/OpenSSLTransceiverI.cpp

d8851b4

Co-authored-by: Joe George <joe@externl.com>

Update cpp/src/IceSSL/SChannelTransceiverI.cpp

79426bd

Co-authored-by: Joe George <joe@externl.com>

pepone added 6 commits May 3, 2024 12:04

Review fixes

ed26e4e

More review fixes

97c3dc6

Fix typo

e0bf553

Cleanup

f406d54

Merge remote-tracking branch 'origin/main' into ssl-native

3f0b970

Build fixes

456022a

bernardnormier self-requested a review May 3, 2024 13:14

bernardnormier approved these changes May 3, 2024

View reviewed changes

cpp/src/Ice/ObjectAdapterI.cpp Outdated Show resolved Hide resolved

cpp/src/Ice/ObjectAdapterI.h Outdated Show resolved Hide resolved

Additional review fixes

01f506f

pepone merged commit 91ff0c7 into zeroc-ice:main May 3, 2024
17 checks passed

This was referenced May 17, 2024

SChannel host name verification #2066

Closed

Move IceSSL::SSLEngine::verifyPeerCertName to SchannelEngine implementation #2111

Closed

pepone mentioned this pull request Jun 27, 2024

Merge IceSSL into the Ice core #1575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the platform native APIs with C++ SSL transport #2063

Use the platform native APIs with C++ SSL transport #2063

pepone commented Apr 19, 2024 •

edited

Loading

bernardnormier Apr 21, 2024 •

edited

Loading

pepone Apr 21, 2024

bernardnormier Apr 21, 2024

pepone Apr 21, 2024

pepone Apr 24, 2024 •

edited

Loading

pepone Apr 24, 2024

pepone Apr 24, 2024

pepone Apr 24, 2024

pepone Apr 24, 2024 •

edited

Loading

pepone Apr 24, 2024

externl left a comment

externl May 2, 2024

pepone May 3, 2024

externl May 2, 2024

bernardnormier May 3, 2024

pepone May 3, 2024

externl May 2, 2024

pepone May 3, 2024

externl May 3, 2024

externl May 2, 2024

bernardnormier left a comment

bernardnormier May 3, 2024

pepone May 3, 2024 •

edited

Loading

bernardnormier May 3, 2024

bernardnormier May 3, 2024

bernardnormier May 3, 2024

pepone commented May 3, 2024

bernardnormier left a comment

		std::function<int(int ok, X509_STORE_CTX*, const IceSSL::ConnectionInfoPtr& info)>
		clientCertificateVerificationCallback;

	* The application must ensure that the certificate store remains valid during the setup of the Communicator. It
	* The application must ensure that the certificate store remains valid during the setup of the Communicator. It

	* that represents the client's certificate chain, or NULL if no certificate chain should be used for the
	* that represents the client's certificate chain, or nullptr if no certificate chain should be used for the

Use the platform native APIs with C++ SSL transport #2063

Use the platform native APIs with C++ SSL transport #2063

Conversation

pepone commented Apr 19, 2024 • edited Loading

bernardnormier Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone commented May 3, 2024

bernardnormier left a comment

Choose a reason for hiding this comment

pepone commented Apr 19, 2024 •

edited

Loading

bernardnormier Apr 21, 2024 •

edited

Loading

pepone Apr 24, 2024 •

edited

Loading

pepone Apr 24, 2024 •

edited

Loading

pepone May 3, 2024 •

edited

Loading