Add JWT support for users, accounts and import activations #804

derekcollison · 2018-11-21T15:46:32Z

This PR adds support for trusted roots (operators) and JWT support for accounts, users, import and export claims and import activation tokens.

Supports resolver updates (memory based for now, will be URL) for accounts and URL or inline for activation tokens. Support JWT expiration.

/cc @nats-io/core

coveralls · 2018-11-21T15:57:02Z

Coverage decreased (-0.2%) to 90.937% when pulling 6b23e40 on trust into fcb15da on master.

kozlovic

Review of first commit only...

server/const.go

server/server.go

server/trust_test.go

Add in trusted keys options and binary stamp User JWT and Account fetch with AccountResolver Account and User expiration Account Imports/Exports w/ updates Import activation expiration Signed-off-by: Derek Collison <derek@nats.io>

kozlovic

Review of accounts.go

kozlovic · 2018-11-21T18:49:02Z

server/accounts.go

-	for _, a := range accounts {
-		ma[a.Name] = a
+	ea := a.exports.services[subject]
+	if accounts != nil && ea == nil {


Difficult to understand: the ea map is only created and never updated (at least in this function). Is this the intent? In other words, for a given account, there will be always a single call to AddServiceExport() for a given subject? (no ability to add more accounts with additional calls to this function)

Seeing the equivalent function for stream makes me wonder if the for _, acc should not be moved out of this if.

Setting to nil ok since that indicates something, when we retrieve we check ok not for v != nil.

But what I meant is that we populate ea only once, when it was not found and we create/populate it. But next time there is a lookup for subject and it is not nil, then nothing happens. Is that right?

When we fetch ea it could be missing and nil, but we set it outside the loop regardless because if it was not found but we have no accounts[] then we need to set the entry to nil in the map. On authorization we check v, ok := a.exports.services[subject]; !ok

Again, what I was asking is that: will there ever be an incremental call to AddServiceExport for the same account and same subject but possibly different list of accounts? If not, then that's fine, otherwise, I think there may be a problem.

Meaning you want the function to treat the authorized list as idempotent correct?

I want to know if there will be a case of calling AddServicesExport() with same account/subject and different list of accounts. If not, then code is fine. Otherwise, not sure.
I see that in one case this function is called from a place where we just create the account (buildInternal) so that's fine, but otherwise it looks like it is from an update of an account.

server/accounts.go

kozlovic · 2018-11-21T19:15:23Z

server/accounts.go

+}
+
+// Sets the expiration timer for an account JWT that has it set.
+func (a *Account) setExpirationTimer(d time.Duration) {


No locking used there... but it seems that we are calling this without account's lock.

Server lock is held.

I tracked it down up to UpdateAccount() and FetchAccount(), which are not holding the lock but these function are called by LookupAccount(), which is. But UpdateAccount() and FetchAccount() being exported, this is not safe...

ok let me take a look and resolve.. Good point on exports with lock assumed to be held.

server/accounts.go

sasbury · 2018-11-21T19:56:31Z

server/auth.go

+			c.Debugf("Account JWT not signed by trusted operator")
+			return false
+		}
+		if acc.IsExpired() {


There is a second time check we should do for "nbf" (Not Before) Claim. This is the opposite of expired basically. We don't use it yet, but the server should probably check just in case. The isBlocking(true) checks this on jwt claims

note, this probably shows up in a number of places

I don't think we will make it available if it fails the IsBlocking(true) since we process it that way when first handed the claims IIRC. The acc.IsExpired is for us when we fire the timer to mark the account. Also triggers us to refetch at some point.

sasbury · 2018-11-21T19:57:49Z

server/client.go

@@ -479,6 +483,20 @@ func (c *client) setPermissions(perms *Permissions) {
 	}
 }

+// Check to see if we have an expiration for the user JWT via base claims.
+// FIXME(dlc) - Clear on connect with new JWT.
+func (c *client) checkExpiration(claims *jwt.ClaimsData) {


We can maybe check the nbf claim here since it is similar to expires.

For the user JWT (juc) I do a check before any of this for IsBlocking(true) which I think covers us.

sasbury · 2018-11-21T19:59:42Z

server/jwt_test.go

+	}
+}
+
+func TestJWTUserExpired(t *testing.T) {


Worth adding a not before test

server/client.go

server/server.go

server/errors.go

Signed-off-by: Derek Collison <derek@nats.io>

sasbury

New JWT changes look good.

Signed-off-by: Derek Collison <derek@nats.io>

kozlovic

Clarification needed about AddServiceExport() and big concern about LookupAccount() that ultimately releases then regrabs the server lock during processing.

kozlovic · 2018-11-23T17:07:16Z

server/accounts.go

-	for _, a := range accounts {
-		ma[a.Name] = a
+	ea := a.exports.services[subject]
+	if accounts != nil && ea == nil {


Again, what I was asking is that: will there ever be an incremental call to AddServiceExport for the same account and same subject but possibly different list of accounts? If not, then that's fine, otherwise, I think there may be a problem.

kozlovic · 2018-11-23T17:15:33Z

server/accounts.go

+}
+
+// Sets the expiration timer for an account JWT that has it set.
+func (a *Account) setExpirationTimer(d time.Duration) {


I tracked it down up to UpdateAccount() and FetchAccount(), which are not holding the lock but these function are called by LookupAccount(), which is. But UpdateAccount() and FetchAccount() being exported, this is not safe...

server/accounts.go

server/errors.go

server/server.go

derekcollison · 2018-11-23T23:03:20Z

We need to release any locks when doing a fetch, since its expected to be a URL Get(). And in that case I believe worse that can happen is we update the account twice with the same result.

I think this code may be reworked at some point, but I believe is safe as it is.

Definitely do not want to hold a lock, especially the server lock which is what is protecting accounts, during the fetch.

As we add in Limits to the Account JWTs and need to track usage, we may introduce a different locking mechanism for account lookup etc. If that is the case we will revisit this, IMO.

Signed-off-by: Derek Collison <derek@nats.io>

kozlovic

LGTM

derekcollison requested review from wallyqs, aricart, sasbury and kozlovic November 21, 2018 15:46

kozlovic requested changes Nov 21, 2018

View reviewed changes

derekcollison force-pushed the trust branch from 8a7d952 to 0ee714c Compare November 21, 2018 18:36

kozlovic requested changes Nov 21, 2018

View reviewed changes

sasbury reviewed Nov 21, 2018

View reviewed changes

kozlovic requested changes Nov 21, 2018

View reviewed changes

server/client.go Outdated Show resolved Hide resolved

server/server.go Show resolved Hide resolved

server/errors.go Show resolved Hide resolved

Vendor jwt, fixes for nkey, jwt changes

a2e310f

Signed-off-by: Derek Collison <derek@nats.io>

sasbury reviewed Nov 22, 2018

View reviewed changes

fixes for PR comments, MaxControlLine update for tests

43bd71c

Signed-off-by: Derek Collison <derek@nats.io>

kozlovic requested changes Nov 23, 2018

View reviewed changes

Fix for AddExport and unexport server funcs that require lock

6b23e40

Signed-off-by: Derek Collison <derek@nats.io>

kozlovic approved these changes Nov 24, 2018

View reviewed changes

derekcollison merged commit 7aa5d21 into master Nov 24, 2018

derekcollison deleted the trust branch November 24, 2018 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JWT support for users, accounts and import activations #804

Add JWT support for users, accounts and import activations #804

derekcollison commented Nov 21, 2018

coveralls commented Nov 21, 2018 •

edited

kozlovic left a comment

kozlovic left a comment

kozlovic Nov 21, 2018

kozlovic Nov 21, 2018

derekcollison Nov 22, 2018

kozlovic Nov 22, 2018

derekcollison Nov 22, 2018

kozlovic Nov 23, 2018

derekcollison Nov 23, 2018

kozlovic Nov 23, 2018

kozlovic Nov 21, 2018

derekcollison Nov 22, 2018

kozlovic Nov 23, 2018

derekcollison Nov 23, 2018

sasbury Nov 21, 2018

sasbury Nov 21, 2018

derekcollison Nov 22, 2018 •

edited

sasbury Nov 21, 2018

derekcollison Nov 22, 2018

sasbury Nov 21, 2018

derekcollison Nov 22, 2018

sasbury left a comment

kozlovic left a comment

kozlovic Nov 23, 2018

kozlovic Nov 23, 2018

derekcollison commented Nov 23, 2018

kozlovic left a comment

Add JWT support for users, accounts and import activations #804

Add JWT support for users, accounts and import activations #804

Conversation

derekcollison commented Nov 21, 2018

coveralls commented Nov 21, 2018 • edited

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic 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

Choose a reason for hiding this comment

derekcollison Nov 22, 2018 • edited

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

sasbury left a comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison commented Nov 23, 2018

kozlovic left a comment

Choose a reason for hiding this comment

coveralls commented Nov 21, 2018 •

edited

derekcollison Nov 22, 2018 •

edited