Add TokenHandler option to connection options

[nicholaslam]

This pull request adds TokenFunc to the Options struct. This adds a way to generate a new token every time a connectProto is built and allows the use of expiring tokens like JWTs when authenticating.

[coveralls]

Coverage increased (+0.09%) to 94.094% when pulling ce64911 on nicholaslam:token-func into 15a3afb on nats-io:master.

[derekcollison]

Do note that we are adding JWTs (kindof as we speak) that are signed by nkeys and allow embedding of permissions and account imports and exports. Will review this one for sure but we may want to see if the upcoming changes will solve your issue in a more broad sense.

[derekcollison]

Probably call it AuthorizationUpdate since it is updating opts.Authorization. Also, why not just add a method to update it under lock? The way it is here what happens if the function blocks indefinitely or does not have the ability to get a new token?

[seriousben]

@derekcollison we have looked a bit at nkeys.

Unfortunately we have our own circle of trust within our system allowing service to service calls in a common way that is a bit more complex. Having a tokenFunc allows us to leverage that same approach which would be great.

[derekcollison]

That is fair. I still think a better approach for this lib would be a setter under the lock to update opts.Authorization. You can then build on top of this lib to figure out when you want to update it, on reconnect, or when it expires etc.. WDYT?

[seriousben]

@derekcollison that makes sense to me. Unfortunately right now, we have no way of having a synchronous callback before a reconnect is done. If there was a way to get a callback when Authorization is required so that the user of the lib could know when to change the options it would work.

Maybe instead of the proposed approach, we could do something like:

...
	// Take from options (possibly all empty strings)
	if nc.Opts.AuthenticationUpdateHook != nil {
		nc.Opts.authMutext.Lock()
		err := nc.Opts.AuthenticationUpdateHook(nc) // nc or opts as arg
		nc.Opts.authMutext.Unlock()
                // return err
	}
	user = nc.Opts.User
	pass = nc.Opts.Password
	token = nc.Opts.Token
	nkey = nc.Opts.Nkey
...

This is more general and allows interesting use cases like no downtime password rotation.

[seriousben]

Regarding "The way it is here what happens if the function blocks indefinitely or does not have the ability to get a new token?"

Changing the hook function to return an error might be enough and we could let the users of the library deal with the blocking issue.

[derekcollison]

In practice, do you only update when you know you have to reconnect? Do you update the server config via reload and that kicks out the clients and makes them reconnect and re-authorize?

[derekcollison]

We obviously see this as a large use case as well with upcoming nkey/jwt.

[seriousben]

We would update auth options only when the client reconnects and only if needed. The authenticationUpdate hook would be smart enough to do nothing if the password hasn't changed or the token hasn't expired.

While a connection is established we don't need to do anything. But we want to make sure that if there is a network blip or a a rolling update of nats that the reconnects will work and will use the new password (could be read from a kubernetes secret mounted as a volume) or generate a new token.

[derekcollison]

Sounds reasonable. Playing a bit of devil's advocate since we are dealing with this, what if the client does not know it needs a new token? For instance we are building a generic revocation mechanism that all servers could use, so the token might not have expired but been revoked for other reasons. The server will try to let the client know why it disconnected it, but not guaranteed to have the client receive the proper -ERR. Your approach may already handle it when the reconnect fails.

[seriousben]

Don't worry, this is a valuable discussion.

Your use case is a bit different than ours I think. Our tokens are single-use in that the expiry is set very low (<10 seconds) and are only valid for one purpose which is connecting to Nats (audience=nats). It is a connection token. These tokens are not Nats specific but actually what we call service-to-service JWTs in our system. What we want from our approach is trust in a service (this paired with nats authorization is very powerful), preventing reuse of tokens (with the short expiration) and easy rotation of credentials.

We are not looking at revoking and disconnecting an active connection with these token at the moment. But if gnatsd gained the capability of revoking/disconnecting clients after some time (unrelated to the connection token expiry), we would definitely leverage it since it does add an extra layer of safety.

To address your concerns specifically (of our approach):

• clients do not need to know they need a new token, on reconnect they will always need a new token since tokens are very short lived.
• clients will always be able to get a token, but might be prevented to connect because of Nats authorization. (for example: Service has no permissions configured)

[wallyqs]

@seriousben Maybe the Disconnected callback could be useful here? Since already
know that have to issue a new credentials then could use that to close current NATS connection, issue a new token and then replace with a new NATS connection using the updated credentials, that could be a workaround I think.

Alternatively, maybe we could consider something like an OnReconnectAttempt callback that is executed synchronously previous to trying to establish the connection and sending CONNECT as well, that way some of the state of the client could be updated the there (and could also help for tracing/logging connection attempts on the client, there is a Reconnected handler but that is executed async once after having established the connection).

[seriousben]

@wallyqs Yeah our first thought was to do exactly that. But we feel it is more of a workaround than a real fix especially with the nextServer handling, also because we would need to do that in multiple languages (and teams) (polyglot microservice architectures are fun but ... :) ) and since we use nats-streaming where we would need to tear down that client as well.

I really like your OnReconnectAttempt idea, that is basically what I tried to do with the AuthenticationUpdateHook but your idea is more generic and useful. And yeah we have realized early that the callbacks were asynchronous :)

Thanks a lot for the feedback. We are waiting for some agreement before implementing this in the Go, Node and C clients.

Edit: in node it might already be supported because of the use of syncrhonous events (event-emitters) for the callbacks.

[kozlovic]

The OnReconnectAttempt would be more flexible indeed, but I don't like that we would then change Options. It is possible as of now but not safe in general and not something we should encourage. For this specific case it would be sage I think since you would change the options synchronously before the library sends the CONNECT.
But for instance in C, I make a clone of the options passed during the connect and you won't be able to change them after the initial connect. We could add an API in C to be able to change that value, but something to think about.

[seriousben]

I think we have discussed 3 different approaches so far:

1. Workaround where a new NatsClient is created for every reconnect when a disconnect happens
   • Pros: Could work now.
   • Cons: Different users come up with different approaches, every reconnect needs to allocate a new client, no subscription tracking and resubscribes, Up to users to block while the client is being reconnected/recreated.
2. Options.TokenFunc (or similar) that does not mutate Options but returns a Token (could be similar for username, and password if needed or maybe a AuthorizationFunc that returns some or all of these)
   • Pros: No mutation of Options, Fairly easy to understand usage
   • Cons: Does not reuse Authorization Options
3. Synchronous OnReconnectAttempt that is allowed to mutate the Options
   • Pros: Flexible and has more uses
   • Cons: Mutating the Options is not always safe and in C is not even possible, No clear distinction between the asynchronous connection callbacks and the synchronous OnReconnectAttempt callback

Did I miss anything? Did I get something wrong?

[kozlovic]

Seem accurate and I think you mentioned using nats-streaming? So if this is a connection from NATS Streaming we are talking about, you don't want to re-create the low level NATS because that would break the NATS Streaming connection (all its internal subs would be lost). If you end-up with a solution where you re-create NATS connection, then you would have somehow track that this was used by NATS Streaming and recreate NATS Streaming connection and subs.

[seriousben]

@kozlovic yes at the end of the day for us this is for Nats Streaming. For the workaround, we were thinking of having to recreate the nats streaming client as well and yes that would mean recreating the subs.

That is why I am a bit active here today, because if we can find a good solution that helps others and does not involve implementing that workaround in multiple languages. It will make me so happy :)

[seriousben]

Please let us know what your thoughts are on the different approaches.

[derekcollison]

Been thinking about it for a bit. Apologies for the slow response. I think we should follow the Nkey pattern and have the client lib never hold the token and present a callback to the library to retrieve a token before every connect. So I would have option Token() Option either changed to take interface{} or add TokenHandler(func() string) Option such that the NATS lib never actually holds onto the token and always calls into the supplied function to retrieve.

[seriousben]

No worries.

How about this:

• TokenHandler(func() string) and field Options.TokenHandler added
• Options.Token and Token() are marked as deprecated in favor of the Handler.

[derekcollison]

I think if people have a token that is long lived their code should work as is by setting it once before connect. So would not deprecate that.

[derekcollison]

Squash all these down into one and I will take a look..

[seriousben]

In nats-io/nats.c#194 I realized that we should expose the Nats Connection here to be consistent and to allow for more flexibility (i.e: URL specific tokens)

I suggest changing func() to func(*Conn)

@derekcollison does that make sense to you as well?

[derekcollison]

I added @kozlovic to that one since he maintains the C client.

[kozlovic]

I would advise against that. The token callback is invoked synchronously from the connect/reconnect thread under the connection lock, which means that if user invokes almost any connection's function, then it will deadlock (since there is no re-entrant lock support).

It is actually something that should be clearly stated for users planning to use this callback.

Alternatively, we would have to release/reacquire lock around the invocation of the callback, but that scares me since there is state that then could change and may need to be re-evaluated upon reacquiring the lock.

[seriousben]

Thanks a lot. Very good point!

[nicholaslam]

Thanks for all the feedback. I think this PR is ready now.

[derekcollison]

I think if you define tokenHandler and token should be a config error. Maybe check for others like defining a u/p and tokenHandler as well.

[derekcollison]

I think the test case where you define a token and a tokenHandler should fail locally in the client with a config error.

…

On Mon, Nov 12, 2018 at 8:31 AM Nicholas Lam ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/auth_test.go <https://github.com/nats-io/go-nats/pull/405#discussion_r232726629>: > + + tokenURL := ***@***.***:8232", secret) + nc, err := nats.Connect(tokenURL) + if err != nil { + t.Fatal("Should have connected successfully") + } + nc.Close() + + // Use Options + nc, err = nats.Connect("nats://localhost:8232", nats.TokenHandler(func() string { return secret })) + if err != nil { + t.Fatalf("Should have connected successfully: %v", err) + } + nc.Close() + // Verify that token in the URL takes precedence. + nc, err = nats.Connect(tokenURL, nats.TokenHandler(func() string { return "badtoken" })) OK. Should TestTokenAuth also be changed to return an error for this case? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/nats-io/go-nats/pull/405#discussion_r232726629>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFf8UHv9Hzei51DOU-XOs0nkVusHejXks5uuaJMgaJpZM4YSiui> .

[nicholaslam]

Sorry for deleting my comment. I understood what you meant afterwards. I'll implement your suggestions. Thanks for the feedback.

[nicholaslam]

@derekcollison I've implemented the suggestions. I moved the check out of the else block. I also wrote validation code in the Token and TokenHandler setters to prevent misuse outside of the context of the connectProto function.

However, I think that tokens provided via the URL should take precedence a few reasons. Firstly, the behavior between the Token and TokenHandler would be consistent. Secondly, the validation code could be contained within the setters. Finally, I think this respects what is documented in the readme:

// Note that if credentials are specified in the initial URLs, they take
// precedence on the credentials specfied through the options.
// For instance, in the connect call below, the client library will use
// the user "my" and password "pwd" to connect to locahost:4222, however,
// it will use username "foo" and password "bar" when (re)connecting to
// a different server URL that it got as part of the auto-discovery.
nc, err = nats.Connect("nats://my:pwd@localhost:4222", nats.UserInfo("foo", "bar"))

Thanks for your patience and feedback.

[derekcollison]

This looks good, will have @kozlovic take one last look. Thanks!

[kozlovic]

We need to think about the implications. What the README says is that if you provide an URL with username/password or token in the URL, then this is what should be used when trying to connect. However, when connecting to a cluster, the client library is sent the list of URLs it can connect to but those URLs obviously do not contain user information. This is what UserInfo() option is for, to specify the credentials to use when connecting to a server that was discovered, as opposed to using an URL provided by the user.

So it is not an hard requirement to always use what is in the URL, but instead to understand that the client library will have URLs that won't have credentials in them, and so in that case we needed a way to get them through other means.

This PR would behave in that if a token handler is specified, always use that handler to override any token value that may be set in the URL (not in the option since we will fail early if we find it set in Options and in TokenHandler).

[nicholaslam]

I triggered another build, but I'm getting inconsistent results. Are there any known flaky tests?

[kozlovic]

I have recycled the build, let's see if we get green this time. Yes, there are some flaky tests unfortunately (especially on Travis, almost never fail locally).

[nicholaslam]

@derekcollison @kozlovic Thanks for rebuilding.

[derekcollison]

Thanks!