Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use standard/existing JWT claims #736

Closed
dannylamb opened this issue Oct 18, 2017 · 16 comments
Closed

Use standard/existing JWT claims #736

dannylamb opened this issue Oct 18, 2017 · 16 comments
Assignees
Milestone

Comments

@dannylamb
Copy link
Contributor

Currently we use purely custom claims for JWT whose structure are drupal based: uid, roles, etc...

There are standard/existing claims out there we can utilize instead. First step is to see if any of the undelrying libraries we use in Java and PHP support them.

@acoburn
Copy link
Contributor

acoburn commented Oct 18, 2017

The PHP library allows setting both standard and custom claims (the code is already doing some of both), so changing the custom claims to standard claims is just a matter of changing one string to another.

On the Java side, I have had good luck using the Java io.jsonwebtoken/jjwt library. It allows for arbitrary claims, but it makes it easy if you're using standard claims. For instance: https://static.javadoc.io/io.jsonwebtoken/jjwt/0.9.0/io/jsonwebtoken/Claims.html

This means that reading the claims is as simple as Claims::getSubject() or Claims::getIssuer(). And for custom claims, it's like this: Claims::get("webid", String.class)

@whikloj whikloj self-assigned this Jan 16, 2018
@whikloj
Copy link
Member

whikloj commented Jan 17, 2018

In playing with this I was back inside Syn and I want to try converting it to a ServletFilter. This removes the dependencies on Tomcat. But also removes the availability of the Digester. So then I looked at replacing the syn-settings.xml with a YAML file. Just wondering if this seems like a bad idea from anyone?
Using jackson I was able to pretty easily parse this into our current data structures for one of the existing unit tests.

---
version: 1
site:
  url: http://test.com
  algorithm: HS256
  encoding: plain
  key: test data

@dannylamb
Copy link
Contributor Author

@whikloj No tomcat dependency and yaml instead of xml? Works for me.

@acoburn
Copy link
Contributor

acoburn commented Jan 17, 2018

@whikloj 👍 x 100

@whikloj
Copy link
Member

whikloj commented Jan 17, 2018

Ok, I'll move forward with this tentatively as I'm a novice at it. This will mean we will want to alter Crayfish to be able to handle the same YAML structure.

When I have something in a PR form I'd appreciate all input and suggestions.

@acoburn
Copy link
Contributor

acoburn commented Jan 17, 2018

There are a number of servlet filters defined in Trellis: https://github.com/trellis-ldp/trellis/tree/master/trellis-http/src/main/java/org/trellisldp/http (look for any class that ends in Filter.java. You may be particularly interested in the AgentAuthorizationFilter which reads from the security context -- your code would just do the inverse: setting the value of your security Principal.

An example where I set the Principal from JWT claims is here: https://git.io/vNBmN (though that code is in the context of a dropwizard.io application)

@whikloj
Copy link
Member

whikloj commented Jan 17, 2018

@acoburn being that this (I think) has to be a ServletFilter as we will use this to secure Fedora. What I have seen is people that read the JWT and if it authenticates basically pass along the chain a "fake" Principal with the parsed name and roles...

I like your AgentAuthorizationFilter but can I use that as a standalone Filter to be used outside of a full JAX-RS application? Like I said, this is really new to me.

@acoburn
Copy link
Contributor

acoburn commented Jan 17, 2018

@whikloj as I understand it, if you want your code to be independent of Tomcat, you will want to make use of JAX-RS interfaces (JAX-RS being the successor of the Servlet API). That is, while it's possible to just use the servlet-api, I'd recommend using JAX-RS. Also, if this is something that would run alongside Fedora, you will already have Jersey (a JAX-RS impl) available.

@ajs6f
Copy link

ajs6f commented Jan 17, 2018

@acoburn, I'm not sure that is a good move. Whether or not we think of JAX-RS as in some sense a successor to the Servlet API, the Servlet API is alive and well, and it is what tomcat/Jetty/etc. implement-- not JAX-RS. This is what @whikloj ultimately would implement there.

What brings a JAX-RS impl into the mix is a specific impl of Fedora, and writing to an impl is not what I would prefer to see. Staying agnostic with the Servlet API seems stronger to me.

@whikloj
Copy link
Member

whikloj commented Jan 17, 2018

@acoburn @ajs6f So my reading seems to indicate that either the new JAX-RS methods (like ContainerRequestFilter) or the older ServletFilter are both just implementations of the Filter. If I go with a straight ServletFilter...the work should be transferable if we decide later to use the fancier JAX-RS methods? right?

@ajs6f
Copy link

ajs6f commented Jan 17, 2018

They are not the same, but the business logic that you write for either should be reusable. The real work is writing the filtering logic. The rest is packaging. If we decide to switch later, it's not a huge deal.

If you expect to wire this filter in via web.xml, you should use the Servlet API (which is entirely independent of Tomcat).

@acoburn
Copy link
Contributor

acoburn commented Jan 17, 2018

@ajs6f thanks for chiming in here (I was hoping you would). I agree that the servlet-api Filter is the way to go here.

@whikloj
Copy link
Member

whikloj commented Jan 17, 2018

ok cool, I'll work with the Servlet API and see what kind of damage I can do there. Thanks for your assistance @acoburn and @ajs6f

@ajs6f
Copy link

ajs6f commented Jan 17, 2018

Stands up on creaky legs, toddles off grumbling "Plain old Servlet API was good enough for my grand-dad, it's good enough for me! Damn kids with their fancy resource-mapping-based MVC style and their magic annotations and dynamic bindings!
My hero

@whikloj
Copy link
Member

whikloj commented Jan 17, 2018

Ok back to the actual subject of this ticket.

So here is what I am thinking and let me know if this makes sense. Some of these might be dropped later, but for now.

Current claim Description New claim
id Drupal user number webid
name Drupal user name sub
url Drupal hostname iss
roles Drupal roles roles

For the id -> webid, we can consider either dropping it or altering it to be a full URI to the users account.

@dannylamb
Copy link
Contributor Author

Resolved via 9e42c33

@whikloj whikloj added this to the 1.0.0 milestone May 9, 2019
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

No branches or pull requests

4 participants