Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Security auto-configuration for packaged installations #75144
Security auto-configuration for packaged installations #75144
Changes from 48 commits
7d9b6f5
2598dd3
9c29107
e566c3e
d104c9b
38ba6b5
6fd7123
3ecbb0f
576b608
d566434
3aab03e
21fcc13
6d924e5
6fd451e
0d4ad2f
1239ab7
75c26e2
b88f1ec
0113694
dcc40e3
776a155
882c191
f0b1a38
fd4e4f9
ea8dfb9
26ec0e3
072a672
b58fe80
6cc80bf
f6a313d
c1fa865
776de7c
3a37699
f6ecd0d
5c732e2
680503f
5fb7152
bc1c78a
1e702a5
076f1f9
c88cbc6
72fc1fd
a97c469
7b70758
7a48d74
19df189
3bf3962
9a2f6ef
7bbb58d
99485ea
7db3187
9ed95b1
2ab4d37
9f81dab
5f65641
423dbd6
639f5fd
f73aa25
d7e8cf4
3c8416c
5f173f9
15e1eff
2795c3a
bc28f81
d0a1122
48c7c80
d0c9270
d0ad3c1
c75dda6
71bd34d
fa4b93e
83d4036
cce8094
9c82a71
7afeee2
169b3e7
a05bb85
b66c345
230a364
26ad69a
f9b74fd
4360d08
b0cbdfc
1feac33
5dc4258
c05732d
4dcbc84
4f0a743
3535b00
26e7811
7dcfd2f
f86e6a8
11834f1
bdcd267
d83fd6c
d90104b
9fd5e77
0ef54aa
da5e3de
c29e3e7
c8785c4
1498131
84046fd
cd8c285
182c45d
8ac2890
1e439a6
48da88c
dd5f118
9791b77
71c934e
bc68b30
9cd79e0
69990d1
6221408
a754c1e
c150a08
3f1d58a
73be68f
a69986c
c55d9dd
deafd2d
ec3c51f
6ccf723
c1771d8
749ba9b
084b2f9
a8f7c47
a5baae0
28bca9e
90a9022
90bd0f6
7d2219c
4449fd5
9ecc182
9781eba
9145898
ea5932d
a781149
ae591d3
10f3474
6076aab
674202d
98e3ed9
ccd1dbe
c2092d8
0b89640
e8b390c
93abfb2
ed8646f
1a3220d
42d88e9
6ebcebc
405c60d
c3124d2
53443bd
5645b7d
125480c
8decc71
4a9f987
41d6067
1e4648f
8ac813e
a0cd8de
de20c38
ba6720c
6cde022
5798743
38df918
ff48940
7b86021
820fd28
b59c55f
b070178
b25cc0a
f4e784f
8fb6362
14e1f36
a7e4c49
4128d61
59c2542
299c710
1b2eed3
f908c25
c5eeb4f
d646a3b
dcb6ee5
b0377e1
a33a1da
6f2be87
c83160d
4cf3243
4a7e626
0d75418
53e6a2d
b28bd52
be9eefa
bc8d9c5
1acd0cc
8df3cbf
35b9845
6f752ab
ea66082
a8337c2
622dffd
507b3f6
df561e8
514a240
0ca655b
ba30af4
4d1658b
dd2f567
e175b5c
99b649c
178580b
99b4ad0
5d793a3
dbf7250
8bda57f
c3547ec
e3d0825
8abbf94
1e981e0
7e817d0
ceca0f1
cbf60bd
af4d838
2089d1c
6e72adb
27a655c
05c2efe
bae98d3
738eb2e
c72b48d
40a05af
5e6db81
0f2d589
f721f19
a8eeff9
f0f7b65
c3c6329
3b0e7ff
a77460f
211391e
bbb38b9
ffc4f88
ffce3d3
bda567b
7cdf1e9
2e19810
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke with @colings86 about this today and we may want to revisit this decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which decision is that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That if we fail to perform auto-configuration (can't write to file) that we might want to fail instead of ignore. Something about "leniency" and being "abhorent", blah blah blah 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like aphorisms as the next person but this is not about leniency. We ( and by we I mean mostly @albertzaharovits ) have spent a lot of time thinking about the cases where we should fail and when we should not. This is written in code in #74868 and Albert has the task to write this down to the design doc too. In short, we consider the case where we can't write to the config file because it is set as read-only, as a case where we have an explicit configuration decision by the user ( the file doesn't get to be read only by its own). As such we have elected to not treat this as an error and fail to start the node ( and in doing so introduce a breaking change as we allow this today ) but simply not attempt to auto-configure security for them. Authentication will be enabled but we won't enable and configure TLS. Happy to reconsider or discuss alternative approaches, @colings86 feel free to bring this up for discussion in the project or we can chat anytime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really rather not continue to add test coverage as assertions to
verifyInstallation()
. We should implement this as a new distinct test suite. We also need to add a lot more coverage here for all the other scenarios as this only covers the "happy path". For example:elasticsearch.yml
file is non-writableThere are likely other more specific scenarios that probably are better implemented as unit tests for the CLI tool for which no tests currently yet exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, I'll make sure I'll add test coverage before I raise this for review.
What is under debate regarding this ?
Makes sense, I think it's better if we keep comments targeted in relevant PRs so that we don't miss anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mentioned, @colings86 and I briefly talked about possibly erroring on startup if we fail to auto-config security rather than pressing forward. No decisions were made but we might want to reconfirm that this is indeed the behavior we want.