Add working --serveTLS #272

ineiti · 2023-09-26T14:25:03Z

Instead of using --noTLS to indicate not to use TLS, changed the flag to --serveTLS. Now:

if a certificate and a chain is given, --serveTLS is enabled
if --serveTLS is enabled w/o a certificate, a self-signed certificate is created
if the remote point starts with https, a TLS connection is established, and the certificate is verified using public CAs used in the system

pierluca

I have some serious doubts about these changes.

I might have missed something (it's late and it was a long day), but if I understand correctly, we're introducing (or revamping) insecure defaults, and the solution to handling reverse proxies is removing a MitM protection, relying entirely on certificate authorities to verify remote node certificates.

If my understanding is correct, this would (at the very least) break a few of our own internal (experimental) deployments, which aren't always exposed to the internet. It would also make it harder to catch configuration mistakes.

I'm happy to have a quick (offline) chat about this if it helps move the functionality along.

pierluca · 2023-09-26T17:29:09Z

mino/minogrpc/controller/mod.go

+			Name:     "serveTLS",
+			Usage:    "enables TLS on the gRPC server and creates a self-signed certificate if necessary",
 			Required: false,
 			Value:    false,


effectively, this makes it insecure by default. I'm not sure this is a great idea.

pierluca · 2023-09-26T17:30:38Z

mino/minogrpc/controller/mod.go

@@ -77,7 +77,7 @@ func (m miniController) SetCommands(builder node.Builder) {
 		},
 		cli.StringFlag{
 			Name:     "certKey",
-			Usage:    "provides the certificate private key path",


If I understand correctly, it becomes impossible to be certificate-aware (e.g., if nginx acts as reverse proxy to do TLS-termination) without also serving TLS. I'm not sure this is desirable.

pierluca · 2023-09-26T17:32:32Z

mino/minogrpc/controller/mod.go

+		if certChain != "" {
+			fmt.Println("certChain:", certChain, "certKey:", certKey)
+			cert, err := tls.LoadX509KeyPair(certChain, certKey)


why would we only load this if certKey isn't set ? 🤔

pierluca · 2023-09-26T17:33:04Z

mino/minogrpc/server.go

+	"google.golang.org/grpc/credentials/insecure"
+	"strings"


we're using goimports formatting, which isn't respected here.

I'm never sure what Goland does, sorry. I'll try to convince it to use goimports. Do you have a common formatter you're using?

pierluca · 2023-09-26T17:35:06Z

mino/minogrpc/controller/actions.go

+	fmt.Fprintf(req.Out, "--token %s%s\n",
+		token, certHash)


If I understand correctly, the certHash here may be empty.
This gives no way for the joining node to authenticate the node it is connecting to.
That does not seem desirable.

pierluca · 2023-09-26T17:38:39Z

mino/minogrpc/mod.go

+	if !tmpl.serveTLS && public.Scheme != "https" {
+		dela.Logger.Warn().Msg("⚠️ running in insecure mode and no TLS endpoint, you should not " +
+			"publicly expose the node's socket without TLS")


Technically grpcs could also be a valid scheme, I believe.
Furthermore, a misconfiguration of the public address could hide this error, shouldn't we be (slightly) louder and let the sysadmin figure it out if the warning applies to them or not ?

remove check for public.Scheme and reword warning for students

pierluca · 2023-09-26T17:40:23Z

mino/minogrpc/mod.go

+	serveTLS bool
+}
+
+func (m *minoTemplate) makeCertificate() error {


I know this somewhat existed before, but is it a good idea to generate a certificate when a misconfiguration could be the reason the certificate wasn't passed ? 🤔

I'd consider making it an option, e.g. --generate-certificate, rather than a fallback that might entirely hide that there was a problem somewhere, and make the sysadmin's task harder.

I only moved this from the overlay to minoTemplate to simplify newOverlay.

pierluca · 2023-09-26T17:45:09Z

mino/minogrpc/server.go

+		// If the remote is accessible via port 443, we suppose it is TLS terminated and signed by a
+		// CA available on the system.


this assumption might easily break in our own deployments.
that was the whole point of passing the certificate hash along with the token when joining nodes 😕

pierluca · 2023-09-26T17:47:52Z

mino/minogrpc/server.go

+	if addr.Port() == "" {
+		switch addr.Scheme {
+		case "http":
+			host += ":80"
+		case "https":
+			host += ":443"
+		default:
+			return xerrors.Errorf("address doesn't contain a port and uses "+
+				"an unknown scheme: %s", addr.Scheme)
+		}
+	}


why is this needed ? Effectively the code is adding default ports for those schemes, that's normally not needed 🤔

pierluca · 2023-09-26T17:49:42Z

mino/minogrpc/server.go

-	if tmpl.cert != nil && tmpl.useTLS {
-		chain := bytes.Buffer{}
-		for _, c := range tmpl.cert.Certificate {
-			chain.Write(c)
-		}
-
-		err := o.certs.Store(o.myAddr, chain.Bytes())
-		if err != nil {
-			return nil, xerrors.Errorf("failed to store cert: %v", err)
-		}
-	}
-
-	if tmpl.useTLS {
-		cert, err := o.certs.Load(o.myAddr)
-		if err != nil {
-			return nil, xerrors.Errorf("while loading cert: %v", err)
-		}
-
-		if cert == nil {
-			err = o.makeCertificate()
-			if err != nil {
-				return nil, xerrors.Errorf("certificate failed: %v", err)
-			}
-		}


good call on moving all this stuff ahead of the overlay creation, it feels cleaner !

ineiti · 2023-09-27T08:22:30Z

Use cases

Currently I see two use-cases, and a third for paranoid people:

Use dela behind a reverse proxy with TLS termination
Use dela in development environment
Use dela standalone with self-signed certificates

For 1. I think we should trust the reverse proxy to do its thing. Of course there are attacks on CAs with rogue certificates where you can have a mitm. If you care about this attack, go for 3. But using TLS-in-TLS seems overly complicated, and IIRC, was one of the problems with the last try to get it to run.

For 2., no TLS is needed. It's development, it should be fast and w/o fuss. Every argument saved in development mode is a happy developer.

For 3., use self-signed certificates and serve TLS with dela. Or set up a reverse proxy with terminating TLS and trust it, but then the reverse proxy could be colluding with your adversary...

So with this PR, 1. and 2. is the default. No TLS served by default, for deployment we use reverse proxies.

If you're paranoid and want to be really sure nobody is interfering with your communication, go for 3.

Serving TLS in dela

Perhaps the arguments could be reduced to --certKey and --certChain, which must either both be given, or none. If they are present, TLS is served. For convenience, we could add a --makeCertificate which writes the keys and the chain in the files given by the other two arguments. But if you're paranoid, perhaps you want to use another tool to create your certificate.

Do we need `--certKey` AND `--certChain`?

I'm always lost with certificates and never know which contain the private key and which contain the public key. But I think that --certChain doesn't make sense: when you Join to a TLS serving dela, you have to give the hash of its certificate anyway. So why should you add it? Or is the --certChain for the certificate of the --certKey?

History

In cothority we also started serving TLS in the conodes for the websockets, but this turned out to be complicated with renewing and reloading of certificates. It's still there, but mostly unused.

We kept self-signed certificates only for the node-to-node communication. Which is not ideal, because you need a dedicated port and cannot use a reverse proxy for that. Or at least I never tried.

ineiti · 2023-09-27T11:49:16Z

After discussion:

By default:

makeCertificate and listen on TLS

Or one of:

give --certKey
give both --certKey and --certChain
--noTLS to remove listening on TLS

ineiti · 2024-03-27T14:21:06Z

Done in #280 and #282

Refactoring and fixing some comments

a5db0a0

ineiti self-assigned this Sep 26, 2023

ineiti requested review from lanterno and PascalinDe September 26, 2023 14:25

ineiti added 2 commits September 26, 2023 16:28

Replacing --noTLS with --serveTLS

93fd036

Fix code to correctly serve and connect to TLS / plain endpoints

4380828

ineiti force-pushed the fix-notls branch from f164e5b to 4380828 Compare September 26, 2023 14:28

pierluca reviewed Sep 26, 2023

View reviewed changes

ineiti mentioned this pull request Sep 28, 2023

Fix notls c4dt/dela#3

Merged

ineiti closed this Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add working --serveTLS #272

Add working --serveTLS #272

ineiti commented Sep 26, 2023

pierluca left a comment

pierluca Sep 26, 2023

pierluca Sep 26, 2023

pierluca Sep 26, 2023

pierluca Sep 26, 2023

ineiti Sep 27, 2023

pierluca Sep 26, 2023

pierluca Sep 26, 2023

ineiti Sep 27, 2023

pierluca Sep 26, 2023

ineiti Sep 27, 2023

pierluca Sep 26, 2023

pierluca Sep 26, 2023

pierluca Sep 26, 2023

ineiti commented Sep 27, 2023

ineiti commented Sep 27, 2023

ineiti commented Mar 27, 2024

		// If the remote is accessible via port 443, we suppose it is TLS terminated and signed by a
		// CA available on the system.

Add working --serveTLS #272

Add working --serveTLS #272

Conversation

ineiti commented Sep 26, 2023

pierluca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ineiti commented Sep 27, 2023

Use cases

Serving TLS in dela

Do we need --certKey AND --certChain?

History

ineiti commented Sep 27, 2023

ineiti commented Mar 27, 2024

Do we need `--certKey` AND `--certChain`?