Skip to content

Conversation

@uniphil
Copy link

@uniphil uniphil commented May 4, 2025

Inter-Service Authentication (JWT) from the XRPC doc specifies that the user's DID is passed in the iss (issuer) field. The JWT from the bearer token that the client receives from the PDS happens to put it in sub (subject) but this token is meant to be opaque to clients and confidential to the PDS. I don't think third-party appviews can ever use that token safely, since only the PDS can verify it.

This change makes AppViewLite look instead for Issuer which should make xrpc-proxied requests work, but note that it's still not verifying the JWT, so these tokens can be trivially forged.

Assuming this works, the token can be verified with the users's pubkey, and then this will be closer to being safe.

[Inter-Service Authentication (JWT)](https://atproto.com/specs/xrpc) from the XRPC doc specifies that the user's DID is passed in the `iss` (issuer) field. The JWT from the bearer token that the client receives from the PDS happens to put it in `sub` (subject) but this token is meant to be opaque to clients and confidential to the PDS. I don't think third-party appviews can ever use that token safely, since only the PDS can verify it.

This change makes AppViewLite look instead for `Issuer` which should make xrpc-proxied requests _work_, but note that it's still not verifying the JWT, so these tokens can be trivially forged.

Assuming this works, the token can be verified with the users's pubkey, and then this will be closer to being safe.
@alnkesq
Copy link
Owner

alnkesq commented May 4, 2025

I logged into my PDS, and the JWT I got back had Subject=(my did) and Issuer=null.
Given that the address of the PDS comes from the PLC directory (and writing there on my behalf requires my own private key), is there really any security issue here?

Additionally, TryGetSessionCookie is already known to return untrusted/non-verified data (the original pre-XRPC version simply returns the cookie that you sent, in the format unverifiedDid=sessionId).

@uniphil
Copy link
Author

uniphil commented May 5, 2025

the JWTs your client (social-app/deer/) gets from your PDS are opaque bearer tokens meant only for the PDS

Clients should treat the tokens as opaque string tokens: the JWT fields and semantics are not a stable part of the specification.

so i think it's a few problems to send this token to an AppViewLite instance:

  1. the Subject is not guaranteed to be there at all (and different PDS implementations can do something entirely different for this token)
  2. it leaks the client's bearer token to the AppViewLite instance, giving AppViewLite the client's whole session permission against the PDS
  3. i don't think it's possible for AppViewLite to verify this token, so it can be forged (which is probably mostly fine if AppViewLite is acting read-only and discarding it after extracting the DID)

Instead, clients should proxy requests through their PDS to the intended AppView (eg setting the atproto-proxy: did:web:appview.bad-example.com#bsky_appview. when the PDS forwards the request to the AppView, it generates a new JWT token for that request, which is well-defined in the specs ("Inter-Service Authentication (JWT)" section), and verifiable with the user's pubkey.

This is where Issuer gets added.

It does make it really annoying for local app development, since your client can't just make a local network request to the AppView -- it goes through the PDS so you need to have a service DID resolveable that points back to a PDS-accessible dev instance.

soooo needs ngrok, tailscale funnel, or cloudflare's thing, or dynamic dns with some router port forwarding 😭... plus some public DNS records to resolve the did:web for the local dev appview into that tunnel. i'm not sure if someone has made a good helper for this yet. feels like a locally-running dummy PDS that just does local appview proxying might be enough.

i think i'm going to capture some forwarded requests and just CURL them at the xrpc endpoints directly for now, skipping social app entirely to debug appview.

@uniphil uniphil mentioned this pull request May 5, 2025
@alnkesq
Copy link
Owner

alnkesq commented May 10, 2025

As a stopgap measure, the DID is now fetched from Issuer or Subject, whatever is set and valid.
I didn't merge your PR directly because then it would break DID extraction for Bluesky PDSes.
c2b8aa4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants