Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add JWT documentation and improve sample config #7776

Merged
merged 8 commits into from
Jul 6, 2020
Merged

Add JWT documentation and improve sample config #7776

merged 8 commits into from
Jul 6, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented Jul 2, 2020 •
edited
Loading

This does a few somewhat related things:

  • Adds a document describing how the JWT login should work (this is meant to be "spec-like", but is non-standard). See docs/jwt.md in this PR.
  • Improves the information in the sample configuration for JWT.
  • Adds more type information to the login REST code.
  • Makes the do_jwt_login and do_token_login methods private (to match _do_other_login).

clokep
clokep commented Jul 2, 2020
View reviewed changes
docs/dev/jwt.md Outdated
Comment on lines 27 to 28
(Note that this differs from the token based logins which return a
`403 Forbidden` and an error code of `M_FORBIDDEN` if an error occurs.)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably update the code to be the same as this, but I wanted to keep this PR to documentation only.

@clokep clokep marked this pull request as ready for review July 2, 2020 17:51
@clokep clokep requested a review from a team July 2, 2020 17:57
@clokep clokep mentioned this pull request Jul 2, 2020
Closed
kegsay
kegsay reviewed Jul 2, 2020
View reviewed changes
synapse/config/jwt_config.py
# password database.
#
# Each JSON Web Token needs to contain a "sub" (subject) claim, which is
# used as the localpart of the mxid.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no guarantee the sub will meet mxid character restrictions. How do we map under this scenario?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it is assumed that it meets the restrictions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this again since I wasn't 100% sure this was true and the string just gets jammed into one of our UserID objects:

synapse/synapse/rest/client/v1/login.py

Lines 376 to 380 in 21a212f

user = payload.get("sub", None)
if user is None:
raise LoginError(401, "Invalid JWT", errcode=Codes.UNAUTHORIZED)
user_id = UserID(user, self.hs.hostname).to_string()

So yeah, this is essentially completely unchecked. 😢

docs/sample_config.yaml Show resolved Hide resolved
erikjohnston
erikjohnston approved these changes Jul 3, 2020
View reviewed changes
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

docs/jwt.md

The `jwt` should encode the local part of the user ID as the standard `sub`
claim. In the case that the token is not valid, the homeserver must respond with
`401 Unauthorized` and an error code of `M_UNAUTHORIZED`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am somewhat tempted to cheekily change this to a 400. If we ever move login to be more UIA like we won't be able to use 401

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so similar to my comment:

I think we should probably update the code to be the same as this, but I wanted to keep this PR to documentation only.

Doing this likely makes sense, but the first step was to document what is going on.

@richvdh
Copy link
Member

richvdh commented Jul 3, 2020

🎉

Indeed. It makes me very happy to see this documented so clearly.

clokep
clokep commented Jul 6, 2020
View reviewed changes
docs/jwt.md
Comment on lines +63 to +64
secret: "my-secret-token"
algorithm: "HS256"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note also that I did test this using a private key / public key pair, but for testing using the shared secret is much simpler.

@clokep clokep merged commit 2a266f4 into develop Jul 6, 2020
@clokep clokep deleted the clokep/jwt-docs branch July 6, 2020 12:31
@clokep clokep mentioned this pull request Jul 14, 2020
Merged
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit '43726783e' into dinsic
530c8a5 
* commit '43726783e': (22 commits)
  1.17.0rc1
  Fix some spelling mistakes / typos. (#7811)
  `update_membership` declaration: now always returns an event id. (#7809)
  Improve stacktraces from exceptions in background processes (#7808)
  Fix `can only concatenate list (not "tuple") to list` exception (#7810)
  Pass original request headers from workers to the main process. (#7797)
  Generate real events when we reject invites (#7804)
  Add `HomeServer.signing_key` property (#7805)
  Revert "Update the installation docs on apt-transport-https (#7801)"
  Do not use simplejson in Synapse. (#7800)
  Stop passing bytes when dumping JSON (#7799)
  Update the installation docs on apt-transport-https (#7801)
  shuffle changelog slightly
  Change Caddy links (old is deprecated) (#7789)
  Stop populating unused table `local_invites`. (#7793)
  Refactor getting replication updates from database v2. (#7740)
  Add libwebp dependency to Dockerfile (#7791)
  Add documentation for JWT login type and improve sample config. (#7776)
  Convert the appservice handler to async/await. (#7775)
  Don't ignore `set_tweak` actions with no explicit `value`. (#7766)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kegsay kegsay kegsay left review comments

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clokep @richvdh @kegsay @erikjohnston