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

Remove tls.server.supported_ciphers field #662

Merged
merged 2 commits into from
Dec 3, 2019

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Dec 2, 2019

Unlike the client, the server doesn't provide a list of supported ciphers in its hello message.

Unlike the client, the server can't provide a list of it's supported
ciphers in its hello message.
@adriansr adriansr added the review label Dec 2, 2019
@adriansr adriansr requested a review from webmat December 2, 2019 22:29
@ruflin
Copy link
Contributor

ruflin commented Dec 3, 2019

@webmat Was this already released?

@webmat
Copy link
Contributor

webmat commented Dec 3, 2019

@ruflin Yes it was.

Since it's a field that doesn't make sense, I propose we release ECS 1.3.1 with only this removal in it.

@dcode or @dainperkins you guys agree this field doesn't make sense, right?

@webmat webmat requested a review from dcode December 3, 2019 14:23
@webmat webmat self-assigned this Dec 3, 2019
@webmat
Copy link
Contributor

webmat commented Dec 3, 2019

@ruflin To clarify.

During the exchange, the client says which ciphers it supports -- captured at tls.client.supported_ciphers.

The server picks which one to chose, and the selected cipher should be captured in tls.cipher.

At no point are there server side "supported ciphers" we can capture. I think this was a copy/pasta mistake ;-)

Copy link
Contributor

@dcode dcode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the quick fix @adriansr!

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

@webmat webmat merged commit 94e7376 into elastic:master Dec 3, 2019
webmat pushed a commit to webmat/ecs that referenced this pull request Dec 3, 2019
Unlike the client, the server doesn't provide a list of supported ciphers in its hello message.
webmat pushed a commit to webmat/ecs that referenced this pull request Dec 3, 2019
Unlike the client, the server doesn't provide a list of supported ciphers in its hello message.
webmat pushed a commit that referenced this pull request Dec 3, 2019
Unlike the client, the server doesn't provide a list of supported ciphers in its hello message.
dcode pushed a commit to dcode/ecs that referenced this pull request Apr 15, 2020
Unlike the client, the server doesn't provide a list of supported ciphers in its hello message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants