launchdarkly / cpp-sdks

C++ Client/Server SDKs
Other
5 stars 2 forks source link

"LDConfigSetSSLCertificateAuthority" and "LDConfigSetVerifyPeer" are not available from 3.0. #385

Closed ngangomsamananda closed 1 month ago

ngangomsamananda commented 4 months ago

Is your feature request related to a problem? Please describe. We are updating client sdk from v2.5.2 to v3.4.0 (c binding) but the equivalent replacement of LDConfigSetSSLCertificateAuthority and LDConfigSetVerifyPeer are not available.

cwaldren-ld commented 4 months ago

Hi @ngangomsamananda, you are correct, these are not available in the SDK at the moment.

I have filed an internal feature request.

Filed internally as 238927.

ngangomsamananda commented 4 months ago

@cwaldren-ld Thank you. How long does it take a feature request to complete? Will it be available in the next SDK release?

cwaldren-ld commented 4 months ago

Hi @ngangomsamananda , I cannot provide an estimate, but I will address it as soon as I am able.

ngangomsamananda commented 3 months ago

Hi @cwaldren-ld, is there any particular reason why LDConfigSetSSLCertificateAuthority and LDConfigSetVerifyPeer are not included in v3.x? Could you please take up this issue on priority than the other tickets (https://github.com/launchdarkly/cpp-sdks/issues/392 and https://github.com/launchdarkly/cpp-sdks/issues/394) which I had requested? We are not able to complete the sdk update because of these APIs.

cwaldren-ld commented 3 months ago

Hi, please accept my apologies for slow response as I am out of the office.

I will prioritize this feature when I return next week.

On Tue, Apr 16, 2024, 6:15 AM ngangomsamananda @.***> wrote:

Hi @cwaldren-ld https://github.com/cwaldren-ld, is there any particular reason why LDConfigSetSSLCertificateAuthority and LDConfigSetVerifyPeer are not included in v3.x? Could you please take up this issue on priority than the other tickets (

392 https://github.com/launchdarkly/cpp-sdks/issues/392 and #394

https://github.com/launchdarkly/cpp-sdks/issues/394) which I had requested? We are not able to complete the sdk update because of these APIs.

— Reply to this email directly, view it on GitHub https://github.com/launchdarkly/cpp-sdks/issues/385#issuecomment-2058948283, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWJYQJYILEDZXCXWRRCMBQTY5UI6LAVCNFSM6AAAAABFSVEJG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJYHE2DQMRYGM . You are receiving this because you were mentioned.Message ID: @.***>

ngangomsamananda commented 3 months ago

@cwaldren-ld No worries. In one of the tickets, you had mentioned you will be out of office this week. I have added the comment so that you can have a look when you come back.

cwaldren-ld commented 3 months ago

Hi @ngangomsamananda , the reason these APIs were not included was because we didn't have a proven use-case for them.

Since you have requested them, I have begun work here: https://github.com/launchdarkly/cpp-sdks/pull/391

I will update you when the work is complete.

ngangomsamananda commented 3 months ago

@cwaldren-ld Thank you so much.

cwaldren-ld commented 3 months ago

Hi @ngangomsamananda , I apologize this is taking longer than expected. I am still working on it.

The LDConfigSetVerifyPeer will be released first, then LDConfigSetSSLCertificateAuthority will come after.

ngangomsamananda commented 2 months ago

@cwaldren-ld Let me know when LDConfigSetVerifyPeer is release. I appreciate you for working this on priority.

cwaldren-ld commented 2 months ago

Hi @ngangomsamananda , we have released C++ Client v3.5.0 with support for disabling Peer Verification in TLS handshake.

Here is a link to the docs.

Example:

// First, create a new TLS Config builder
LDClientHttpPropertiesTlsBuilder tls_builder = LDClientHttpPropertiesTlsBuilder_New();

// Disable peer verification 
LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tls_builder, true);

// Assuming you have an existing LDClientConfigBuilder
LDClientConfigBuilder_HttpProperties_Tls(config, tls_builder);

Please note, the SDK will emit logs informing you that TLS peer verification is disabled, since it can be considered a security flaw in many scenarios. You can disregard the log message if you use this feature.

ngangomsamananda commented 2 months ago

@cwaldren-ld Thank you so much. I will check and let you know. One thing: is LDClientHttpPropertiesTlsBuilder_Free() need to be call? I saw this in the docs you have shared.

cwaldren-ld commented 2 months ago

Not normally.

If you pass the TLS builder into the HttpPropertiesBuilder, there is no need to call LDClientHttpPropertiesTlsBuilder_Free() (since the ConfigBuilder takes ownership of it.)

If you do not pass it into the HttpPropertiesBuilder, then yes you must call LDClientHttpPropertiesTlsBuilder_Free() to avoid memory leak.

Relevant part from the docs on LDClientHttpPropertiesTlsBuilder_Free():

Frees a TLS options builder. Do not call if the builder was consumed by the HttpProperties builder.

ngangomsamananda commented 2 months ago

Hi @cwaldren-ld , till now the replacement for LDConfigSetVerifyPeer appears to be working fine. If I face any issue, I will reach out to you. Thanks

cwaldren-ld commented 2 months ago

Hi @ngangomsamananda , please check out https://github.com/launchdarkly/cpp-sdks/pull/409 branch (cw/sc-244781/custom-ca) when you have a chance and test the new API:

// First, create a new TLS Config builder
LDClientHttpPropertiesTlsBuilder tls_builder = LDClientHttpPropertiesTlsBuilder_New();

// Set path to a custom CA file
LDClientHttpPropertiesTlsBuilder_CustomCAFile(tls_builder, "path/to/your/custom_ca.pem");

// Assuming you have an existing LDClientConfigBuilder
LDClientConfigBuilder_HttpProperties_Tls(config, tls_builder);

If you are unable to test the branch, please wait for the upcoming release.

ngangomsamananda commented 2 months ago

@cwaldren-ld I will do the testing when it is release. Thanks for working on that API.

cwaldren-ld commented 2 months ago

Hi @ngangomsamananda , release is available here: https://github.com/launchdarkly/cpp-sdks/releases/tag/launchdarkly-cpp-client-v3.6.0

ngangomsamananda commented 2 months ago

Hi @cwaldren-ld, thank you so much for providing the new API's in short time.

ngangomsamananda commented 2 months ago

Hi @cwaldren-ld We are getting some repeated logs like for every 30 seconds. Do you know what are these logs for? If it's not a major issue, is there a way to disable it? image

cwaldren-ld commented 2 months ago

Yes, please check your LD_LOG_LEVEL environment variable (or log level defined in code.)

These are debug-level logs that show that events are being sent to LaunchDarkly (flush) and that flag updates are arriving from LaunchDarkly (read bytes). Generally a good signal that the SDK is working properly.

If you use info level or above, you should not see them.

ngangomsamananda commented 1 month ago

@cwaldren-ld Thanks for clearing the doubts.

ngangomsamananda commented 1 month ago

Hi @cwaldren-ld , In some of the Windows 10 machine our application is crash due to LDClientConfigBuilder_HttpProperties_Tls. On my machine the crash is not happened. I am trying to understand what is issue. The snippet code is below.

`#include < iostream >

include <launchdarkly/client_side/bindings/c/sdk.h>

include <launchdarkly/bindings/c/context_builder.h>

include <launchdarkly/client_side/bindings/c/config/builder.h>

include <launchdarkly/bindings/c/object_builder.h>

include <launchdarkly/bindings/c/memory_routines.h>

LDClientConfigBuilder ConfigBuilder = nullptr; LDClientSDK g_pLDClient = nullptr; LDListenerConnection m_listenerConnection = nullptr;

static bool enabled(const LDLogLevel level, void* user_data) { return true; }

bool resetNewUserContext(const std::string& userAnalyticsId) { if (g_pLDClient != nullptr) { LDContextBuilder contextBuilder = nullptr; if (!userAnalyticsId.empty()) { contextBuilder = LDContextBuilder_New(); LDContextBuilder_AddKind(contextBuilder, "user", userAnalyticsId.c_str()); }

    bool identified = false;
    unsigned int waitTime = 5000; // milliseconds
    LDContext updatedContext = LDContextBuilder_Build(contextBuilder);

    if (LDClientSDK_Identify(g_pLDClient, updatedContext, waitTime, &identified))
    {
        if (identified)
        {
            // log message
        }
        else
        {
            // log message
        }
    }
    else
    {
        // log message
    }
}

}

void globalLogger(const LDLogLevel level, const char text, void user_data) { } void initLDCallback(LDDataSourceStatus status, void* userData) { LDDataSourceStatus_State statusState = LDDataSourceStatus_GetState(status); if (statusState == LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_INITIALIZING) { // log message } else if (statusState == LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_VALID) { } else if (statusState == LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_INTERRUPTED) { } else if (statusState == LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_OFFLINE) {
} else { } } bool getPeerVerify() { false; }

int main() { const char userid = "user-key"; const char mobile_key = "mob-abc-123"; ConfigBuilder = LDClientConfigBuilder_New(mobile_key);

bool bOffline = false;

if (bOffline)
    LDClientConfigBuilder_Offline(ConfigBuilder, bOffline);

// logging
LDLogBackend backend;
LDLogBackend_Init(&backend);
backend.Write = globalLogger;
backend.Enabled = enabled;
LDLoggingCustomBuilder customLogging = LDLoggingCustomBuilder_New();
LDLoggingCustomBuilder_Backend(customLogging, backend);
LDClientConfigBuilder_Logging_Custom(ConfigBuilder, customLogging);

LDClientConfigBuilder_Events_PrivateAttribute(ConfigBuilder, "hubId");

LDClientConfig config;
LDStatus status = LDClientConfigBuilder_Build(ConfigBuilder, &config);
if (!LDStatus_Ok(status)) {
  std::cout<<"ldcsdk client config builder failed";
}

LDClientHttpPropertiesTlsBuilder tlsBuilder = LDClientHttpPropertiesTlsBuilder_New();
if (tlsBuilder)
{
    // Disable peer verification
    LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, true);
    bool verifyPeer = getPeerVerify();
    if(verifyPeer)
    {
        LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, false);
    }
    LDClientConfigBuilder_HttpProperties_Tls(ConfigBuilder, tlsBuilder);
}

LDContextBuilder context_builder = LDContextBuilder_New();
LDContextBuilder_AddKind(context_builder, "user", userid);

LDContextBuilder_Attributes_SetPrivate(context_builder, "user", "email", LDValue_NewString("abc@gmail.com"));

LDContext context = LDContextBuilder_Build(context_builder);
g_pLDClient = LDClientSDK_New(config, context);
unsigned int maxwait = 10 * 1000; /* 10 seconds */

LDDataSourceStatusListener listener;
LDDataSourceStatusListener_Init(&listener);
listener.StatusChanged = initLDCallback;
m_listenerConnection = LDClientSDK_DataSourceStatus_OnStatusChange(g_pLDClient, listener);

bool initialized_successfully;
if (LDClientSDK_Start(g_pLDClient, maxwait, &initialized_successfully)) 
{
    if (initialized_successfully) 
    {
        std::cout << "LaunchDarkly: client Initialization Succeded." << std::endl;
    }
    else 
    {
        std::cout<<"LaunchDarkly: client Initialization failed\n";
    }
} 
else {
        std::cout<<"LaunchDarkly: The client is still initializing.\n";
}

bool initialized = LDClientSDK_Initialized(g_pLDClient);

bool initialized = LDClientSDK_Initialized(g_pLDClient);
const char* newUserid = "new-user-key";
resetNewUserContext(newUserid);

}`

I am not sure where am I doing wrong. Could you help me on this?

cwaldren-ld commented 1 month ago

Yes, I will test it out.

Just to clarify, is this a typo?

bool getPeerVerify()
{
    false;
}

it should be:

bool getPeerVerify()
{
    return false;
}
cwaldren-ld commented 1 month ago

Hi @ngangomsamananda , I have found another problem:

This block of configuration:


LDClientHttpPropertiesTlsBuilder tlsBuilder = LDClientHttpPropertiesTlsBuilder_New();
if (tlsBuilder)
{
    // Disable peer verification
    LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, true);
    bool verifyPeer = getPeerVerify();
    if(verifyPeer)
    {
        LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, false);
    }
    LDClientConfigBuilder_HttpProperties_Tls(ConfigBuilder, tlsBuilder);
}

Needs to be moved before you call LDStatus status = LDClientConfigBuilder_Build(ConfigBuilder, &config);.

Remember, once you have called LDClientConfigBuilder_Build it is undefined behavior to use any other method that operates on the ConfigBuilder. This is because once you call Build, the memory is deleted automatically.

The nature of undefined behavior in C++ is that it may crash, it may do nothing at all, or it may do anything at all.

ngangomsamananda commented 1 month ago

Yes, I will test it out.

