Repository: nats-io/nats-server
Pull request #1477 (2020-06-14 20:37:31)

[ADDED] Cookie JWT auth for WebSocket
When authenticating via browser, the best security practice is to use HttpOnly cookies, to avoid scripts stealing client identity via XSS.

HttpOnly cookies can't be accessed by JavaScript, making those perfect for server-certified authentication.

PR implements optional JWT auth using cookies by specifying this option in a config file:
`ws: { jwt_cookie: "cookie_name" }`

This will make NATS treat a cookie named "cookie_name" as JWT for authentication. If this cookie exists in the HTTP request and is valid, the server will act as if authentication is not required. If this token is missing or invalid, it's possible to authenticate using other configured methods.

Notice that since signing is not possible when authorizing this way, BearerToken should be set in the JWT (e.g. User should be created with `nsc add user --bearer`).

Tests for regular JWT auth and cookie auth are included.

/cc @nats-io/core


kozlovic: @pas2k First I need to congratulate you on the quality of this PR. Your included test touches areas that are not so easy to grasp and you have made use of all the existing JWT test helper functions.

I need to look more into this approach, but I have reservations on how you use isClientAuthorized() in createClient since this is something normally invoked when receiving a CONNECT, etc.. I wonder if it would not be cleaner and safer to extract (make it a function) the jwt specific authentication part so it could be called here. Again, will look more into it.
pas2k: @kozlovic Thank you for the quick review and kind words.

A couple of thoughts regarding isClientAuthorized() use, since it is certainly the most contentius change in the PR.

Checking JWT changes both server and client state. The server might pull new accounts from account server, for example. So extracting JWT checking logic won't decrease the mutex use. It can eliminate some checks for empty fields, certainly, but that's barely noticeable.

Separating JWT checking from isClientAuthorized() can certainly be unsafe, though. Since isClientAuthorized() is used on config reload, there's space for subtle auth bugs if there's a mismatch between JWT-only auth and the rest of the system. Keeping the two synchronized will become a maintenance burden.

Seeing how it's used in reload logic, isClientAuthorized() seems to be the only function idempotent against the client auth fields and configuration, and that's why I decided to use it.


kozlovic: @pas2k With @aricart we tried few things and here are our observations:

We used https://github.com/aricart/natsws-sandbox to test and made small modifications to `bin/server.js`:
- Replace the listen's address from `127.0.0.1` to `localhost`
- Add `res.setHeader('Set-Cookie','jwt=<some jwt>; HttpOnly')` before `res.writeHead(200)`

Run the NATS Server with your PR and a proper config and started a webpage on `https://localhost:1443` and that worked, it could connect using the new logic. However, when looking at the cookie for this page, we can see the jwt cookie, so that does seem to defeat the purpose.

If we remove setting the cookie in `bin/server.js` but instead do it in the client, that is in `content/index.js`, in `run()` with something like:
```
  ...
  const id = nats.nuid.next()
  
  document.cookie = "jwt=<some jwt>; HttpOnly"
```
Then the NATS Server does not see this cookie, and so does not work. If we remove "HttpOnly" from the command above, then it works, NATS Server see cookies and authenticate based on that.

The point is that in no case we see "HttpOnly" either masking the jwt in the client or its use in the client prevents the NATS Server from seeing it. 

Are we missing something?


pas2k: Hi @kozlovic and @aricart,

Just tried it. It's working as intended. Here are my results:

- `document.cookie = "jwt=<some jwt>; HttpOnly"` doesn't work; you can't set HttpOnly cookies from JavaScript code.
- `document.cookie = "jwt=<some jwt>"` defeats the purpose, since the script knows what `<some_jwt>` is.
- With `res.setHeader('Set-Cookie','jwt=<some jwt>; HttpOnly')`, if I open Chrome's developer console, and enter `document.cookie`, I see `""` as a result. This means a rogue script cannot steal this cookie and send it to the malicious actors.
- With `res.setHeader('Set-Cookie','jwt=<some jwt>; HttpOnly')`, if I open Chrome's developer console, and enter `document.cookie`, I see `"jwt=<some jwt>"` as a result. This means that it can be stolen.

When you say "looking at the cookie for this page", you probably mean looking at the cookie list in the developer tools, right? HttpOnly cookies are displayed there, but are inaccessible to any client JavaScript.

Here's an example attack that this prevents:
- A company hosts a multi-user spreadsheet application using NATS for communication
- There are ads on the website, provided by AdAgency, by including a script inside the page
- Page's Content-Security-Policy allows for this script's execution.
- AdAgency becomes compromised, the ad script is changed to do whatever spreadsheet script does to get the JWT, then does an AJAX request to the remote attacker's collection server.
- The attacker logs in into NATS and steals all important spreadsheets :-)

