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

Add introspection implementation #925

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

StevePorter92
Copy link

Off the back of the pull request comments laid out here #869. I thought I would take the comments on board, as I am keen to start using the introspection features already started by @fetzi.

@Sephster
Copy link
Member

Thanks for this @StevePorter92 and apologies for not picking this up yet. I plan on setting some time aside this weekend to pick up some of the pending PRs. Thanks for your patience and thank you for your contribution

@StevePorter92
Copy link
Author

Hey @Sephster, don't suppose you have had a chance to cast your eye of this?

@Sephster
Copy link
Member

Sorry @StevePorter92 I have not yet. I have been working on numerous things for the library over the past week or so. I'm focussing on getting the PKCE changes into version 8 at the moment as there was at one time 3 separate pull requests trying to address some implemenation concerns around that. Once I've finished this (which is now fairly close to be honest), I will look at getting this reviewed. Thanks for your patience

@StevePorter92
Copy link
Author

Hey @Sephster, no worries at all! 😄

src/AuthorizationServer.php Outdated Show resolved Hide resolved
src/AuthorizationServer.php Outdated Show resolved Hide resolved
src/AuthorizationServer.php Outdated Show resolved Hide resolved
src/ResponseTypes/IntrospectionResponse.php Outdated Show resolved Hide resolved
@Sephster
Copy link
Member

Hi @StevePorter92. Thanks for your patience here. A couple of things spring to mind at the moment which I think are missing from this at the moment:

  • The token doesn't have to be a JWT. Although this server provides JWT bearer tokens out the box, implementers are free to use other formats. I don't think this is handled at the moment so will need to be addressed.
  • I don't believe the introspection end point is protected. We should be protecting this with some form of authentication as it should not be available to public clients. Note that some implementations will put this end point behind a firewall internally so we have to take this into consideration as well.
  • I don't see how the resource server is using this end point. Can you clarify this.

Thanks for your efforts to date. I hope these comments are useful. Cheers!

@StevePorter92
Copy link
Author

Added some changes to address your comments @Sephster.

The Introspector now validates through a IntrospectionValidator interface. I've moved all of the token logic to a BearerTokenValidator class. This should give implementers the freedom to validate introspection requests for other formats.

I'm a little unsure as to the best way to get the resource server to use our new endpoint so could do with being pointed in the right direction. My current line of thought is to create a AuthorizationValidators\IntrospectionValidator class. I think this will just need the URL of the introspection endpoint as well as any information required for authentication?

Also a little unsure on the best way to protect this new endpoint. What are your thoughts on this?

Thanks again.

@ThaDaVos
Copy link

So, I'm currently sort of in the need of introspection because I want to use Express Gateways OAuth 2.0 Introspection functionality with my Lumen 5.8 + Passport based Microservice - any eta on the implementation of Introspection?

@ThaDaVos
Copy link

ThaDaVos commented Mar 13, 2019

Sorry for the pings but, anything @StevePorter92 or @Sephster ?

@Sephster
Copy link
Member

It is awaiting review from myself. I will get to it eventually but don't have a firm timeline for this at present sorry.

@ThaDaVos
Copy link

Ah okay, currently got it half working by using a separate package - only running into one problem which is that the Authorization header seems to be required to have to validation in the BearerTokenValidator succeed, even though it is in the request body...
Apparently it seems Express Gateway doesn't put it into the Header so I'm a bit stuck and I don't know if this is a Bug in the BearerTokenValidator or not because Express Gateway talks about using a combination of Client_Id:Client_secret as Token Validation Value or something

@ThaDaVos
Copy link

So when can we expect this to be implemented @Sephster - cause currently it's a big stopper when you want to use the Microservices Architecture with Laravel or Lumen

Or do you have an alternative/workaround?
I tried overriding the ResourceServer so I can return a custom validator (actually just a modified version of the BearerTokenValidator so it looks in the body instead of the header - where it should actually check when using introspection - but... a few needed class members are private...
authorizationValidator and accessTokenRepository

1 similar comment
@ThaDaVos
Copy link

So when can we expect this to be implemented @Sephster - cause currently it's a big stopper when you want to use the Microservices Architecture with Laravel or Lumen

Or do you have an alternative/workaround?
I tried overriding the ResourceServer so I can return a custom validator (actually just a modified version of the BearerTokenValidator so it looks in the body instead of the header - where it should actually check when using introspection - but... a few needed class members are private...
authorizationValidator and accessTokenRepository

@StevePorter92
Copy link
Author

As @Sephster said, there isn't currently a timeline for this feature @dvdbot.

Introspection can be achieved with laravel through passport and https://github.com/designmynight/laravel-oauth-introspect-middleware.

Currently what's outstanding is finding a sensible way for the resource server to use introspection. We also need to consider authentication.

@Sephster, when you come to review this, let me know if there's anything I can pick up.

@Sephster
Copy link
Member

Will do, thanks @StevePorter92. I should have some free time this evening. Unfortunately @dvdbot, this isn't just a case of reviewing the code. I also need to familiarise myself with the RFC to ensure it is conforming to spec which is why PRs such as this take longer than a normal bug fix. I won't be drawn on timescales as I find they are nearly always optimistic. Hope you understand and sorry I can't give a more definitive answer than that

@ThaDaVos
Copy link

@StevePorter92 and @Sephster - I've got it working for the moment as follows:
I copied the controller from the Laravel Introspection package, then I copied the token validation part from the BearerTokenValidator into the controller, added a public key variable which gets instantiated as follows

$this->publicKey = new CryptKey('file://'.Passport::keyPath('oauth-'. 'public' .'.key'), null, null);

Above I stole from somewhere inside the Passport package 😂

Now I can at least introspect

{
if ($this->introspectionValidator instanceof IntrospectionValidatorInterface === false) {
$this->introspectionValidator = new BearerTokenValidator($this->accessTokenRepository);
}

Choose a reason for hiding this comment

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

The BearerTokenValidator has no validation-key set, unless you also call here

$this->introspectionValidator->setPrivateKey($this->privateKey);

But that also requires that the public-key is also added to the private-key file, as Lcobucci\JWT\Signer\Rsa::doVerify($expected, $payload, Key $key) calls open_ssl_publickey($key->getContent())!

With that change I was able to verify/introspect access_keys generated by my server.

I'm currently implementing an OpenID Connect / OAuth2 server for EGroupware

@canfone
Copy link

canfone commented Aug 7, 2019

I'm just wondering why you put the introspection logic in the authorization server? Shouldn't it be in the resource server? Then create a new validator IntrospectionValidator maybe then apply same logic as the BearerTokenValidator but except it doesn't require to be have Authorization header?

@arondeparon
Copy link

Is this feature still being actively considered for development?

@Sephster
Copy link
Member

Yes. Rough plan is to get Device Code in place and then this. January is super busy for me as I need to do tax returns etc but will be picking up speed on this soon. Cheers

@Veitor
Copy link

Veitor commented Apr 14, 2020

How is the progress?

@ThaDaVos
Copy link

Sorry to comment, but any ETA?

@mikados
Copy link

mikados commented May 25, 2021

Sorry to comment. How is the progress?

@abbluiz
Copy link

abbluiz commented Jul 22, 2021

How can I help to make this happen? I think this feature would be very useful.

@bram-pkg
Copy link

I need this to create separate auth and resource servers in a project at work. Any updates? How can I help?

@Sephster Sephster added this to the 9.00 milestone Feb 22, 2022
@Sephster
Copy link
Member

I'm working on the 9.0 release right now so have added this to the milestone but I don't expect this to be out for at least a month, maybe more sorry

@saade
Copy link

saade commented Jun 13, 2024

Will this ever be implemented?

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.