JWT V2 #66

aricart · 2020-02-04T14:38:57Z

V2 Changes

V1 Deprecates cluster/server claims, as these are not used - will re-introduce when supported
V2 Removes deprecated API
Resolve JWT namespace squatting. Extra fields leaked through to the root object of the claims
NATS claims add a version number enabling the library to reject claims that it doesn't understand. The versioning mechanism is related to the data format, not to a release, so it is an always increasing ordinal.
Old JWTs are automatically migrated into their v2 formats on reading, so reading code is backward compatible for non-tool clients.
V2 Removes internal use of deprecated fields
V2 written JWTs change the algorithm of specified on the JWT to ed25519-nkey. This is done purposefully to make any system component that has not been updated fail validating any v2 written JWTs.

JWT Namespace Squatting Changes

ClaimsData had tags and type fields for all JWT types, these are moved to the structure nats.
ActivationsClaims and UserClaims had issuer_account moved to the nats structure.

Refer to the 3rd checking to see the changes, as I moved files to minimize the diffs and allow focusing on the actual changes.

This PR will merge into jwt/master, but will not be released as v2.0.0 yet. As we'll need to develop the strategy for having services such as NGS conditionally rewrite JWTs matching the format v1 or v2 that was provided for signing.

- V1 Deprecates cluster/server claims, as these are not used - will re-introduced when supported - V2 Removes deprecated API - Resolve JWT namespace squatting. Extra fields leaked through to the root object of the claims - NATS claims add a version number enabling the library to reject claims that it doesn't understand. - Old JWTs are automatically migrated into their v2 formats on read, so reading code is backwards compatible for non-tool clients. - V2 Removes internal use of deprecated fields JWT Namespace Squatting Changes - ClaimsData had `tags` and `type` fields for all JWT types, these are moved to the structure `nats`. - ActivationsClaims and UserClaims had `issuer_account` moved to the `nats` structure.

wallyqs · 2020-02-07T00:38:37Z

Since using modules not sure we need to have a v2 folder for the v2 API. Once the v2.0.0 git tag is pushed due to Go modules would be able to use nats-io/jwt/v2 instead and nats-io/jwt for v1 version of the package if need to have both.

- V1 Deprecates cluster/server claims, as these are not used - will re-introduced when supported - V2 Removes deprecated API - Resolve JWT namespace squatting. Extra fields leaked through to the root object of the claims - NATS claims add a version number enabling the library to reject claims that it doesn't understand. - Old JWTs are automatically migrated into their v2 formats on read, so reading code is backwards compatible for non-tool clients. - V2 Removes internal use of deprecated fields JWT Namespace Squatting Changes - ClaimsData had `tags` and `type` fields for all JWT types, these are moved to the structure `nats`. - ActivationsClaims and UserClaims had `issuer_account` moved to the `nats` structure.

This pulls in system_account changes

Signed-off-by: Matthias Hanel <mh@synadia.com>

Changing name of the algorithm to trip v1 jwt libraries. The old name ed25519 is not used by other jwt libraries and was only valid for our library. Therefore changing that string is ok. Add field assert server version to operator.

aricart · 2020-05-27T21:13:55Z

Since using modules not sure we need to have a v2 folder for the v2 API. Once the v2.0.0 git tag is pushed due to Go modules would be able to use nats-io/jwt/v2 instead and nats-io/jwt for v1 version of the package if need to have both.

Yes we need access to both versions at the same time from the same library.

aricart · 2020-05-27T21:15:37Z

Just for further documentation, the workflow of doing a go get lib@vX.X.X does an internal consistency check. If you do go get lib@v2.0.0 it rejects it, because the module system knows that the valid request for v2 has to be go get lib/v2@v2.0.0

kozlovic

Some changes regarding dates, etc..

kozlovic · 2020-05-27T22:41:22Z

.travis.yml

- 1.13.x
- 1.12.x
-
+  - 1.13.x


Probably want to have 1.14.x and 1.13.x and drop 1.12.x now (I know, PR was originally created long ago).

kozlovic · 2020-05-27T22:41:55Z

.travis.yml

+    script: $HOME/gopath/bin/goveralls -coverprofile=v2/coverage.out -service travis-ci
+    on:
+      tags: true
+      condition: ${V} = v2 && $TRAVIS_GO_VERSION =~ ^1.13.


maybe 1.14 here?

kozlovic · 2020-05-27T22:42:22Z

.travis.yml