Same can be done by a malicious user.
- User logs in into the spreadsheet app
- Inserts a <script> tag into the cell
- If the website improperly escapes the input, JWT can be stolen from users who are currently logged in.

XSS is still the most exploited website vulnerability, so having this feature closes a huge attack surface.

aricart: Yes absolutely this is awesome. I am going to augment the authentication example that I have that uses WSS to include this.
kozlovic: > When you say "looking at the cookie for this page", you probably mean looking at the cookie list in the developer tools, right? HttpOnly cookies are displayed there, but are inaccessible to any client JavaScript.

Yes, and on the lock icon in address bar, then Cookies, etc..

Thanks for all the details. Now that we have established the validity and usefulness of that, I will comment on the code specifically. Stay tuned.
pas2k: > Soon we'll have the ability for the client to verify identity by signing. When we have this on the client, access to the JWT won't jeopardize it, as we'll rely on the nkey signature.

@aricart If the website's script can sign the auth request, that means it has access to the private key. That also means that any malicious script can acquire the private key the way genuine script did.

Any protection against XSS always has to rely on something that JS can't access. Having an ability to sign something with privkey obviously exposes it to XSS, but it is a much less of a problem, since you can't derive a JWT from privkey because JWT is signed with Account key, and without the ability to log in, the privkey is kinda useless.
aricart: > > Soon we'll have the ability for the client to verify identity by signing. When we have this on the client, access to the JWT won't jeopardize it, as we'll rely on the nkey signature.
> 
> @aricart If the website's script can sign the auth request, that means it has access to the private key. That also means that any malicious script can acquire the private key the way genuine script did.
> 
> Any protection against XSS always has to rely on something that JS can't access. Having an ability to sign something with privkey obviously exposes it to XSS, but it is a much less of a problem, since you can't derive a JWT from privkey because JWT is signed with Account key, and without the ability to log in, the privkey is kinda useless.

Dialing it back a bit:

- Browsers are not really secure platforms. Especially since the web application cannot control the types of helpful plugins that the user may have installed.

- If a site requires very strong privacy (banking app), then more than likely they are not embedding adds or other things, but still, the user could have installed some helpful addon.

- Regardless of the authentication mechanism (here we are really talking about playback), it is in the realm of the possibility that a plugin/or script could be targetted to take advantage of some specific issue with a particular web app. If that is the case, they already can probably hijack some other kind of resource, so yes, the HttpOnly will prevent playback of the authentication, but it is also possible to do one shot JWTs, which means that get revoked when the session is over.

If we had access to private keys, we can still be fairly creative, even if HttpOnly is not supported. The server could create a one-shot user in the context of an account with a specific limit of a single connection, the authentication process would require the private key for verifying it. The client would by default be isolated, and services would be specifically scoped to the client. On session completion, the account invalidates the user jwt, making any kind of replay impossible.

aricart: @philpennock would probably have additional comments on the strategies, as he's our security expert.
pas2k: > Creative security strategy

@aricart This is getting a bit off-topic, which is mostly my fault. It would be nice to have a repo with best practice configuration you described and see how it compares. If you ever do, feel free to ping me, would be happy to discuss it there.
pas2k: Implemented @kozlovic's suggestions, rebased vs master, covered options and protocol JWT override with tests.

Patch:
diff --git a/server/client.go b/server/client.go
index 8196cb44..46112d96 100644
--- a/server/client.go
+++ b/server/client.go
@@ -1497,7 +1497,13 @@ func (c *client) processConnect(arg []byte) error {
 	lang := c.opts.Lang
 	account := c.opts.Account
 	accountNew := c.opts.AccountNew
+
+	// If websocket client and JWT not in the CONNECT, use the cookie JWT (possibly empty).
+	if ws := c.ws; ws != nil && c.opts.JWT == "" {
+		c.opts.JWT = ws.cookieJwt
+	}
 	ujwt := c.opts.JWT
+
 	// For headers both client and server need to support.
 	c.headers = supportsHeaders && c.opts.Headers
 	c.mu.Unlock()
diff --git a/server/opts.go b/server/opts.go
index 88a63a76..625ac3cc 100644
--- a/server/opts.go
+++ b/server/opts.go
@@ -265,6 +265,11 @@ type WebsocketOpts struct {
 	// Users defined here or in the global options.
 	NoAuthUser string
 
+	// Name of the cookie, which if present in WebSocket upgrade headers,
+	// will be treated as JWT during CONNECT phase as long as
+	// "jwt" specified in the CONNECT options is missing or empty.
+	JWTCookie string
+
 	// Authentication section. If anything is configured in this section,
 	// it will override the authorization configuration for regular clients.
 	Username string
@@ -3152,6 +3157,8 @@ func parseWebsocket(v interface{}, o *Options, errors *[]error, warnings *[]erro
 			if auth.nkeys != nil {
 				o.Websocket.Nkeys = append(o.Websocket.Nkeys, auth.nkeys...)
 			}
+		case "jwt_cookie":
+			o.Websocket.JWTCookie = mv.(string)
 		case "no_auth_user":
 			o.Websocket.NoAuthUser = mv.(string)
 		default:
diff --git a/server/websocket.go b/server/websocket.go
index 4cf5cec1..8c167830 100644
--- a/server/websocket.go
+++ b/server/websocket.go
@@ -90,6 +90,7 @@ type websocket struct {
 	closeSent  bool
 	browser    bool
 	compressor *flate.Writer
+	cookieJwt  string
 }
 
 type srvWebsocket struct {
@@ -597,6 +598,11 @@ func (s *Server) wsUpgrade(w http.ResponseWriter, r *http.Request) (*wsUpgradeRe
 	if ua := r.Header.Get("User-Agent"); ua != "" && strings.HasPrefix(ua, "Mozilla/") {
 		ws.browser = true
 	}
+	if opts.Websocket.JWTCookie != "" {
+		if c, err := r.Cookie(opts.Websocket.JWTCookie); err == nil && c != nil {
+			ws.cookieJwt = c.Value
+		}
+	}
 	return &wsUpgradeResult{conn: conn, ws: ws}, nil
 }
 
@@ -748,6 +754,12 @@ func validateWebsocketOptions(o *Options) error {
 		}
 		return fmt.Errorf("websocket no_auth_user %q not found in users configuration", wo.NoAuthUser)
 	}
+	// Using JWT requires Trusted Keys
+	if wo.JWTCookie != "" {
+		if len(o.TrustedOperators) == 0 && len(o.TrustedKeys) == 0 {
+			return fmt.Errorf("trusted operators or trusted keys configuration is required for JWT authentication via cookie %q", wo.JWTCookie)
+		}
+	}
 	return nil
 }
 


Test patch:
diff --git a/server/websocket_test.go b/server/websocket_test.go
index 9a625a38..b27dd83b 100644
--- a/server/websocket_test.go
+++ b/server/websocket_test.go
@@ -36,6 +36,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/nats-io/jwt/v2"
 	"github.com/nats-io/nkeys"
 )
 
@@ -1454,6 +1455,11 @@ func TestWSValidateOptions(t *testing.T) {
 			o.Websocket.AllowedOrigins = []string{"http://this:is:bad:url"}
 			return o
 		}, "unable to parse"},
+		{"missing trusted configuration", func() *Options {
+			o := wso.Clone()
+			o.Websocket.JWTCookie = "jwt"
+			return o
+		}, "keys configuration is required"},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			err := validateWebsocketOptions(test.getOpts())
@@ -1584,9 +1590,16 @@ func TestWSAbnormalFailureOfWebServer(t *testing.T) {
 	}
 }
 
