feat(config/server): Add option for TLS and mTLS server #508
Conversation
Signed-off-by: Anna Simon <asimon@mercari.com>
config_example.yaml
Outdated
| mutualtlsfilespath: "/etc/certs" # folder which will used to store client.crt, client.key and ca.crt files for mutual tls for outputs (default: "/etc/certs") | ||
| tlsserver: | ||
| deploy: false # if true, TLS server will be deployed instead of HTTP | ||
| certfile: "/etc/certs/server.crt" # server certification file | ||
| keyfile: "/etc/certs/server.key" # server key | ||
| mutualtls: false # if true, mTLS server will be deployed instead of TLS, deploy also has to be true | ||
| cacertfile: "/etc/certs/ca.crt" # for client certification if mutualtls is true |
There was a problem hiding this comment.
I'm afraid it will create confusion for end users. We have 2 different mTLS:
- the mTLS used to authenticate falcosidekick with its outputs
- the mTLS used by the falcosidekick's webserver
So we can't use the same folder as they might have conflicts with theca.crt, wdyt?
There was a problem hiding this comment.
I agree it could be confusing that the default path is the same folder. They could rename the ca.crt for the server to something else if they need two different ones though.
One solution I see is that we could use etc/certs/server for the new ones and /etc/certs/client for the already existing ones - this sounds more logical in my mind but this would mean breaking change for those who used the default option.
Other path name that I would suggest is /etc/tls/server and leave the client one as is. Just to have something different and in the end everyone can set it to whatever they want.
There was a problem hiding this comment.
Let me propose you something, it's mostly to avoid to break anything:
/etc/certs/remains as is, for client and not break anything/etc/certs/client/is for client too, withclient.key,client.crtandca.crt/etc/certs/server/is for server, withserver.key,server.crtandca.crt
I'll add a message in the changelog and blog post to explain the /etc/certs is obsolete and will be replaced by /etc/certs/client/ in the falcosidekick-ui in future
There was a problem hiding this comment.
Just to be clear, you mean something like this?
mutualtlsfilespath: "/etc/certs" # folder which will used to store client.crt, client.key and ca.crt files for mutual tls for outputs, will be deprecated in the future (default: "/etc/certs")
mutualtlsclient: # takes priority over mutualtlsfilespath if not emtpy
certfile: "/etc/certs/client/client.crt" # client certification file
keyfile: "/etc/certs/client/client.key" # client key
cacertfile: "/etc/certs/client/ca.crt" # for server certification
tlsserver:
deploy: false # if true, TLS server will be deployed instead of HTTP
certfile: "/etc/certs/server/server.crt" # server certification file
keyfile: "/etc/certs/server/server.key" # server key
mutualtls: false # if true, mTLS server will be deployed instead of TLS, deploy also has to be true
cacertfile: "/etc/certs/server/ca.crt" # for client certification if mutualtls is true
Not sure how to resolve the issue of which one gets priority in this case, this is what I could come up with:
By not setting default value for mutualtlsclient file values for now, it will be set to those values if they are set manually and the mutualtlsfilespath + hardcoded file names if mutualtlsfilespath is set or if it's empty (thus backfalls to the default /etc/certs value.
There was a problem hiding this comment.
Ok with your solution.
For the mutualtls, do you think we could enforce deploy if it's true?
There was a problem hiding this comment.
Great!
I think that would be confusing eg. if I turn off the deploy for debugging, then I would expect that the whole TLS part is turned off, but it would still be on. Maybe I could set a warning log message for the deploy == false && mutualtls == true case.
There was a problem hiding this comment.
Added the modifications in a new commit, let me know if it's okay with you or I should change something!
…lTLS Signed-off-by: Anna Simon <asimon@mercari.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: annadorottya, Issif The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. Git tree hash: f0cede59caa40fcc57927e1471b0805a5f97c5bd
|
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area config
What this PR does / why we need it: Currently only HTTP server can be deployed. It would be more secure if we could deploy TLS or mutualTLS server if wanted.
Which issue(s) this PR fixes: #507
Fixes #
Special notes for your reviewer:
The logic for config is a bit different than the one already used. I decided to add 3 config options as full paths for the 3 files needed. I feel this way it's more customizable and does not collide with the
mutualtlsfilespathconfig. Eg. this way we could set it to the same folder but different file names, or a different folder, or reuse the files completely.