-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation
@@ -436,7 +436,7 @@ def _get_kuksa_val_client(command_line_parser: argparse.Namespace, | |||
client.set_port(config.getint(CONFIG_SECTION_GENERAL, CONFIG_OPTION_PORT)) | |||
|
|||
if config.has_option(CONFIG_SECTION_GENERAL, CONFIG_OPTION_TLS_ENABLED): | |||
client.set_tls(config.getboolean(CONFIG_SECTION_GENERAL, CONFIG_OPTION_TLS_ENABLED, fallback=False)) | |||
client.set_tls(config.getboolean(CONFIG_SECTION_GENERAL, CONFIG_OPTION_TLS_ENABLED, fallback=True)) |
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 assume it make sense to have the most strict alternative as fallback, so if you for instance write Ja
or Nein
it shall be interpreted as True
08bf711
to
2a6533f
Compare
# Shall TLS be used | ||
# Currently KUKSA Server use TLS by default, but KUKSA Databroker does not require TLS by default | ||
tls = True | ||
# tls = False |
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 guess this line can be removed then ...
What's the rational here... It seems like a step backwards in terms of ease of use. I think a more reasonable default would be for the client to connect to the server regardless of whether it (the server) is configured to use TLS or not, but fail if it can't authenticate the server when it's using TLS. That would work seemless with both a default configuration of databroker (no TLS) and with databroker set up with a proper public key infrastructure in place. Edit Edit2 |
The "decision" (or maybe just a conclusion) to use TLS as default also for GRPC/Databroker comes from #133, where both @SebastianSchildt and @sophokles73 seems to agree that TLS (only) shall be default for both gRPC (broker) and ws (Server). As of today at least the gRPC connection fails if you use Concerning default certificates and tokens. For Server the behavior has for a long time been that TLS by default is used/required, and both Server and Client builds (native, docker, PyPI) include default certs and tokens. Previously one reason not to change this would be to avoid breaking backward compatibility for downstream projects. Now when we have released 0.4 that argument may not be that relevant any longer, and I have personally no objection if we would start removing them. But if so one should discuss if default for Broker also shall change. I would personally very much like if we have the same default behavior for both Server and Broker. @SebastianSchildt - I think we need some decisions/guidelines from your side on what to do. |
I need to think about it, or maybe we need some more specific proposals to discuss on Dev meeting. Not matter what the "defaults" and options on the various components may turn out to be, I feel very much opposed to use any kind auf "automagic" functionality somewhere, as that may lead to unexpected downgrades. If default is insecure, and you need to enable TLS -> fine, if default is secure and you need to disable - > also fine. But I don't think we want any client/compoent, that I start today and it goes
Whereas the same code/config tomorrow is doing something like
That I think is unexpected and may lead to deployment disaster. |
Changing this one to draft - seems we need to have a broader discussion/decision on how we want to handle TLS and tokens in our components. As we released 0.4.0 some time ago I see no problem in doing bigger changes, like removing default tokens/certs from existing containers, if someone wants the old behavior they could use 0.4.0. instead of master/main/latest Personally my preferred solution would be to:
|
Closing this one for now |
Practical differences:
Fixes #133
Note: No other changes to support for example TLS as command line parameter, there we need a general discussion first