- misspell -error -locale US .
- staticcheck ./...
-
+  - cd $TRAVIS_BUILD_DIR/${V}


Since you do move to that directory to run the tests down below, not sure you need it here.

kozlovic · 2020-05-27T22:46:05Z

go.mod

-require github.com/nats-io/nkeys v0.1.4
+require (
+	github.com/nats-io/nkeys v0.1.3
+	github.com/stretchr/testify v1.5.1 // indirect


Hopefully that does not bring this dep in the nats-server/go client. I know you are a big fan of it, but I like to minimize any dependency unless really required. So far, all testing in nats-server/streaming-server have not needed any feature that testify may bring that could not be done with std lib.

kozlovic · 2020-05-27T22:48:36Z

user_claims_test.go

+	uc.Validate(vr)
+
+	if vr.IsEmpty() || len(vr.Issues) != 1 || !vr.IsBlocking(true) {
+		t.Error("bad permission should be invalid")


t.Error() will mark the test as failed, but will continue execution of the test. Does that make sense? I personally never use t.Error() because any error condition in my opinion invalidates the test.
(I won't repeat comment on all t.Error() occurrences).

yes it is required - the test is mucking with the model further down to catch other kinds of errors - likely it should many different tests but it isn't.

v2/creds_utils_test.go

v2/decoder.go

v2/decoder_account.go

v2/decoder_activation.go

v2/decoder_operator.go

derekcollison · 2020-05-28T03:07:45Z

General by @ripienaar has insisted on making move to UTC for all dates and times. Might be a good time to do so here as well?

wallyqs · 2020-05-28T04:53:39Z

go.mod

@@ -1,5 +1,8 @@
 module github.com/nats-io/jwt

-require github.com/nats-io/nkeys v0.1.4
+require (
+	github.com/nats-io/nkeys v0.1.3


nkeys version was downgraded?

I just re-bumped them up.

wallyqs · 2020-05-28T06:34:52Z

v2/go.mod

+	github.com/stretchr/testify v1.5.1
+)
+
+replace github.com/nats-io/jwt v0.3.2 => ../


Not sure how well this will work since it is an 'inter dependent module' but maybe it will... think this more or less following the major subdirectory strategy. The other alternative is to follow the major branch strategy where can skip having a v2 folder and master with only the v2 contents. I think also requires making the v1.0.0 tag, but can then continue development of both v1 and v2 series just in different branches.

we need to be able to read/write v1 from v2, if we run into a problem while developing that portion, then we can change on that.

yes I know that, but we need to make sure that we are taking the right strategy to avoid causing issues to other repos in the ecosystem

Not following the other issues on the nats side - the JWT library has always been a go.mod library since day one, so all the current clients are already importing the lib as such, regardless of whether the other products rely on vendoring to do their builds.

The subdirectory strategy is how they say to use modules, not some invention. The interdependent is a gray area, but in all honesty, I think they are worried about replacements that cannot be honored. In our case, it is the same repo.

Now if we want to do flat (perfectly ok), we need to take a stance and do it. This also means that the library will probably duplicate a bunch of types, but I am sure we can sort it out.

aricart · 2020-05-28T14:04:05Z

General by @ripienaar has insisted on making move to UTC for all dates and times. Might be a good time to do so here as well?

The timestamp format is defined by the JWT standard in the case of our lib, so they are already defined to be exactly what they are (UNIX time) - which is UTC.

wallyqs · 2020-05-28T14:52:54Z

v2/go.mod

+module github.com/nats-io/jwt/v2
+
+require (
+	github.com/nats-io/jwt v0.3.2


I think we should consider tagging v1.0.0 first before merging in the v2 commits in master

this is probably ok.

note that we are not releasing v2 until later - the merge won't trigger a v2 release until the other systems consume them.

Imho I would imagine that jetstream is going to be the feature that triggers a nats-server version bump (or at least hoping that is the case). I think that is the opportunity to v2 releases. This will draw the line that a previous major version can reject the older jwts. Otherwise, it seems that we are not applying semver properly.

It might be fine but if we don't know we probably shouldn't do it unless testing that is ok, in general should be careful since there isn't really room for errors with Go modules because the Go proxy will cache the state of the repo as soon as it is available.

matthiashanel · 2020-05-28T16:13:42Z

I tested pushing v2 jwt to an "old" account server as well as connecting with a v2 jwt to an "old" nats-server. Both behaved as expected.

wallyqs · 2020-05-28T16:35:37Z

Some notes about the module strategy here:

A v2 folder is created with its own go.mod, which includes an import to the v0 version of the package. This is the rare case where a multi module repository repo strategy is ok (generally they are not) and is actually recommended by the Go team.
The other alternative is to use a major branch strategy where we don't require the v0 code to be forever at the top level and can use the v0 and v2 git tags instead to be able to import v0 code from v2. Although this could be an option, it will only work in projects that have already adopted Go modules and Go kit which includes nats.go does not yet adopt modules and is one of the most popular repos, so to avoid breaking $GOPATH compatibility the v2 folder approach as in this PR is the only option (since so far we have not broken GOPATH compatibility in nats.go).
Not sure of the impact yet, but to avoid issues we should probably tag v1.0.0 first then rebase this PR so that it includes the commit with the v1.0.0 tag and the new v2 folder commits which is what the go mod tooling will probably assume should have happened. Also we should watch this issue related to the usability of editing interdependent modules in same repo: cmd/go: support simultaneous edits of interdependent modules golang/go#27542)

EDIT: Updated mentions of v1 to be v0 which is more correct.

added windows for build test

wallyqs · 2020-05-28T22:50:59Z

Since the library is still at v0.3.2 I'm now thinking that the bump that should be done is against v1.0.0 instead of v2, we might be unnecessarily skipping one important major version and probably making the migration more complex by aiming to call it v2 (which could be done later).

aricart · 2020-05-29T19:01:39Z

Since the library is still at v0.3.2 I'm now thinking that the bump that should be done is against v1.0.0 instead of v2, we might be unnecessarily skipping one important major version and probably making the migration more complex by aiming to call it v2 (which could be done later).

@derekcollison we hashed out that v2 is correct. We are going to tag v1.0.0, but remains at is, otherwise the imports will be weird with a jwt@v1 when that is importing jwt.
@wallyqs can probably fill in what we discussed and agreed.

derekcollison · 2020-05-29T19:05:14Z

If you and @wallyqs agree on approach I am good.

aricart · 2020-05-29T19:28:09Z

If you and @wallyqs agree on approach I am good.

we are all in the same page now.

# Conflicts: # .travis.yml

wallyqs · 2020-05-29T22:19:42Z

Yes now this PR can be rebased on top of the changes from the v1.0.0, v1.0.1 tags and then start dev of the v2 module and merge to master. There can be still changes to v1.0.1 or releases if needed but they would branch out from that v1 tag and the repo will eventually be tagged as v2.0.0 (using the pre-release tags first with the format like v2.0.0-alpha.1. Because it is also a folder then it is backwards compatible with GOPATH users as well since for those users it works like a package, meaning that if nats.go moves to nats-io/jwt/v2 once the repo is tagged with v2.0.0 it should be possible to be used for both GOPATH users and Go module users.

kozlovic

Still some questions about travis..

kozlovic · 2020-05-29T23:02:40Z

.travis.yml

- go test -v -race ./...
- if [[ "$TRAVIS_GO_VERSION" =~ 1.13 ]]; then ./scripts/cov.sh TRAVIS; fi
+  - go test -v -race ./...
+  - go test -v -coverprofile=./coverage.out ./...


If you are going to report coverage only once, you may want to simply not run it here for matrixes that do not report. Say you report only for Go 1.14 and v2, then you should run only then. That being said, jwt tests are run quickly, so that is less of a concern.

Not quite sure what the coverage report means for the multi version repo.

That's my point, we should do the coverage only once per PR. Here it will run for all the matrix entries, but we should post only once.

Again, why run the coverage if you are not going to report it? Since down below we push it on v2, for linux, and Go 1.14, I would have run the coverage code only in those conditions.

kozlovic · 2020-05-29T23:03:42Z

.travis.yml

+- provider: script
+  script: $HOME/gopath/bin/goveralls -coverprofile=v2/coverage.out -service travis-ci
+  on:
+    tags: true


The way I understand you will push coveralls only on a git tags push, which is not what we want I think.

Whenever you do a tag it will push, which is fine, that matches the releases.

The point is that we - for other projects - update the coverage for every PR, not only when there is a new release.

kozlovic · 2020-05-29T23:05:20Z

.travis.yml

+  - go test -v -coverprofile=./coverage.out ./...
+deploy:
+- provider: script
+  script: $HOME/gopath/bin/goveralls -coverprofile=v2/coverage.out -service travis-ci


Not sure about v2/coverage.out. Is that where the file will be? We call go test -v -coverprofile=./coverage.out ./..., does that work by creating a coverage.out in the /v2 directory?

Sure it does. When we activate the v2 release, then I’ll have that coverage be posted.

…y targetting v1.

kozlovic

LGTM

kozlovic · 2020-06-01T20:12:47Z

.travis.yml

- go test -v -race ./...
- if [[ "$TRAVIS_GO_VERSION" =~ 1.13 ]]; then ./scripts/cov.sh TRAVIS; fi
+  - go test -v -race ./...
+  - go test -v -coverprofile=./coverage.out ./...


Again, why run the coverage if you are not going to report it? Since down below we push it on v2, for linux, and Go 1.14, I would have run the coverage code only in those conditions.

aricart added 3 commits February 4, 2020 09:48

v2 move with history

17a6134

restored original files to preserve history

d8ef752

aricart changed the title ~~Jwtv2~~ JWT V2 Feb 4, 2020

aricart requested review from kozlovic, derekcollison, philpennock and wallyqs February 4, 2020 18:25

wallyqs mentioned this pull request Feb 7, 2020

Add support for qsub permissions #68

Closed

aricart and others added 10 commits February 21, 2020 09:16

Merge branch 'master' into jwtv2

2220eef

updated branch from master, propagated v1 changes to v2

deb09e7

Merge branch 'master' into jwtv2

2f74d71

Merge change to master (v1) where the regex was relaxed.

79b67eb

v2 move with history

db21420

restored original files to preserve history

b8765bc

Merge change to master (v1) where the regex was relaxed.

9a2e4cb

Merge branch 'jwtv2' of github.com:nats-io/jwt into jwtv2

12f0d10

This pulls in system_account changes

Include system_account in v1 as well

86d4640

Signed-off-by: Matthias Hanel <mh@synadia.com>

matthiashanel force-pushed the jwtv2 branch from e046940 to 86d4640 Compare May 21, 2020 22:13

v2 jwt need to cause an error in v1 jwt libraries.

090bfbe

Changing name of the algorithm to trip v1 jwt libraries. The old name ed25519 is not used by other jwt libraries and was only valid for our library. Therefore changing that string is ok. Add field assert server version to operator.

aricart requested review from kozlovic, matthiashanel, derekcollison and philpennock and removed request for wallyqs, derekcollison, philpennock and kozlovic May 27, 2020 21:11

aricart requested a review from wallyqs May 27, 2020 21:11

kozlovic requested changes May 27, 2020

View reviewed changes

wallyqs reviewed May 28, 2020

View reviewed changes

review changes

3469ce6

wallyqs reviewed May 28, 2020

View reviewed changes

removed dependencies on the require library

169a155

added windows for build test

Merge branch 'master' into jwtv2

bc998d9

# Conflicts: # .travis.yml

kozlovic reviewed May 29, 2020

View reviewed changes

changed coveralls to collect coverage for every build - currently onl…

d2ec31e

…y targetting v1.

aricart force-pushed the jwtv2 branch from 2f5dc39 to d2ec31e Compare June 1, 2020 14:14

kozlovic approved these changes Jun 1, 2020

View reviewed changes

aricart merged commit 430770d into master Jun 1, 2020

aricart deleted the jwtv2 branch June 1, 2020 20:22

-              - 1.13.x
-              - 1.12.x
+                - 1.13.x

JWT V2 #66

JWT V2 #66

Conversation

aricart commented Feb 4, 2020 • edited Loading

V2 Changes

JWT Namespace Squatting Changes

wallyqs commented Feb 7, 2020

aricart commented May 27, 2020

aricart commented May 27, 2020

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

derekcollison commented May 28, 2020

Choose a reason for hiding this comment

aricart May 28, 2020 • edited Loading

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

aricart commented May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aricart May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiashanel commented May 28, 2020

wallyqs commented May 28, 2020 • edited Loading

wallyqs commented May 28, 2020

aricart commented May 29, 2020

derekcollison commented May 29, 2020

aricart commented May 29, 2020

wallyqs commented May 29, 2020 • edited Loading

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

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aricart commented Feb 4, 2020 •

edited

Loading

aricart May 28, 2020 •

edited

Loading

aricart commented May 28, 2020 •

edited

Loading

aricart May 28, 2020 •

edited

Loading

wallyqs commented May 28, 2020 •

edited

Loading

wallyqs commented May 29, 2020 •

edited

Loading