Add callback to NatsAuthOpts that allows refreshing a Token #712

garrett-sutton · 2025-01-13T22:29:48Z

Adds the following to NatsAuthOpts

Func<Uri, CancellationToken, ValueTask<NatsAuthCred>>? AuthCredCallback

The purpose of this PR is to allow for use cases to refresh a Token, JWT, or NKey associated with their NatsConnection during a reconnect scenario.

resolves #356

garrett-sutton · 2025-01-13T23:23:45Z

@mtmk are you the right person to review this? Or is there someone else that might be good to reach out to? Thanks!

src/NATS.Client.Core/Internal/UserCredentials.cs

mtmk · 2025-01-14T10:34:07Z

@garrett-sutton could you sign your commits please?

garrett-sutton · 2025-01-14T23:11:36Z

@garrett-sutton could you sign your commits please?

Done. As a note, this should probably be called out in CONTRIBUTING.md. I missed this initially because I was only looking there.

src/NATS.Client.Core/Internal/UserCredentials.cs

caleblloyd · 2025-01-16T19:11:52Z

The most recent options callback I reviewed did supply a URI and a CancellationToken as an argument

nats.net/src/NATS.Client.Core/NatsWebSocketOpts.cs

Lines 24 to 28 in caa4bef

    
           /// <summary> 
        
           /// An optional async callback handler for manipulation of ClientWebSocketOptions used for WebSocket connections. 
        
           /// Implementors should use the passed CancellationToken for async operations called by this handler. 
        
           /// </summary> 
        
           public Func<Uri, ClientWebSocketOptions, CancellationToken, ValueTask>? ConfigureClientWebSocketOptions { get; init; } = null;

API usability point: should there be a single callback rather than individual ones?

Sounds like a good idea, a signature could be

In NatsAuthOpts:

   Func<Uri, CancellationToken, ValueTask<NatsAuthOpts>? Callback { get; init; } = null;

This would have to come with the caveat that a Callback could not return another NatsOptsAuth with a Callback

Or in NatsOpts:

   Func<Uri, CancellationToken, ValueTask<NatsAuthOpts>? AuthOptsCallback { get; init; } = null;

Could also be a slipper slope though, what other options could be updated between reconnects? And which ones could be updated after knowing the URI that will be connected to? Is it worth putting just the auth ones in now, or coming up with a broader approach to allow for updating all potential options

mtmk · 2025-01-17T04:44:11Z

Yes we have started to pass in CT in our callbacks only recently so we should carry on with that. I agree passing back the whole NatsAuthOpts is tricky. I was thinking of a simpler enum with a new type:

enum NatsAuthType { Token, Jwt, NKey, Seed }

struct NatsAuthKey(NatsAuthType type, string Value)

Func<Uri, CancellationToken, ValueTask<NatsAuthKey>? AuthKeyCallback { get; init; } = null;

I feel name AuthKey may not be the name we end up with but this structure would be my proposal.

garrett-sutton · 2025-01-17T17:24:26Z

Yes we have started to pass in CT in our callbacks only recently so we should carry on with that. I agree passing back the whole NatsAuthOpts is tricky. I was thinking of a simpler enum with a new type:
enum NatsAuthType { Token, Jwt, NKey, Seed }

struct NatsAuthKey(NatsAuthType type, string Value)

Func<Uri, CancellationToken, ValueTask<NatsAuthKey>? AuthKeyCallback { get; init; } = null;
I feel name AuthKey may not be the name we end up with but this structure would be my proposal.

I like this approach. One follow-up question though. For using something like this, do we actually need to return an array of NatsAuthKey from the callback?

I expect that if the JWT or the NKey change that the Seed should also change. Is that correct? If so, I think we need to provide a way for implementers to specify that multiple things need to be updated.

mtmk · 2025-01-17T19:22:51Z

Yes we have started to pass in CT in our callbacks only recently so we should carry on with that. I agree passing back the whole NatsAuthOpts is tricky. I was thinking of a simpler enum with a new type:
enum NatsAuthType { Token, Jwt, NKey, Seed }

struct NatsAuthKey(NatsAuthType type, string Value)

Func<Uri, CancellationToken, ValueTask<NatsAuthKey>? AuthKeyCallback { get; init; } = null;
I feel name AuthKey may not be the name we end up with but this structure would be my proposal.
I like this approach. One follow-up question though. For using something like this, do we actually need to return an array of NatsAuthKey from the callback?

I expect that if the JWT or the NKey change that the Seed should also change. Is that correct? If so, I think we need to provide a way for implementers to specify that multiple things need to be updated.

you're right. we'd need to pass seed/secret next to value. how about this?

enum NatsAuthType { Token, Jwt, NKey, UserInfo }
 
struct NatsAuthKey(NatsAuthType type, string Value, string Secret)

caleblloyd · 2025-01-17T23:15:07Z

you're right. we'd need to pass seed/secret next to value. how about this?

I think the combination of things they may want to supply is:

Username + Password at same time
Token
Seed (from which we can derive the NKey, so no need to really supply the NKey)
Jwt + Seed at same time
NkeyFile
CredsFile
No Auth

Too bad TypeUnions aren't in C# yet, it'd be nice to have records for

NatsAuthenticator.UsernamePassword(string Username, string Password)
NatsAuthenticator.Token(string Token)
NatsAuthenticator.NKey(string Seed)
NatsAuthenticator.Jwt(string Jwt, string Seed)
NatsAuthenticator.NkeyFile(string NKeyFile)
NatsAuthenticator.CredsFile(string CredsFile)

caleblloyd · 2025-01-17T23:46:09Z

src/NATS.Client.Core/Internal/UserCredentials.cs

+            case NatsAuthType.Nkey:
+                opts.NKey = a.Value;
+                seed = a.Seed;
+                break;


NKeys and Seeds are a pair. It was probably a mistake in the initial implementation to require passing both, as the NKey can be derived from the seed. I guess it acts as a discriminator so we can tell it apart from JWT + Seed

In the latest, I used the "add a seed" and we can derive the nkey from it approach: 21c5e74

caleblloyd · 2025-01-17T23:46:52Z

src/NATS.Client.Core/Internal/UserCredentials.cs

+                opts.JWT = a.Value;
+                seed = a.Seed;
+                break;


JWTs and Seeds are a pair, the Sub in the JWT is the public NKey for the seed

In the static factory methods, I implemented it such that you need to input the jwt and seed for jwt auth but only the seed for nkey auth: 21c5e74

mtmk · 2025-01-18T05:17:53Z

you're right. we'd need to pass seed/secret next to value. how about this?

I think the combination of things they may want to supply is:

Username + Password at same time

Token

Seed (from which we can derive the NKey, so no need to really supply the NKey)

Jwt + Seed at same time

NkeyFile

CredsFile

No Auth

Too bad TypeUnions aren't in C# yet, it'd be nice to have records for

NatsAuthenticator.UsernamePassword(string Username, string Password)

NatsAuthenticator.Token(string Token)

NatsAuthenticator.NKey(string Seed)

NatsAuthenticator.Jwt(string Jwt, string Seed)

NatsAuthenticator.NkeyFile(string NKeyFile)

NatsAuthenticator.CredsFile(string CredsFile)

that's good analysis! ~~I'm happy with the name NatsAuthenticator~~ (see edit). we can achieve the same syntax using the above enum + struct combo with these static helpers. One question about JWTs: does the seed change as well or just JWT in which case we could have an overload for that:

enum NatsAuthType { None, Token, Jwt, UserInfo, CredFile .... }

struct NatsAuthenticator
{
    NatsAuthType _type;
    string? _value;
    string? _secret;

    private NatsAuthenticator(NatsAuthType, string?, string?) { ... }
    
    public static NatsAuthenticator NoAuth() { ... }

    public static NatsAuthenticator UsernamePassword(string Username, string Password) { ... }

    public static NatsAuthenticator Token(string Token) { ... }

    public static NatsAuthenticator Jwt(string Jwt, string Seed) { ... }

    public static NatsAuthenticator Jwt(string Jwt) { ... } // maybe?

    ...
}

EDIT: actually just scanned through the changes quick. I feel NatsAuthCred and AuthCredCallback are more inline with our minimalist naming scheme.

mtmk · 2025-01-18T05:33:39Z

src/NATS.Client.Core/Internal/UserCredentials.cs

+        string? seed = null;
+        if (AuthCredCallback != null)
+        {
+            using var cts = new CancellationTokenSource(timeout);


We should take in CT from above as param and use it here. if we need the timeout we should link the tokens. The idea is if we're e.g. shutting down callbacks should get the signal as well and quit whatever they might be doing and not hanging.

A question about this. I don't necessarily object to this comment, but I'm not sure of whether 1) we need the timeout and if we do 2) what token to link it to. 3) Otherwise, I'm not sure if it is just OK to pass in CancellationToken.None to this method?

Do you have thoughts on these points?

I took the current approach because it seems to be what other AuthenticateAsync type methods did (i.e. sslConnection.AuthenticateAsClientAsync

You're right we haven't been consistent across the board:

// None public Func<SslClientAuthenticationOptions, ValueTask>? ConfigureClientAuthentication { get; init; } // Uses CTS with Connection timeout - but not passing in dispose token from connection object public Func<Uri, ClientWebSocketOptions, CancellationToken, ValueTask>? ConfigureClientWebSocketOptions { get; init; }

I think we should start using the CT from connection dispose to have a clean shutdown. Should we pass _disposedCancellationTokenSource into Authenticate here:

await _userCredentials.AuthenticateAsync(_clientOpts, WritableServerInfo, _currentConnectUri, Opts.ConnectTimeout, _disposedCancellationTokenSource.Token).ConfigureAwait(false);

then we can link them before passing to the callback here e.g.:

using var cts = new CancellationTokenSource(timeout); #if NETSTANDARD using var ctr = cancellationToken.Register(static state => ((CancellationTokenSource)state!).Cancel(), cts); #else await using var ctr = cancellationToken.UnsafeRegister(static state => ((CancellationTokenSource)state!).Cancel(), cts); #endif var authCred = await AuthCredCallback(uri.Uri, cts.Token).ConfigureAwait(false);

Ok, that makes sense! Thank you for the help on this. Those changes are reflecting in the most recent update of the commit with functional changes: 9088040

src/NATS.Client.Core/NatsAuthOpts.cs

caleblloyd

I like how the API turned out!

caleblloyd · 2025-01-22T16:17:32Z

src/NATS.Client.Core/Internal/ClientOpts.cs

+    public string? Username { get; set; } = null;

    /// <summary>Connection password (if auth_required is set)</summary>
    [JsonPropertyName("pass")]
-    public string? Password { get; init; } = null;
+    public string? Password { get; set; } = null;


Can these two be switched back to init?

I believe they need to have setters because without them the callback can't allow for a client to specify a new username and password (i.e. we are passing in an existing instance of ClientOpts to the AuthenticateAsync method that invokes the new callback).

yes as @garrett-sutton said they are being set here

We should probably make ClientOpts a readonly record all together and return a new ClientOpts object from the auth method. that may be too much change for this PR we can follow up.

garrett-sutton · 2025-01-23T23:13:47Z

@mtmk at this point, do you have a feeling on how close we are to approval/merging?

And as a follow-up, how long can I expect until a new release is cut with these changes?

mtmk · 2025-01-23T23:34:13Z

@garrett-sutton I'd like to allow a few days for others to have a chance to review and comment. All being well, I'd say we can merge and release early next week.

garrett-sutton · 2025-01-24T03:01:16Z

@mtmk sounds good. I'm looking to utilize the new feature in a new project at my org that we're trying to release soon. I just wanted to check in on where we are at. Thanks!

mtmk

sorry @garrett-sutton a few last minute changes from me nothing too big hopefully.

mtmk · 2025-01-24T03:18:03Z

src/NATS.Client.Core/NatsAuthOpts.cs

+
+    internal string? Secret { get; }
+
+    public static NatsAuthCred NoAuth() => new(NatsAuthType.None, string.Empty, string.Empty);


we should remove this. to me it's not well defined what it's for and I don't believe there is a known use case for it now. I suggest we take it out to avoid confusion unless I'm missing something (sorry I may have suggested it, not sure).

I think the use case was you had auth before, but you don't now. Or you have a pool of servers, some with auth and some without. So when you see the Url that is being connected to, you can specify no auth if you would like.

I believe this could be a public static readonly NatsAuthCred None though if you like that better

agreed. I removed this in the latest iteration of the functional commit: 333c03b

@caleblloyd Do you have a sense for how likely that workflow could be? I'm torn here that yes, this is technically possible but it seems much less likely than the other workflows. We're also not taking away someone's ability to connect w/o auth.

Curious to hear your thoughts.

Also, impeccable timing on your comment and mine.

I don't think switching auth methods is all that common, but I do like how this Struct represents the Union of all available Auth types. I could see us using it in the future to construct a NatsAuthOpts, so it'd be nice to have the NoAuth option. Also since it's a Struct that does represent an accurate default for an empty struct that is declared. (well it did when the NatsAuthCred enum had None as the default 😄

mtmk · 2025-01-24T03:18:20Z

src/NATS.Client.Core/NatsAuthOpts.cs

@@ -1,5 +1,47 @@
 namespace NATS.Client.Core;

+public enum NatsAuthType


could this be made internal?

changed in the latest iteration of the functional commit: 333c03b

mtmk · 2025-01-24T03:18:48Z

src/NATS.Client.Core/NatsAuthOpts.cs

@@ -1,5 +1,47 @@
 namespace NATS.Client.Core;

+public enum NatsAuthType
+{
+    None,


we should remove this too as per NoAuth comment elsewhere.

removed in latest iteration of functional commit: 333c03b

Not sure if removing this is a good idea, since NatsAuthCred is a struct if an empty struct is declared, it's going to have the default value of this enum. The None value makes sense there.

mtmk · 2025-01-24T03:20:32Z

src/NATS.Client.Core/NatsAuthOpts.cs

+
+    public static NatsAuthCred NoAuth() => new(NatsAuthType.None, string.Empty, string.Empty);
+
+    public static NatsAuthCred UserInfo(string username, string password)


Can we call these methods FromUserInfo(), FromToken(), ...? I think that would be more inline with other APIs like TimeSpan.

I did this in the latest iteration of the commit with functional changes: 333c03b

Add factory methods for NatsAuthCred

garrett-sutton force-pushed the auth-callback branch from 7bf2f6f to c85fb33 Compare January 13, 2025 23:17

garrett-sutton marked this pull request as ready for review January 13, 2025 23:18

mtmk reviewed Jan 14, 2025

View reviewed changes

src/NATS.Client.Core/Internal/UserCredentials.cs Outdated Show resolved Hide resolved

garrett-sutton force-pushed the auth-callback branch 4 times, most recently from 0bdb854 to fb423fb Compare January 14, 2025 23:06

mtmk reviewed Jan 16, 2025

View reviewed changes

src/NATS.Client.Core/Internal/UserCredentials.cs Outdated Show resolved Hide resolved

garrett-sutton force-pushed the auth-callback branch from fb423fb to 6080576 Compare January 17, 2025 23:33

caleblloyd reviewed Jan 17, 2025

View reviewed changes

mtmk mentioned this pull request Jan 18, 2025

Implement Lame Duck Mode Event Handler #716

Merged

mtmk reviewed Jan 18, 2025

View reviewed changes

src/NATS.Client.Core/NatsAuthOpts.cs Outdated Show resolved Hide resolved

garrett-sutton force-pushed the auth-callback branch from 6080576 to 8851ab5 Compare January 20, 2025 20:28

garrett-sutton requested review from caleblloyd and mtmk January 21, 2025 22:02

garrett-sutton force-pushed the auth-callback branch from 8851ab5 to a093653 Compare January 22, 2025 15:17

caleblloyd reviewed Jan 22, 2025

View reviewed changes

mtmk requested changes Jan 24, 2025

View reviewed changes

gsutton added 2 commits January 23, 2025 22:25

Create callback for NatsAuthOpts

9dcb623

Add factory methods for NatsAuthCred

Add callback tests

333c03b

garrett-sutton force-pushed the auth-callback branch from a093653 to 333c03b Compare January 24, 2025 04:27


		internal string? Secret { get; }

		public static NatsAuthCred NoAuth() => new(NatsAuthType.None, string.Empty, string.Empty);

		@@ -1,5 +1,47 @@
		namespace NATS.Client.Core;

		public enum NatsAuthType


		public static NatsAuthCred NoAuth() => new(NatsAuthType.None, string.Empty, string.Empty);

		public static NatsAuthCred UserInfo(string username, string password)

Add callback to NatsAuthOpts that allows refreshing a Token #712

Are you sure you want to change the base?

Add callback to NatsAuthOpts that allows refreshing a Token #712

Conversation

garrett-sutton commented Jan 13, 2025 • edited Loading

garrett-sutton commented Jan 13, 2025

mtmk commented Jan 14, 2025

garrett-sutton commented Jan 14, 2025

caleblloyd commented Jan 16, 2025

mtmk commented Jan 17, 2025

garrett-sutton commented Jan 17, 2025

mtmk commented Jan 17, 2025

caleblloyd commented Jan 17, 2025

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

mtmk commented Jan 18, 2025 • 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

caleblloyd 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

garrett-sutton commented Jan 23, 2025

mtmk commented Jan 23, 2025

garrett-sutton commented Jan 24, 2025

mtmk 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

garrett-sutton commented Jan 13, 2025 •

edited

Loading

mtmk commented Jan 18, 2025 •

edited

Loading