Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: bump default TLS version to TLS v1.2 #121

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented May 8, 2023

What does this PR do?

Bump the default TLS version to TLS v1.2 from TLS v1.1.

Support for TLS v1.1 is not removed for the sake of compatibility.

Why is it important?

Motivated from the work in APM Server toward bumping the default TLS version to 1.2: elastic/apm-server#10491

TLS v1.1. was formally deprecated in 2021 and is considered obsolete: https://datatracker.ietf.org/doc/html/rfc8996

Several standard and the NIST guidelines for TLS implementation recommends not using TLS 1.1 or lower.

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
    - [ ] I have added an entry in CHANGELOG.md

Author's Checklist

  • [ ]

Related issues

Related to elastic/apm-server#10491

@kruskall kruskall added the enhancement New feature or request label May 8, 2023
@kruskall kruskall requested a review from a team as a code owner May 8, 2023 16:05
@kruskall kruskall requested review from belimawr and pierrehilbert and removed request for a team May 8, 2023 16:05
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-05-08T16:05:24.706+0000

  • Duration: 9 min 37 sec

Test stats 🧪

Test Results
Failed 0
Passed 777
Skipped 5
Total 782

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@cmacknz cmacknz added the Team:Elastic-Agent Label for the Agent team label May 8, 2023
belimawr
belimawr previously approved these changes May 8, 2023
@cmacknz
Copy link
Member

cmacknz commented May 8, 2023

Let's hold off on merging this until we have have sanity checked that this won't do anything surprising when Beats, Elastic Agent, and Fleet Server are eventually updated with this change.

In particular we need to verify that an Elastic Agent defaulting to TLS v1.2 can connect to a Fleet Server defaulting to TLS v1.1 and vice versa. The version negotiation should just work, but the downside and impact of being wrong about that is big enough that I don't want to assume there will be no problems. Let's actually test it first.

@cmacknz cmacknz dismissed belimawr’s stale review May 8, 2023 19:03

Needs testing before merge

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@simitt
Copy link

simitt commented Jun 12, 2023

@cmacknz can you provide an update whether this is planned to make it into 8.9?

@cmacknz
Copy link
Member

cmacknz commented Jun 12, 2023

No progress on this, not a high priority for us yet.

@jlind23
Copy link
Contributor

jlind23 commented Jan 25, 2024

@pierrehilbert can we please make this a priority for one of the next upcoming sprints? As stated by @cmacknz we should test this and it will most probably work OOTB.
Testing 7.17.X and 8.X together would be the path to take.

@ycombinator
Copy link
Contributor

As mentioned in #121 (comment), the work left to do on this PR now is testing the impact of the changes in this PR before we can be confident about merging it. Concretely, the following two tests will need to be performed:

Agent using TLSv1.1 can talk to Fleet Server using TLSv1.2

For this test, Elastic Agent will need to built without any changes to its dependencies. This will produce an Agent that's using TLSv1.1.

Fleet Server will need to be built with it's elastic-agent-libs dependency temporarily pointing to a local copy that has this PR checked out in it. This will produce a Fleet Server that's using TLSv1.2.

Then it's a matter of enrolling the built Agent with the built Fleet Server and smoke testing that a few common operations in Fleet that would cause back-and-forth communication between Agent and Fleet Server work as expected.

Agent using TLSv1.2 can talk to Fleet Server using TLSv1.1

For this test, Elastic Agent will need to built with it's elastic-agent-libs dependency temporarily pointing to a local copy that has this PR checked out in it. This will produce an Agent that's using TLSv1.2.

Fleet Server will need to be built without any changes to its dependencies. This will produce a Fleet Server that's using TLSv1.1.

Then it's a matter of enrolling the built Agent with the built Fleet Server and smoke testing that a few common operations in Fleet that would cause back-and-forth communication between Agent and Fleet Server work as expected.

@ycombinator ycombinator requested a review from nchaulet May 3, 2024 18:47
@kruskall
Copy link
Member Author

Support for TLS v1.1 is not removed for the sake of compatibility.

this doesn't apply anymore now that we have SetInsecureDefaults. PR Updated

@kruskall
Copy link
Member Author

We might want to release a new version before merging this so people can backport to 8.x, 7.x and call the insecure default func

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @nchaulet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants