Added TLS to WebSocket by ValentinMoullet · Pull Request #405 · dedis/onet

ValentinMoullet · 2018-04-19T16:07:01Z

Added 2 fields in private.toml for setting the WebSocket TLS certificate and key, if wanted.
WebSocket is now using ListenAndServeTLS using the TLS configuration found in the private.toml if given.
Client (in websocket.go) can now communicate in TLS, if asked.

In the private.toml file, we can set the 2 WebSocket TLS fields using 2 different ways:

string://{THE CONTENT OF THE CERTIFICATE}
file://{THE PATH OF THE FILE CONTAINING THE CERTIFICATE}

Fixes #383

ineiti · 2018-04-23T06:07:00Z

websocket_test.go

 }

+// Adapted from 'https://golang.org/src/crypto/tls/generate_cert.go'
+func generateSelfSignedCert() (string, string, error) {


Please simplify this method to only use what you're giving in the var-part.

ineiti · 2018-04-23T06:07:29Z

websocket_test.go


+// Adapted from 'https://golang.org/src/crypto/tls/generate_cert.go'
+func generateSelfSignedCert() (string, string, error) {
+	var (


var is very useful outside of methods, but not needed inside. You can directly write:

host := "127.0.0.1"

ineiti · 2018-04-23T06:07:49Z

websocket_test.go

+
+	var priv *ecdsa.PrivateKey
+	var err error
+	switch ecdsaCurve {


remove case

ineiti · 2018-04-23T06:07:56Z

websocket_test.go

+	var notBefore time.Time
+	if len(validFrom) == 0 {
+		notBefore = time.Now()
+	} else {


only keep else

ineiti · 2018-04-23T06:08:26Z

websocket_test.go

+	}
+
+	hosts := strings.Split(host, ",")
+	for _, h := range hosts {


remove for - there is only one host.

ineiti · 2018-04-23T06:08:37Z

websocket_test.go

+
+	hosts := strings.Split(host, ",")
+	for _, h := range hosts {
+		if ip := net.ParseIP(h); ip != nil {


only keep this if

ineiti · 2018-04-23T06:09:38Z

websocket_test.go

+		Val:              10,
+	}
+	sr := &SimpleResponse{}
+	assert.Equal(t, uint64(0), client.Rx())


I think you need require here, else the test will continue, even if this fails.

jeffallen · 2018-04-25T13:35:33Z

I found a way to download the patch: https://patch-diff.githubusercontent.com/raw/dedis/onet/pull/405.patch

See:
https://stackoverflow.com/questions/6188591/download-github-pull-request-as-unified-diff#6188624

jeffallen · 2018-04-25T14:50:22Z

local.go


+// newTCPServerWithWebSocketTLS creates a new server with a tcpRouter with
+// "localhost:"+port as an address and configure TLS for its websocket.
+func newTCPServerWithWebSocketTLS(s network.Suite, port int, path string,


This is repeated code with respect to newTCPServer, right? Better would be to move the h.Start() and listening check out, and then call newTCPServer in both the TLS and non-TLS case. In the TLS case, add a call to SetWebsocketTLS.

Private methods are private for a reason: you are free to hack on them as much as necessary in order to make your new feature fit in without massive copy/paste. Becaues those copy/pastes become a huge anchor around our neck later.

jeffallen · 2018-04-25T14:51:22Z

server.go

 }

+// SetWebsocketTLS sets the TLS configuration for its websocket.
+func (c *Server) SetWebsocketTLS(tlsCertificate []byte, tlsCertificateKey []byte) error {


The type is WebSocket (capital S) so all methods that talk about it should use the same capitalization.

jeffallen · 2018-04-25T15:01:29Z

websocket.go

+	if err != nil {
+		return err
+	}
+	w.server.Server.TLSConfig = &tls.Config{Certificates: []tls.Certificate{cert}}


With this API, you make it impossible for the caller to customize the tls.Config if they need to (and we will need to, in order to support LetsEncrypt via the golang.org/x/crypto/acme/autocert package) Another way to do this would be for type WebSocket to have a field called TLSConfig, with a comment saying, "can only be modified before Start is called". Then in app/config.go, instead of calling server.SetWebsockerTLS, you'd set server.WebSocket.TLSConfig as needed. the private field websocket in type onet.Server would need to be exported, but I see no problem with that.

Go prefers correct defaults, and empowering the caller to override them by sending in custom configs. And on balance, a reasonable new variable export (onet.Server.websocket) is better than two new exported APIs.

jeffallen · 2018-04-25T15:05:43Z

websocket.go

+	tls bool
+	// Pool of certificates we trust, can be used for self-signed certificates;
+	// only useful if tls is set to true.
+	trustedCertificates *x509.CertPool


Same as the server side:

This makes it impossible for callers to change the tls.Config if they want to. Instead, give them a TLSClientConfig field they can set, and then in Send(), do d := &websocket.Dialer{ TLSClientConfig: c.TLSClientConfig }

jeffallen · 2018-04-25T15:06:54Z

websocket.go

+		}
+
+		var wsProtocol string
+		if c.tls {


I do not see how the caller makes c.tls == true.

jeffallen · 2018-04-25T15:07:52Z

websocket.go

+			wsProtocol = "ws"
+		}
+		serverURL := fmt.Sprintf("%s://%s/%s/%s", wsProtocol, url, c.service, path)
+		headers := http.Header{"Origin": []string{"http://" + url}}


Should this http be https if wsProtocol is wss?

jeffallen · 2018-04-25T15:15:02Z

app/config.go

+
+const (
+	// String is a CertificateURL type containing a certificate.
+	String CertificateURLType = "string"


Another proposal: the type should default to file, so that WebSocketTLSCertificate = "/etc/conode.crt" will do what any reasonable person would expect.

But since it is already here, WebSocketTLSCertificate = "string:XXX" should work too. (Although it is unclear that YAML can accept the embedded newlines that you typically find in PEM-encoded cert files.)

I do like the type:blob format, because it will help with LetsEncrypt. We can add LetsEncrypt support here like this: WebSocketTLSCertificate = "le:hostname.example.com" (LE requires that the server know what it's public FQDN is at the moment it requests the certificate.)

I see! Yes I can do that. Also, are you saying we should change the delimiter from "://" to ":", or is "://" still fine?

No, that's a mistake. I propose that the following work:

"filename.pem"

"file://filename.pem"

"string://certificate-goes-here"

"letsencrypt://FQDN-goes-here" (later, in another commit)

jeffallen · 2018-04-25T15:16:12Z

app/config.go

+	Address                    network.Address
+	ListenAddress              string
+	Description                string
+	WebSocketTLSCertificate    CertificateURL


There needs to be some new code in config_test.go proving that YAML can handle string:foo\nbar\nbaz correctly, because PEM-encoded certs have newlines in them.

I guess you mean TOML, not YAML. But that's a fair concern. By checking the doc (https://github.com/toml-lang/toml) it seems it should not be an issue, right? I can still write a test for that (ParseCothority), since there isn't a test for loading and parsing the private.toml file in config_test.go anyway.

jeffallen · 2018-04-25T15:28:16Z

We also chatted on Slack about the fact that "go test -v" fails while "go test" succeeds.

…mments on generating self-signed certificate.

…bSocket TLS certificate type set to 'file' + added test for ParseCothority.

jeffallen · 2018-05-02T08:35:15Z

app/config_test.go

+	certFile.WriteString(wsTLSCert)
+	certFile.Close()
+
+	keyFile, err := ioutil.TempFile("", "temp_key.pem")


from godoc for TempFile: "It is the caller's responsibility to remove the file when no longer needed" See: https://godoc.org/io/ioutil#example-TempFile

Ohh right! I was in fact looking at examples in our codebase and didn't see those removed, thus I assumed it was automatically done. For example, if you look in platform_test.go and search for ioutil.TempFile, are any of those removed in any way? Or should we add an issue to fix those?

Yes, you can add an issue, thanks.

ValentinMoullet self-assigned this Apr 19, 2018

ineiti reviewed Apr 23, 2018

View reviewed changes

websocket_test.go Outdated

var priv *ecdsa.PrivateKey

var err error

switch ecdsaCurve {

Copy link

Member

ineiti Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove case

ineiti reviewed Apr 23, 2018

View reviewed changes

websocket_test.go Outdated

var notBefore time.Time

if len(validFrom) == 0 {

notBefore = time.Now()

} else {

Copy link

Member

ineiti Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only keep else

ineiti reviewed Apr 23, 2018

View reviewed changes

websocket_test.go Outdated

}

hosts := strings.Split(host, ",")

for _, h := range hosts {

Copy link

Member

ineiti Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove for - there is only one host.

ineiti reviewed Apr 23, 2018

View reviewed changes

ValentinMoullet force-pushed the onet_fork/issue_383 branch from f97b92f to 0c26f50 Compare April 25, 2018 13:39

jeffallen suggested changes Apr 25, 2018

View reviewed changes

ValentinMoullet force-pushed the onet_fork/issue_383 branch from 0c26f50 to 7907072 Compare May 2, 2018 00:01

ValentinMoullet added 7 commits May 2, 2018 02:04

Added TLS to websocket.

5ebcd9e

Added TLS to websocket with testing.

73868e5

Replaced log.ErrFatal by require.Nil when possible.

1bfda43

Added source URL for generating self-signed TLS certificate.

f5a5ddf

Removed log.ErrFatal for ListenAndServe and ListenAndServeTLS.

6c39351

Changed 'assert' to 'require' in 'websocket_test.go' + assessed PR co…

508f134

…mments on generating self-signed certificate.

Assessed Jeff's comments for enhancing WebSocket TLS API + default We…

7907072

…bSocket TLS certificate type set to 'file' + added test for ParseCothority.

jeffallen suggested changes May 2, 2018

View reviewed changes

ValentinMoullet added 2 commits May 2, 2018 19:23

Be sure to remove temp files when creating them.

ab4edec

Formatting.

a3eff79

jeffallen approved these changes May 3, 2018

View reviewed changes

jeffallen merged commit 2111ca6 into dedis:master May 3, 2018

ValentinMoullet deleted the onet_fork/issue_383 branch May 3, 2018 09:38

jeffallen mentioned this pull request Sep 21, 2018

Conodes should be available through wss dedis/cothority#1486

Closed

ineiti mentioned this pull request Jan 22, 2020

ServerIdentity.ID shouldn't rely on String #617

Open

Conversation

ValentinMoullet commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffallen commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffallen commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ValentinMoullet commented Apr 19, 2018 •

edited

Loading

jeffallen commented Apr 25, 2018 •

edited

Loading