-func testWSCreateClientGetInfo(t testing.TB, compress, web bool, host string, port int) (net.Conn, *bufio.Reader, []byte) {
+type testWSClientOptions struct {
+	compress, web bool
+	host          string
+	port          int
+	extraHeaders  map[string]string
+}
+
+func testNewWSClient(t testing.TB, o testWSClientOptions) (net.Conn, *bufio.Reader, []byte) {
 	t.Helper()
-	addr := fmt.Sprintf("%s:%d", host, port)
+	addr := fmt.Sprintf("%s:%d", o.host, o.port)
 	wsc, err := net.Dial("tcp", addr)
 	if err != nil {
 		t.Fatalf("Error creating ws connection: %v", err)
@@ -1596,12 +1609,17 @@ func testWSCreateClientGetInfo(t testing.TB, compress, web bool, host string, po
 		t.Fatalf("Error during handshake: %v", err)
 	}
 	req := testWSCreateValidReq()
-	if compress {
+	if o.compress {
 		req.Header.Set("Sec-Websocket-Extensions", "permessage-deflate")
 	}
-	if web {
+	if o.web {
 		req.Header.Set("User-Agent", "Mozilla/5.0")
 	}
+	if o.extraHeaders != nil {
+		for hdr, val := range o.extraHeaders {
+			req.Header.Add(hdr, val)
+		}
+	}
 	req.URL, _ = url.Parse("wss://" + addr)
 	if err := req.Write(wsc); err != nil {
 		t.Fatalf("Error sending request: %v", err)
@@ -1623,6 +1641,102 @@ func testWSCreateClientGetInfo(t testing.TB, compress, web bool, host string, po
 	return wsc, br, info
 }
 
+type testClaimsOptions struct {
+	nac            *jwt.AccountClaims
+	nuc            *jwt.UserClaims
+	connectRequest interface{}
+	dontSign       bool
+	expectAnswer   string
+}
+
+func testWSWithClaims(t *testing.T, s *Server, o testWSClientOptions, tclm testClaimsOptions) (kp nkeys.KeyPair, conn net.Conn, rdr *bufio.Reader, auth_was_required bool) {
+	t.Helper()
+
+	okp, _ := nkeys.FromSeed(oSeed)
+
+	akp, _ := nkeys.CreateAccount()
+	apub, _ := akp.PublicKey()
+	if tclm.nac == nil {
+		tclm.nac = jwt.NewAccountClaims(apub)
+	} else {
+		tclm.nac.Subject = apub
+	}
+	ajwt, err := tclm.nac.Encode(okp)
+	if err != nil {
+		t.Fatalf("Error generating account JWT: %v", err)
+	}
+
+	nkp, _ := nkeys.CreateUser()
+	pub, _ := nkp.PublicKey()
+	if tclm.nuc == nil {
+		tclm.nuc = jwt.NewUserClaims(pub)
+	} else {
+		tclm.nuc.Subject = pub
+	}
+	jwt, err := tclm.nuc.Encode(akp)
+	if err != nil {
+		t.Fatalf("Error generating user JWT: %v", err)
+	}
+
+	addAccountToMemResolver(s, apub, ajwt)
+
+	c, cr, l := testNewWSClient(t, o)
+
+	var info struct {
+		Nonce        string `json:"nonce,omitempty"`
+		AuthRequired bool   `json:"auth_required,omitempty"`
+	}
+
+	if err := json.Unmarshal([]byte(l[5:]), &info); err != nil {
+		t.Fatal(err)
+	}
+	if info.AuthRequired {
+		cs := ""
+		if tclm.connectRequest != nil {
+			customReq, err := json.Marshal(tclm.connectRequest)
+			if err != nil {
+				t.Fatal(err)
+			}
+			// PING needed to flush the +OK/-ERR to us.
+			cs = fmt.Sprintf("CONNECT %v\r\nPING\r\n", string(customReq))
+		} else if !tclm.dontSign {
+			// Sign Nonce
+			sigraw, _ := nkp.Sign([]byte(info.Nonce))
+			sig := base64.RawURLEncoding.EncodeToString(sigraw)
+			cs = fmt.Sprintf("CONNECT {\"jwt\":%q,\"sig\":\"%s\",\"verbose\":true,\"pedantic\":true}\r\nPING\r\n", jwt, sig)
+		} else {
+			cs = fmt.Sprintf("CONNECT {\"jwt\":%q,\"verbose\":true,\"pedantic\":true}\r\nPING\r\n", jwt)
+		}
+		wsmsg := testWSCreateClientMsg(wsBinaryMessage, 1, true, false, []byte(cs))
+		c.Write(wsmsg)
+		l = testWSReadFrame(t, cr)
+		if !strings.HasPrefix(string(l), tclm.expectAnswer) {
+			t.Fatalf("Expected %q, got %q", tclm.expectAnswer, l)
+		}
+	}
+	return akp, c, cr, info.AuthRequired
+}
+
+func setupAddTrusted(o *Options) {
+	kp, _ := nkeys.FromSeed(oSeed)
+	pub, _ := kp.PublicKey()
+	o.TrustedKeys = []string{pub}
+}
+
+func setupAddCookie(o *Options) {
+	o.Websocket.JWTCookie = "jwt"
+}
+
+func testWSCreateClientGetInfo(t testing.TB, compress, web bool, host string, port int) (net.Conn, *bufio.Reader, []byte) {
+	t.Helper()
+	return testNewWSClient(t, testWSClientOptions{
+		compress: compress,
+		web:      web,
+		host:     host,
+		port:     port,
+	})
+}
+
 func testWSCreateClient(t testing.TB, compress, web bool, host string, port int) (net.Conn, *bufio.Reader) {
 	wsc, br, _ := testWSCreateClientGetInfo(t, compress, web, host, port)
 	// Send CONNECT and PING
@@ -3157,6 +3271,124 @@ func TestWSNkeyAuth(t *testing.T) {
 	}
 }
 
+func TestJWTWSCookieUser(t *testing.T) {
+
+	nucSigFunc := func() *jwt.UserClaims { return newJWTTestUserClaims() }
+	nucBearerFunc := func() *jwt.UserClaims {
+		ret := newJWTTestUserClaims()
+		ret.BearerToken = true
+		return ret
+	}
+
+	o := testWSOptions()
+	setupAddTrusted(o)
+	setupAddCookie(o)
+	s := RunServer(o)
+	buildMemAccResolver(s)
+	defer s.Shutdown()
+
+	genJwt := func(t *testing.T, nuc *jwt.UserClaims) string {
+		okp, _ := nkeys.FromSeed(oSeed)
+
+		akp, _ := nkeys.CreateAccount()
+		apub, _ := akp.PublicKey()
+
+		nac := jwt.NewAccountClaims(apub)
+		ajwt, err := nac.Encode(okp)
+		if err != nil {
+			t.Fatalf("Error generating account JWT: %v", err)
+		}
+
+		nkp, _ := nkeys.CreateUser()
+		pub, _ := nkp.PublicKey()
+		nuc.Subject = pub
+		jwt, err := nuc.Encode(akp)
+		if err != nil {
+			t.Fatalf("Error generating user JWT: %v", err)
+		}
+		addAccountToMemResolver(s, apub, ajwt)
+		return jwt
+	}
+
+	cliOpts := testWSClientOptions{
+		host: o.Websocket.Host,
+		port: o.Websocket.Port,
+	}
+	for _, test := range []struct {
+		name         string
+		nuc          *jwt.UserClaims
+		opts         func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions)
+		expectAnswer string
+	}{
+		{
+			name: "protocol auth, non-bearer key, with signature",
+			nuc:  nucSigFunc(),
+			opts: func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions) {
+				return cliOpts, testClaimsOptions{nuc: claims}
+			},
+			expectAnswer: "+OK",
+		},
+		{
+			name: "protocol auth, non-bearer key, w/o required signature",
+			nuc:  nucSigFunc(),
+			opts: func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions) {
+				return cliOpts, testClaimsOptions{nuc: claims, dontSign: true}
+			},
+			expectAnswer: "-ERR",
+		},
+		{
+			name: "protocol auth, bearer key, w/o signature",
+			nuc:  nucBearerFunc(),
+			opts: func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions) {
+				return cliOpts, testClaimsOptions{nuc: claims, dontSign: true}
+			},
+			expectAnswer: "+OK",
+		},
+		{
+			name: "cookie auth, non-bearer key, protocol auth fail",
+			nuc:  nucSigFunc(),
+			opts: func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions) {
+				co := cliOpts
+				co.extraHeaders = map[string]string{}
+				co.extraHeaders["Cookie"] = o.Websocket.JWTCookie + "=" + genJwt(t, claims)
+				return co, testClaimsOptions{connectRequest: struct{}{}}
+			},
+			expectAnswer: "-ERR",
+		},
+		{
+			name: "cookie auth, bearer key, protocol auth success with implied cookie jwt",
+			nuc:  nucBearerFunc(),
+			opts: func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions) {
+				co := cliOpts
+				co.extraHeaders = map[string]string{}
+				co.extraHeaders["Cookie"] = o.Websocket.JWTCookie + "=" + genJwt(t, claims)
+				return co, testClaimsOptions{connectRequest: struct{}{}}
+			},
+			expectAnswer: "+OK",
+		},
+		{
+			name: "cookie auth, non-bearer key, protocol auth success via override jwt in CONNECT opts",
+			nuc:  nucSigFunc(),
+			opts: func(t *testing.T, claims *jwt.UserClaims) (testWSClientOptions, testClaimsOptions) {
+				co := cliOpts
+				co.extraHeaders = map[string]string{}
+				co.extraHeaders["Cookie"] = o.Websocket.JWTCookie + "=" + genJwt(t, claims)
+				return co, testClaimsOptions{nuc: nucBearerFunc()}
+			},
+			expectAnswer: "+OK",
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			cliOpt, claimOpt := test.opts(t, test.nuc)
+			claimOpt.expectAnswer = test.expectAnswer
+			_, c, _, _ := testWSWithClaims(t, s, cliOpt, claimOpt)
+			c.Close()
+		})
+	}
+	s.Shutdown()
+
+}
+
 // ==================================================================
 // = Benchmark tests
 // ==================================================================