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.

Sign up for GitHub

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

Jump to bottom

docs: Update cipher suites documentation #20697

Merged
merged 1 commit into from
Aug 31, 2020
Merged

docs: Update cipher suites documentation #20697

merged 1 commit into from
Aug 31, 2020

Conversation

bmorelli25
Copy link
Member

@bmorelli25 bmorelli25 commented Aug 19, 2020 •
edited
Loading

What does this PR do?

This PR:

  • Updates the cipher_suites documentation to better display TLS version compatibility.
  • Adds a tagged region around the content so that it can be reused in APM Server's input (agent ←→ server) documentation. Note that I attempted to list TLS versions for each cipher, but it made the content tougher to scan. I think it works best to only note the ciphers that are compatible with 1.2.
  • Links to Go's crypto library default cipher suites. Is this the correct place to link?

Why is it important?

This content previously only existed in the APM Server's output (server ←→ es) documentation.

Open questions

Related issues

For elastic/apm-server#4002.

Screenshots

Screen Shot 2020-08-19 at 3 31 58 PM

@bmorelli25 bmorelli25 requested a review from a team August 19, 2020 22:36
@bmorelli25 bmorelli25 self-assigned this Aug 19, 2020
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 19, 2020
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #20697 opened]

  • Start Time: 2020-08-19T22:37:36.334+0000

  • Duration: 23 min 11 sec

@andresrc andresrc added the Team:Services (Deprecated) Label for the former Integrations-Services team label Aug 20, 2020
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 20, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@andresrc andresrc added the Team:Docs Label for the Observability docs team label Aug 24, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/obs-docs (Team:Docs)

EamonnTP
EamonnTP approved these changes Aug 24, 2020
View reviewed changes
Copy link

@EamonnTP EamonnTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bmorelli25
Copy link
Member Author

Chatted with @andrewvc about adding the following ciphers to the documentation:

beats/libbeat/common/transport/tlscommon/types.go

Lines 69 to 71 in 5f42d21

"TLS-AES-128-GCM-SHA256": tlsCipherSuite(tls.TLS_AES_128_GCM_SHA256),
"TLS-AES-256-GCM-SHA384": tlsCipherSuite(tls.TLS_AES_256_GCM_SHA384),
"TLS-CHACHA20-POLY1305-SHA256": tlsCipherSuite(tls.TLS_CHACHA20_POLY1305_SHA256),

We agreed they should be added, but now I'm noticing that TLS 1.3 cipher suites are not individually configurable in Go... So I don't think they need to be added after all.

@andresrc andresrc requested review from andrewvc and kvch August 25, 2020 15:21
@bmorelli25
Copy link
Member Author

Bump @elastic/integrations-services. Can I get a review on this, please? I'm specifically interested in the two open questions in the description.

@andrewvc
Copy link
Contributor

@bmorelli25 I added the cipher suites there not so much to make them configurable but so that inverse lookups would work (we flip that map around and use it for inverse lookups). We use these inverse lookups to report the cipher suite used in heartbeat data. If they aren't configurable maybe the docs should reflect that, but the code should stay.

@bmorelli25
Copy link
Member Author

bmorelli25 commented Aug 31, 2020 •
edited
Loading

Neat. The docs say "Note that TLS 1.3 cipher suites are not individually configurable in Go, so they are not included in this list.", so I think we're good then! Thanks, Andrew.

EDIT: Sorry, I reread and realize I wasn't clear in my comment above. I wasn't suggesting we remove them from the code, just that we exclude them from the docs.

@bmorelli25 bmorelli25 merged commit 1e05957 into elastic:master Aug 31, 2020
@bmorelli25 bmorelli25 deleted the docs-cipher branch August 31, 2020 22:03
This was referenced Aug 31, 2020
Merged
Merged
bmorelli25 added a commit to bmorelli25/beats that referenced this pull request Aug 31, 2020
@bmorelli25
docs: update cipher suites (elastic#20697)
9f0d215
bmorelli25 added a commit to bmorelli25/beats that referenced this pull request Aug 31, 2020
@bmorelli25
docs: update cipher suites (elastic#20697)
2e21304
@bmorelli25 bmorelli25 mentioned this pull request Sep 1, 2020
Merged
1 task
bmorelli25 added a commit that referenced this pull request Sep 1, 2020
@bmorelli25
docs: update cipher suites (#20697) (#20880)
b897076
bmorelli25 added a commit that referenced this pull request Sep 1, 2020
@bmorelli25
docs: update cipher suites (#20697) (#20879)
f74a98f
v1v added a commit to v1v/beats that referenced this pull request Sep 2, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/ci-pipeli…
1faa11d 
…ne-2.0

* upstream/master: (87 commits)
  [packaging] Normalise GCP bucket folder structure (elastic#20903)
  [Metricbeat] Add billing metricset into googlecloud module (elastic#20812)
  Include python docs in devguide index (elastic#20917)
  Avoid generating incomplete configurations in autodiscover (elastic#20898)
  Improve docs of leaderelection configuration (elastic#20601)
  Document how to set the ES host and Kibana URLs in Ingest Manager (elastic#20874)
  docs: Update beats for APM (elastic#20881)
  Adding cborbeat to community beats (elastic#20884)
  Bump kibana version to 7.9.0 in x-pack/metricbeat (elastic#20899)
  Kubernetes state_daemonset metricset for Metricbeat (elastic#20649)
  [Filebeat][zeek] Add new x509 fields to zeek (elastic#20867)
  [Filebeat][Gsuite] Add note about admin in gsuite docs (elastic#20855)
  Ensure kind cluster has RFC1123 compliant name (elastic#20627)
  Setup python paths in test runner configuration (elastic#20832)
  docs: Add  `processor.event` info to Logstash output (elastic#20721)
  docs: update cipher suites (elastic#20697)
  [ECS] Update ecs to 1.6.0 (elastic#20792)
  Fix path in hits docs (elastic#20447)
  Update filebeat azure module documentation (elastic#20815)
  Remove duplicate ListGroupsForUsers in aws/cloudtrail (elastic#20788)
  ...
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
@bmorelli25 @melchiormoulin
docs: update cipher suites (elastic#20697)
19c9ed4
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@bmorelli25
docs: update cipher suites (elastic#20697) (elastic#20879)
6fd5a14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EamonnTP EamonnTP EamonnTP approved these changes

@andrewvc andrewvc Awaiting requested review from andrewvc

@kvch kvch Awaiting requested review from kvch

Assignees

@bmorelli25 bmorelli25

Labels
docs Team:Docs Label for the Observability docs team Team:Services (Deprecated) Label for the former Integrations-Services team v7.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bmorelli25 @elasticmachine @andrewvc @EamonnTP @andresrc