forwarder: Use TLS 1.2 by default

[Kaderinho]

What does this PR do?

This PR sets the default value of min_tls_version to tlsv1.2 and completely removes force_tls_12 from the code logic.

Motivation

TLS 1.0 and 1.1 are insecure so we should use TLS 1.2 by default. Customers still can use TLS 1.0 and 1.1 by setting min_tls_version

Additional Notes

Can we confirm that we are depreciating the force_tls_v12 option ? If yes, can I just remove all the references to force_tls_v12 in the code. (by removing this line config.BindEnvAndSetDefault("force_tls_12", false)) ?

Possible Drawbacks / Trade-offs

That might breaks some customers' configuration if they are sending data to a proxy / non Datadog endpoint which do not support TLS 1.2

Describe how to test/QA your changes

Changes are unit tested but it is still worth to check if min_tls_version is correctly setup.

Python script

This scripts creates several fake TLS servers with different TLS version starting from TLS 1.0 to TLS 1.2. The servers will refuse a connection if the client (Agent in this case) is using a version above the one used by the server.

For instance, if the Agent can connect to a TLS 1.0 server, it means that min_tls_server is set to tlsv1.0 because it would have failed if the min_tls_server was tlsv1.1.

# ssl_server.py
import socket
import ssl

def test_conn_tls_version(protocol):

    # This function simply creates a listening socket with a given tls protocol version
    # and waits for a connection. Then it outputs if the connection was successful or not
    # so it's possible to know if the client (Agent in our case) is using the same tls version
    # or above.
    context = ssl.SSLContext(protocol)
    context.load_cert_chain(certfile='cert.pem', keyfile='private.key')

    with socket.socket(socket.AF_INET, socket.SOCK_STREAM, 0) as sock:
        sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
        sock.bind(('127.0.0.1', 8443))
        sock.listen(5)

        with context.wrap_socket(sock, server_side=True) as ssock:
            try:
                print("Waiting for connection ...")
                conn, addr = ssock.accept()
                print('Used tls version: ', conn.version())
                conn.close()
                return True
            except ssl.SSLError as e:
                print(f"Agent cannot communicate with the server using {str(protocol)}")
            except Exception:
                pass
    print("==============================================================================\n")
    return False


# Testing connection establishment with tls version starting from 1.0 to 1.2.
# The first successful connection is actually the value of min_tls_version in the Agent
tls_versions =  [(ssl.PROTOCOL_TLSv1, 'tlsv1.0'), (ssl.PROTOCOL_TLSv1_1, 'tlsv1.1'), (ssl.PROTOCOL_TLSv1_2, 'tlsv1.2')]
for tls_version, version_str in tls_versions:
    if test_conn_tls_version(tls_version):
        print(f"Agent min_tls_version is set to {version_str}")
        break

Agent configuration

Technically we want the following behavior :

if min_tls_version is set and valid:
     min = min_tls_version
else
     min = TLSv1.2

If we want to be exhaustive we need to test all the combinations of :

• min_tls_version: not set, invalid, tlsv1.0, tlsv1.1, tlsv1.2, tlsv1.3
• force_tls_12: not set, true, false

To make the testing less cumbersome we can set force_tls_12: true and test only the different values for min_tls_version

Steps to follow :

• Stop the Agent
• Set DD_DD_URL to https://localhost:8443 and min_tls_version to the tested value
• Launch the python script with python3 ssl_server.py
• Start the Agent
• Verify the output of the script : the last line should be Agent min_tls_version is set to tlsv<x.y>

Here are the values that need to be tested and the expected python output :

• [ ] min_tls_version: "hello" -> tlsv1.2
• [ ] min_tls_version: "" -> tlsv1.2
• [ ] min_tls_version: "tlsv1.0" -> tlsv1.0
• [ ] min_tls_version: "tlsv1.1" -> tlsv1.1
• [ ] min_tls_version: "tlsv1.2" -> tlsv1.2
• [ ] min_tls_version: "tlsv1.3" -> Agent is not able to communicate with servers

Reviewer's Checklist

• [ ] If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
• [ ] Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
• [ ] A release note has been added or the changelog/no-changelog label has been applied.
• [ ] Changed code has automated tests for its functionality.
• [ ] Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
• [ ] At least one team/.. label has been applied, indicating the team(s) that should QA this change.
• [ ] If applicable, docs team has been notified or an issue has been opened on the documentation repo.
• [ ] If applicable, the need-change/operator and need-change/helm labels have been applied.
• [ ] If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
• [ ] If applicable, the config template has been updated.

[Kaderinho]

Given all the comments I believe that we have two scenarios:

• Completely remove references to force_tls_v12 (in config.go and config_template.yaml)
• Keep references (for a few releases) even if it's not used anymore + add a warning about it

My opinion is that we should be consistent with what we did previously if we have any example of config parameter deletion. Otherwise I feel like the second option (keep) has a little bit more value as it give information to the customers vs just making an option disappear.

Also, we need to choose between upgrade and enhancements.

WDYT ? @hush-hush @sgnn7 (please correct me if I'm wrong)

[hush-hush]

Given all the comments I believe that we have two scenarios:

• Completely remove references to force_tls_v12 (in config.go and config_template.yaml)
• Keep references (for a few releases) even if it's not used anymore + add a warning about it

My opinion is that we should be consistent with what we did previously if we have any example of config parameter deletion. Otherwise I feel like the second option (keep) has a little bit more value as it give information to the customers vs just making an option disappear.

That field was only used when min_tls_version was unset or set to an invalid version. Since we now default to this behavior there is no need to keep it. Removing it will not impact users that are using it (it will just be ignored and TLS 1.2 will be used as expected). The real choice is between removing this field and moving to 1.2 or printing a warning saying that we will move to 1.2 in a few releases.

Also, we need to choose between upgrade and enhancements.

I think we should use upgrade since we're changing the behavior of the agent. Users look at this section to know if they need to take any actions when upgrading. Here they need to if they want to keep using TLS 1.0.

[sgnn7]

@Kaderinho I think there was a majority leaning towards both removal of the old option (without trailing warning) and leaning to use of upgrade field for the release note. I think we can remove the logic around force_tls_v12 for sure since it's a noop after we bump min_tls_version to TLS v1.2 but I would somewhat prefer we have a warning on force_tls_v12 option use since the users do not need this field in their configs anymore. With that said, there's no need to hold up this PR over minute details so I'm ok with deferring to the team on what path to choose.

[hush-hush]

@Kaderinho I think there was a majority leaning towards both removal of the old option (without trailing warning) and leaning to use of upgrade field for the release note. I think we can remove the logic around force_tls_v12 for sure since it's a noop after we bump min_tls_version to TLS v1.2 but I would somewhat prefer we have a warning on force_tls_v12 option use since the users do not need this field in their configs anymore. With that said, there's no need to hold up this PR over minute details so I'm ok with deferring to the team on what path to choose.

The agent already log a warning about all unknown key in the configuration at startup.

[Kaderinho]

Hi @jtappa,

The release note wording has changed since your approval. Would it be possible to have a final check to sure that everything is correct please ? 🙏

[Kaderinho]

Thanks ! 🙏