Just to clarify, is this a typo?

bool getPeerVerify()
{
    false;
}

it should be:

bool getPeerVerify()
{
    return false;
}

Yes, that is typo. My bad.

ngangomsamananda commented 1 month ago

Hi @ngangomsamananda , I have found another problem:

This block of configuration:

LDClientHttpPropertiesTlsBuilder tlsBuilder = LDClientHttpPropertiesTlsBuilder_New();
if (tlsBuilder)
{
    // Disable peer verification
    LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, true);
    bool verifyPeer = getPeerVerify();
    if(verifyPeer)
    {
        LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, false);
    }
    LDClientConfigBuilder_HttpProperties_Tls(ConfigBuilder, tlsBuilder);
}

Needs to be moved before you call LDStatus status = LDClientConfigBuilder_Build(ConfigBuilder, &config);.

Remember, once you have called LDClientConfigBuilder_Build it is undefined behavior to use any other method that operates on the ConfigBuilder. This is because once you call Build, the memory is deleted automatically.

The nature of undefined behavior in C++ is that it may crash, it may do nothing at all, or it may do anything at all.

@cwaldren-ld I think this would be the issue. I have one more query. I want to change the peer verification after sdk is connected. Can I do something like the below code?

LDClientHttpPropertiesTlsBuilder tlsBuilder = LDClientHttpPropertiesTlsBuilder_New();
if (tlsBuilder)
{
    // Disable peer verification
    LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, true);
    bool verifyPeer = getPeerVerify();
    if(verifyPeer)
    {
        LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, false);
    }
    LDClientConfigBuilder_HttpProperties_Tls(ConfigBuilder, tlsBuilder);
}

LDClientConfig config;
LDStatus status = LDClientConfigBuilder_Build(ConfigBuilder, &config);
LDContext context = LDContextBuilder_Build(context_builder);
g_pLDClient = LDClientSDK_New(config, context);
unsigned int maxwait = 10 * 1000; /* 10 seconds */

if (LDClientSDK_Start(g_pLDClient, maxwait, &initialized_successfully)) 
{
}

// Enable peer verify
 LDClientHttpPropertiesTlsBuilder_SkipVerifyPeer(tlsBuilder, false);
cwaldren-ld commented 1 month ago

I want to change the peer verification after sdk is connected. Can I do something like the below code?

No, that is not supported. The snippet you provide is also undefined behavior.

May I please ask what is the situation that would require changing the peer verification mode at runtime? The way the SDK is setup is that you configure it once, and then use it.

If you need to change configuration, generally you should destroy the SDK and create a new one.

ngangomsamananda commented 1 month ago

@cwaldren-ld Thanks for clearing the doubts. In the old version of sdk the peer verification was change at runtime using LDConfigSetVerifyPeer. I was replacing that. Seeing in the code it looks like changing the peer verification mode at runtime for some specific use case. This is handled by some other team. I will check with them if it's really necessary to change peer verification at runtime and what are the use cases.

ngangomsamananda commented 1 month ago

@cwaldren-ld We have another crash issue. This is on MacOS machines. Some of the people face the issue. But I could not reproduce it. I am still not sure when the crash got occurred and why. The only thing I know is the crash happened after the SDK is configured and running. I have the crash log. I am not sure that is enough for you to identify the root cause of the crash. Screenshot 2024-06-11 at 2 44 26 PM image (3)

On different thread this is showing: Screenshot 2024-06-12 at 1 00 38 PM

I have edited code of https://github.com/launchdarkly/cpp-sdks/issues/385#issuecomment-2160120815 to get you better understanding of the scenario. In our caseresetNewUserContext is call multiple times.

FYI, the binaries which is use here is built by me by running the script https://github.com/launchdarkly/cpp-sdks/blob/main/scripts/build-release.sh because I needed the Universal Binaries and also boost needed to be statically link.

cwaldren-ld commented 1 month ago

Hi @ngangomsamananda , it is not allowed to call Identify in two scenarios: (1) while initialization is in progress from Start (2) while another Identify is taking place

Doing so could cause a crash. From your comment, I think maybe this is the problem since you mention:

In our case resetNewUserContext is call multiple times.

See docs here:

Only one Identify call can be in progress at once; calling it concurrently invokes undefined behavior.

If you need to call Identify, make sure you do it only if Start operation completed. For example:

bool initialized_successfully;
bool completed = LDClientSDK_Start(client, 5000, &initialized_successfully);
if (!completed) {
  // The Start operation is still in progress, you cannot call Identify yet. You can find out when it succeeds using
  // 
} else {
  // Start is complete. It may have succeeded or failed, you can check with initialized_successfully bool.
  // You could call Identify.
}

Similarly with Identify:

bool identified_successfully;
bool completed = LDClient_Identify(client, context, 5000, &identified_successfully);
if (!completed) {
   // The Identify operation is still in progress, you cannot call it again because that is undefined behavior.
} else {
  // Identify is complete. It may have succeeded or failed, but either way you could call it again.
}

You can think of the SDK performing Start and Identify asynchronously. The Start and Identify methods don't wait until the operation is complete, instead they take a "wait" time parameter.

If the operation completes in less than the "wait" time, then you know it's safe to start another operation. Otherwise, it's not safe because the same operation cannot be running in parallel.

This is something we want to improve in future version of the SDK so you don't have to worry about it.

cwaldren-ld commented 1 month ago

To help you, I need to know if your application is structured as an event-driven architecture, or blocking architecture.

In other words does your application do blocking I/O calls everywhere (wait for the result to complete), or does it use callbacks to know when operations are complete?

If blocking, you could call LDClientSDK_Initialized() repeatedly in a loop with a short wait time to know when the operation is complete.

If callbacks, you could register a DataSourceStatus listener and receive a callback when the operation is complete.

ngangomsamananda commented 1 month ago

Hi @cwaldren-ld , we use the callback method. It's also present in the above code https://github.com/launchdarkly/cpp-sdks/issues/385#issuecomment-2160120815. I have looked in the application logs today. It looks like the crash happened when internet connectivity gets restored. So, when the Internet connectivity is down, we get a log "read timeout reached" then I get a callback status LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_SHUTDOWN. Then when the internet connection is restored the LDClient_Identify get call. Perhaps because LDClient_Identify is called before the sdk is connected back caused the crash?

From the log I can see after the callback status LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_SHUTDOWN there is LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_INITIALIZING . LDDataSourceStatus_State::LD_DATASOURCESTATUS_STATE_INITIALIZING gets whenever we call LDClient_Identify method.

cwaldren-ld commented 1 month ago

@ngangomsamananda , yes, it is likely the Identify call is interfering with the SDK automatically restoring the streaming connection when the internet is back.

Please note: if the connection is lost, there is no need to call Identify. It only needs to be called if the Context attributes have changed (such as context key, kind, or any other attribute.)

I understand this makes it difficult to use Identify correctly. We are planning a way to ensure this is safer in future version of SDK.

ngangomsamananda commented 1 month ago

@cwaldren-ld I think that was the issue. Identify call is interfering with the SDK automatically restoring the streaming. Now I have few conditions check before calling the Identify. I hope this resolve the issue.

ngangomsamananda commented 1 month ago

Hi @cwaldren-ld, we get another issue. we are not getting expected result from the old and the new implementation of below code

Old Sdk code:

 std::string value = "[]";
   std::string flagName = "flag-name";
    struct LDJSON* fallback = LDJSONDeserialize(value.c_str() );
    struct LDJSON* evaluation = LDJSONVariation(g_pLDClient, flagName.c_str(), fallback);
    char* evaluationString = LDJSONSerialize(evaluation);
    LDFree(evaluation);
    LDFree(fallback);

In the new SDK the same code is replaced with:

std::string value = "[]";
std::string flagName = "flag-name";
LDValue defaultValue = LDValue_NewString(value.c_str());
LDValue resultValue = LDClientSDK_JsonVariation(g_pLDClient, flagName.c_str(), defaultValue);
const char* evaluationString = LDValue_GetString(resultValue);
LDValue_Free(resultValue);
LDValue_Free(defaultValue);

Since LDJSONDeserialize and LDJSONSerialize does not have replacement so i used LDValue_NewString and LDValue_GetString but this does give the same result. Do you have idea what would be the replacement for LDJSONDeserialize and LDJSONSerialize?

cwaldren-ld commented 1 month ago

Hi @ngangomsamananda , I have filed a new issue with response here: https://github.com/launchdarkly/cpp-sdks/issues/416

Please respond in the new issue. I will close this one, since LDConfigSetSSLCertificateAuthority and LDConfigSetVerifyPeer are now implemented.