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

When certificate_authorities is configured for ServerConfig, we now set client auth to required #12584

Merged
merged 4 commits into from
Jun 17, 2019

Conversation

ph
Copy link
Contributor

@ph ph commented Jun 17, 2019

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

… set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.
@ph ph added the libbeat label Jun 17, 2019
@ph ph requested a review from urso June 17, 2019 20:05
@ph ph requested a review from a team as a code owner June 17, 2019 20:05
@ph ph added the needs_backport PR is waiting to be backported to other branches. label Jun 17, 2019
@ph
Copy link
Contributor Author

ph commented Jun 17, 2019

Need backport to 6.8, 7.0, 7.1 and 7.2

@ph
Copy link
Contributor Author

ph commented Jun 17, 2019

jenkins test this

@@ -99,9 +99,9 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Skipping unparsable log entries from docker json reader {pull}12268[12268]
- Parse timezone in PostgreSQL logs as part of the timestamp {pull}12338[12338]
- Require client_auth by default when ssl is enabled for tcp input {pull}12333[12333]
- Require certificate authorities, certificate file, and key when SSL is enabled for the TCP input. {pull}12355[12355]
Copy link
Contributor Author

@ph ph Jun 17, 2019

Choose a reason for hiding this comment

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

that was not correctly removed from a previous revert.

@graphaelli
Copy link
Member

A corresponding update in the reference configs would be great, eg:

# Configure what types of client authentication are supported. Valid options
# are `none`, `optional`, and `required`. Default is required.
#ssl.client_authentication: "required"

Copy link
Member

@graphaelli graphaelli left a comment

Choose a reason for hiding this comment

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

lgtm

@ph ph merged commit 7f99982 into elastic:master Jun 17, 2019
ph added a commit to ph/beats that referenced this pull request Jun 17, 2019
… set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
@ph ph added v6.8.1 and removed needs_backport PR is waiting to be backported to other branches. labels Jun 17, 2019
ph added a commit to ph/beats that referenced this pull request Jun 17, 2019
… set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
@ph ph added the v7.2.0 label Jun 17, 2019
ph added a commit to ph/beats that referenced this pull request Jun 17, 2019
… set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
@ph ph added the v7.3.0 label Jun 17, 2019
ph added a commit to ph/beats that referenced this pull request Jun 17, 2019
… set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
@ph ph added the v7.1.2 label Jun 17, 2019
ph added a commit to ph/beats that referenced this pull request Jun 17, 2019
… set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
@ph ph added the v7.0.2 label Jun 17, 2019
ph added a commit that referenced this pull request Jun 18, 2019
…ed for ServerConfig, we now set client auth to `required` (#12586)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
ph added a commit that referenced this pull request Jun 18, 2019
…ed for ServerConfig, we now set client auth to `required` (#12585)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
ph added a commit that referenced this pull request Jun 18, 2019
…ed for ServerConfig, we now set client auth to `required` (#12587)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
ph added a commit that referenced this pull request Jun 18, 2019
…ed for ServerConfig, we now set client auth to `required` (#12589)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)
@@ -99,9 +99,9 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Skipping unparsable log entries from docker json reader {pull}12268[12268]
- Parse timezone in PostgreSQL logs as part of the timestamp {pull}12338[12338]
- Require client_auth by default when ssl is enabled for tcp input {pull}12333[12333]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should also be removed with the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, since this PR change that logic it make sense to make sure that the changelog is clear.

ph added a commit to ph/beats that referenced this pull request Jun 18, 2019
Because of TLS changes for client_authentication in elastic#12584, we should
remove the foloowing lines in the changelog to reduce any change of
confusion.

reported by @simitt
ph added a commit that referenced this pull request Jun 18, 2019
Because of TLS changes for client_authentication in #12584, we should
remove the foloowing lines in the changelog to reduce any change of
confusion.

reported by @simitt
ph added a commit that referenced this pull request Jun 18, 2019
…ed for ServerConfig, we now set client auth to `required` (#12588)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit 7f99982)

* fixed changelog
urso pushed a commit to urso/beats that referenced this pull request Jun 21, 2019
…es` is configured for ServerConfig, we now set client auth to `required` (elastic#12587)"

This reverts commit 51566ec.
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…onfigured for ServerConfig, we now set client auth to `required` (elastic#12586)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit c3291b7)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…onfigured for ServerConfig, we now set client auth to `required` (elastic#12588)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit c3291b7)

* fixed changelog
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…onfigured for ServerConfig, we now set client auth to `required` (elastic#12589)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required` (elastic#12584)

* When `certificate_authorities` is configured for ServerConfig, we now set client auth to `required`

When a CA is explicitly set in the configuration options we now default
the client authentication to required.

(cherry picked from commit c3291b7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants