RSDK-11520: Persist app+signaling connection jwts on the robot by EvanDorsky · Pull Request #6384 · viamrobotics/rdk

Evan Dorsky (EvanDorsky) · 2026-08-25T16:33:56Z

This is part of APP-8535 for resilient signaling so that robots can make connections to smart machines even if there is degradation on the app/system.

This branch adds jwt caching to NewAppConn in viam-server by wrapping calls to rpc.DialDirectGRPC in a new method, authedDialDirectGRPC.

authedDialDirectGRPC has largely the same signature and behavior as rpc.DialDirectGRPC - it dials the requested server (app or signaling) and returns an rpc.ClientConn.

However, authedDialDirectGRPC also explicitly authenticates the connection - instead of relying on lazy auth - and then caches the resulting jwt on disk. It proceeds as usual on the happy path, but if - and only if - authentication fails, authedDialDirectGRPC will attempt to use the corresponding cached token to authenticate its connection.

Tokens are cached in $VIAM_HOME/grpc and are named with a (filepath-safe) base64 encoding of the machine's part ID + the host that minted the token.

This branch does not include any cache invalidation i.e. client-side token revocation. I think the server (app/signaling) is the right place for revocation to live, and this change to rdk should continue to work if we change our revocation policy, since we always start by requesting a new token.

While approaching resilient signaling, I went for this change first for a few reasons:

it's an rdk-only change
it doesn't degrade the security story (see below)
it takes advantage of an existing authentication mechanism
it allows machines to connect to the signaling server even when the signaling server can't mint a token but is otherwise working
it also makes app connections more resilient (though we could change this)

Why this doesn't degrade the security story

note: here, "rdk" refers to viam-server and "server" refers to app or signaling. "token" refers to the jwt issued by the server.

rdk provides two pieces of information to the server it's dialing when it authenticates its connection with that server:

robot part id
robot part secret
(there's also a shared secret, but it's generated from these two as far as I understand, so it doesn't count)

The server uses this information to mint a token which it sends back to rdk. rdk caches this token in process memory and uses it to authenticate its connection with the host. Today, the token is lost when viam-server restarts.

Caching the token on disk does not degrade the security story at all, because the robot part id and secret - all we need to get a brand new token from the server - are already stored unencrypted on disk inside the cloud config .json file that robots need to connect to app (this is why they need that file to connect to app).

Also, rdk is open source, so in theory anybody could just fork rdk, build their own binary which cached the token like we do here, and point their robot to that new binary.

Basically, any threat model that involves compromising this cached token (i.e. physical/root access to the machine) also involves compromising the secrets required to request the token.

Caching tokens on disk actually pairs well with moving the robot part ID and secret from the filesystem into a user's env, which - combined with more aggressive token revocation - could open a path to marginally improving the security story.

I'm still new to the auth system so I might have gotten things wrong, will definitely appreciate the review.

Dan Gottlieb (dgottlieb) · 2026-08-25T19:08:08Z

+
+	authenticator, ok := conn.(rpc.ClientConnAuthenticator)
+	if !ok {
+		// we cannot auth this connection, but it might still be valid, so just return it


We don't expect any code paths to hit this? If so, let's log as a warning.

we know the types of everything we're returning (even if it's a bit hidden in goutils), so we should know if something is wrong/unexpected. the comment/log should be more opinionated on that front

Yeah, good idea. I will need to test whether we hit this path when the user has explicitly disabled authentication in their config. If we do, warning level is probably still fine since that's a rare use case.

Dan Gottlieb (dgottlieb) · 2026-08-25T19:10:45Z

+				"error", authErr, "code", code.String())
+			return conn, nil
+		}
+		// auth failed but our token was not explicitly rejected, so check if we have a cached token, then try dialing with it


Can you describe the scenario where this helps?

Dan Gottlieb (dgottlieb) · 2026-08-25T19:13:52Z

+		// dial completely failed, pass the results through for the retry loop
+		return conn, err
+	}
+


The rest of this function omits newlines. 66 lines of concise logic, minus the 1 empty line here. This is a real missed opportunity. /s

Dan Gottlieb (dgottlieb) · 2026-08-25T19:18:00Z

+		return newConn, nil
+	}
+	// just in case we got an empty token, we don't want to overwrite a potentially good cache
+	// this check is most likely unnecessary, but it's cheap and harmless


I disagree this is harmless. I think it's good to call out the assumption that the token is not nil. I think we should log if the string is nil.

Dan Gottlieb (dgottlieb) · 2026-08-25T19:22:25Z

+		// if our auth request gets denied with certain failure codes, it means serverside auth is working fine
+		// and our credentials were just bad. in that case, we want to fail early instead of trying the cached token,
+		// which could mask a real problem with auth
+		if code := status.Code(authErr); code == codes.Unauthenticated || code == codes.PermissionDenied {


I think we need to be clear about what error code "KMS is down" manifests as.

Agreed - I'll be doing testing today to figure this out.

Dan Gottlieb (dgottlieb) · 2026-08-25T19:37:26Z

+// authedDialDirectGRPC calls DialDirectGRPC while also explicitly authenticating the connection
+// and caching the resulting token on disk. If the normal auth path fails, we attempt to auth
+// with the cached token
+func authedDialDirectGRPC(ctx context.Context,


Getting to the bottom of this function, I think the answer to most of the "what case hits this" is "we don't know".

Please let me know if that's not the case for some of them. Assuming I'm right though, I'd prefer to simplify the flow through exactly what we expect. We can certainly log more such that if this isn't working right for a customer, we can ascertain whether those other code paths would have been helpful. But I think we can save some cognitive load by reducing this down to:

Dial to app presenting our robot secret to exchange for a JWT

If that succeeds, write down the new JWT. Exit

If that fails, dial with a cached JWT.

If the cached JWT works, early exit -- there's nothing new to write to disk.

Distinguish between "no cached jwt file" and "cannot read cached jwt file" errors for logging.

I think the code for this function should also be opinionated. Particularly about variable names. So calling out robot secret vs signed JWT. We can relax some on the name there, but I think the vague "token" is too opaque. We want to communicate that this is a signed token. Giving the user intuition why the follow-up might succeed where the original had a problem.

Reading through the code again today, I'm agreeing with you that the variable names should be more opinionated, I think that will make the logic and intent easier to follow.

And I do have ideas for what case would hit most of the error branches in here, but I don't think the comments are doing a good enough job explaining them. Also the happy path is kind of buried by all the error handling. I think better comments + variable names + a bit of refactoring will improve this a lot, and I might be able to get rid of a few cases too. I'll be back with this soon. Also good call on "no cached jwt" vs "can't read jwt".

Distinguish between "no cached jwt file" and "cannot read cached jwt file" errors for logging.

I think logging the error message from jwtCacheRead (new name) will differentiate between these cases, but let me know if you want stronger differentiation than that - this feels like enough to me.

Dan Gottlieb (dgottlieb) · 2026-08-25T19:40:41Z


+func tokenInfo(partID, host string) (string, string) {
+	cacheDir := filepath.Join(rutils.ViamDotDir, "grpc")
+	tokenFilename := base64.RawURLEncoding.EncodeToString([]byte(partID + "_" + host))


Dumb question, what is host? I'm guessing it's app.viam.com or app.viam.dev? Is it just for guaranteed uniqueness as opposed to "statistically unique"?

Yes, those are the possible hosts. I was a little confused here at first because I didn't understand that the signaling server and app share the same url - so I was adding host to the filename to make sure that tokens from different hosts wouldn't overwrite each other.

I still think accounting for host in the filename is still reasonable, since in testing (and maybe at some point in the future?) the app and signaling hosts will sometimes be different.

Cheuk (cheukt)

generally agree with dan's comments, I think the code/comments should be more precise about what can/cannot happen and what is expected/unexpected so that future readers are not taking on the cognitive burden of trying to figure out what might be happening

Cheuk (cheukt) · 2026-08-26T17:09:08Z

+
+	authenticator, ok := conn.(rpc.ClientConnAuthenticator)
+	if !ok {
+		// we cannot auth this connection, but it might still be valid, so just return it


we know the types of everything we're returning (even if it's a bit hidden in goutils), so we should know if something is wrong/unexpected. the comment/log should be more opinionated on that front

Cheuk (cheukt) · 2026-08-26T17:09:31Z

+		// if our auth request gets denied with certain failure codes, it means serverside auth is working fine
+		// and our credentials were just bad. in that case, we want to fail early instead of trying the cached token,
+		// which could mask a real problem with auth
+		if code := status.Code(authErr); code == codes.Unauthenticated || code == codes.PermissionDenied {


Cheuk (cheukt) · 2026-08-26T17:13:19Z

+	if err != nil {
+		return err
+	}
+


probably want to make these atomic - we have an atomic store in goutils

Agree. Claude recommended against pulling in the goutils implementation - since goutils.artifact pulls in all the gcs dependencies which are pretty heavy - so I just replicated the atomic write logic here, which wasn't that bad. Hope that sounds reasonable.

Evan Dorsky (EvanDorsky) · 2026-09-02T19:15:52Z

Re-requested reviews - thanks for all the input. I made the variable names, error messages, and comments a lot more clear and specific to what this change is actually doing. Also factored the retry path into its own function so that the happy path is much more clear.

Evan Dorsky (EvanDorsky) added 8 commits August 21, 2026 15:32

First (broken) pass at token caching

28769d2

Cache token

e95e7b2

Fix some things

8864448

Use token cache

217b10d

Small bugfix

27596ae

gosec nolint with explanation

fdb6439

Tests

286836f

Merge branch 'main' into rsdk-11520-jwt-persist

ad2bd52

Evan Dorsky (EvanDorsky) requested a review from Cheuk (cheukt) August 25, 2026 16:34

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Aug 25, 2026

Evan Dorsky (EvanDorsky) requested a review from Dan Gottlieb (dgottlieb) August 25, 2026 16:34

Evan Dorsky (EvanDorsky) changed the title ~~RSDK-11520: Persist signaling JWTs on the dialer~~ RSDK-11520: Persist app+signaling connection jwts on the robot Aug 25, 2026

Evan Dorsky (EvanDorsky) added 3 commits August 25, 2026 14:35

Don't try the cache if our creds were actually bad

38221db

Fix unused var

451b460

Sandbox/tmp token cache in tests

3365db8

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Aug 25, 2026

Dan Gottlieb (dgottlieb) reviewed Aug 25, 2026

View reviewed changes

Cheuk (cheukt) reviewed Aug 31, 2026

View reviewed changes

Evan Dorsky (EvanDorsky) requested a review from Benjamin Rewis (benjirewis) August 31, 2026 18:15

Refactor, rename variables, better error messages

b365dd0

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 1, 2026

Evan Dorsky (EvanDorsky) added 3 commits September 1, 2026 15:50

More renames

869ede1

Remove unused variable

0bd65c5

More rewriting

f671100

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 1, 2026

Evan Dorsky (EvanDorsky) added 2 commits September 1, 2026 16:47

Do atomic write

db2677f

Do atomic write

88ca2b2

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 1, 2026

Rename func

df4bd53

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 2, 2026

Evan Dorsky (EvanDorsky) requested review from Cheuk (cheukt) and Dan Gottlieb (dgottlieb) September 2, 2026 19:13

Uh oh!

Conversation

Evan Dorsky (EvanDorsky) commented Aug 25, 2026

Why this doesn't degrade the security story

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dan Gottlieb (dgottlieb) Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cheuk (cheukt) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Evan Dorsky (EvanDorsky) commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dan Gottlieb (dgottlieb) Aug 25, 2026 •

edited

Loading