eclipse-kuksa / kuksa.val.feeders

kuksa.val.feeders
Apache License 2.0
8 stars 22 forks source link

Change dbc2val TLS default to True #135

Closed erikbosch closed 1 year ago

erikbosch commented 1 year ago

Practical differences:

Fixes #133

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

erikbosch commented 1 year ago

FYI @sophokles73 @SebastianSchildt

argerus commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

erikbosch commented 1 year ago

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:

erikbosch commented 1 year ago

Closing this one for now