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

Change dbc2val TLS default to True #135

Closed
wants to merge 1 commit into from

Conversation

erikbosch
Copy link
Contributor

@erikbosch erikbosch commented Sep 5, 2023

Practical differences:

  • If use default config for your Databroker instance (no TLS) then you cannot use dbcfeeder defult config (TLS), i.e. just must either change databroker config to use TLS or change dbcfeeder config to not use TLS

Fixes #133

Note: No other changes to support for example TLS as command line parameter, there we need a general discussion first

@@ -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))
Copy link
Contributor Author

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

@erikbosch
Copy link
Contributor Author

FYI @sophokles73 @SebastianSchildt

# Shall TLS be used
# Currently KUKSA Server use TLS by default, but KUKSA Databroker does not require TLS by default
tls = True
# tls = False
Copy link
Member

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 ...

@argerus
Copy link
Contributor

argerus commented Sep 8, 2023

Practical differences:
If use default config for your Databroker instance (no TLS) then you cannot use dbcfeeder defult config (TLS), i.e. just must either change databroker config to use TLS or change dbcfeeder config to not use TLS

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
An option to require TLS can then be added (e.g. --require-tls) if the client should not accept connecting to an unauthenticated server at all.

Edit2
And having a server ship with a self signed (publicly available) certificate as the default configuration doesn't add any security at all. It does make it harder for clients to use though, which is why I don't think it makes sense in the first place.

@erikbosch
Copy link
Contributor Author

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 tls = False on client side but the server use TLS, i.e. grpc.insecure_channel will fail if the Broker offer only a TLS connection. So to be able to accept both secure and insecure connection if TLS=false would require some changes, for example first test secure and if it fails test insecure (unless tls=true is given)

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.

@SebastianSchildt
Copy link
Contributor

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

  • "Let me try TLS, as that is best"
  • "Ah works. Awesome

Whereas the same code/config tomorrow is doing something like

  • "Let my try TLS, as I know that is best"
  • "Aw fiddlestticks, doesn't work at all... let me try plain... nobody explicitly told me not to...."
  • "Awesome works!"

That I think is unexpected and may lead to deployment disaster.

@erikbosch erikbosch marked this pull request as draft September 12, 2023 07:04
@erikbosch
Copy link
Contributor Author

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:

  • Phase out the argument insecure (for those components who use it) as it is not really clear by the name what it means, I prefer specific arguments like tls=False or --no-tls.
  • Make TLS and token authorization default, and do not include default certs/token at all. I.e. make it required that you either must specify certs/key/tokens or use --no-tls, --no-authorization or similar.

@erikbosch
Copy link
Contributor Author

Closing this one for now

@erikbosch erikbosch closed this Oct 27, 2023
@erikbosch erikbosch deleted the erik_tls_default branch February 27, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DBC Feeder uses TLS for connection to kuksa.val Server by default
4 participants