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

Rsa signature support #315

Merged
merged 7 commits into from
Oct 9, 2017
Merged

Conversation

michallula
Copy link

In this fork I added support for RSA256 algorithm. So far only symetric algorithm HMAC was supported. Thanks to support of RSA256, the plugin can be used togheter with other components based on oauth2 using JWT signed tokens. This is the standard approach for spring-boot based applications using spring-security-oauth2 library.

@alvarosanchez
Copy link
Contributor

Thanks a lot for your PR!

Don't you think that #294 fulfils the same requirement? See section 9.1.2 at http://alvarosanchez.github.io/grails-spring-security-rest/next/docs/index.html#_jwt

@michallula
Copy link
Author

Hmm, I'm not sure. I think that issue #294 covers only token generation. My goal was to create grails client to OpenID Connect service provider. This service produces token signed with RSA256 algorithm and I wasn't able to verify the signature in current plugin version. Here is a block of code responsible for token signature validation:

  if (jwt instanceof  SignedJWT) {
        log.debug "Parsed an HMAC signed JWT"
        SignedJWT signedJwt = jwt as SignedJWT
        if(!signedJwt.verify(new MACVerifier(jwtSecret))) {
            throw new JOSEException('Invalid signature')
        }
    }

JwtService.groovy

As you can see, there is hardcoded MACVerifier which using jwtSecret. Current code assumes that token are signed with HMAC algorithm and there is no possibility to verify token signed with other algorithms.

@alvarosanchez alvarosanchez merged commit 45cdd97 into grails:develop Oct 9, 2017
@alvarosanchez
Copy link
Contributor

@michallula while I merged it initially, after looking at the changes more carefully, I'm afraid I had to revert it.

The approach you've taken is great, however, I miss documentation on how to use it, and more importantly, tests. Considering that there are new configuration values that can be used, it could be a matter of defining a new feature in any of the JWT test apps. I can walk you through on how to do it if you are still interested.

Sorry that it's taken forever to review your PR. I apologise for that.

alvarosanchez added a commit that referenced this pull request Oct 11, 2017
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