-
Notifications
You must be signed in to change notification settings - Fork 40
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
Introduce Server Security JWT Support #322
base: 4.4.x
Are you sure you want to change the base?
Introduce Server Security JWT Support #322
Conversation
api "com.nimbusds:nimbus-jose-jwt:9.4.2" | ||
|
||
implementation("io.micronaut.security:micronaut-security-jwt:$micronautSecurityVersion") { | ||
exclude group: 'io.micronaut', module: 'micronaut-http' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to exclude the http
dependencies since they are not used and might not be necessary on the classpath of consumers.
if (!jwtOptional.isPresent()) { | ||
final String message = "JWT validation failed"; | ||
LOG.error(message); | ||
throw Status.PERMISSION_DENIED.withDescription(message).asRuntimeException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to also return Status.UNAUTHENTICATED
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this configurable, but we can still discuss the default.
LOG.error(message); | ||
throw Status.PERMISSION_DENIED.withDescription(message).asRuntimeException(); | ||
} | ||
return new ForwardingServerCallListener.SimpleForwardingServerCallListener<T>(listener) { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add any additional metadata to denote that the user is authenticated?
Any feedback on this @jameskleeh @ilopmar @graemerocher ? |
.../src/main/java/io/micronaut/grpc/server/security/jwt/GrpcServerSecurityJwtConfiguration.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/micronaut/grpc/server/security/jwt/GrpcServerSecurityJwtConfiguration.java
Outdated
Show resolved
Hide resolved
*/ | ||
@Override | ||
public <T, S> ServerCall.Listener<T> interceptCall(final ServerCall<T, S> call, final Metadata metadata, final ServerCallHandler<T, S> next) { | ||
final List<String> requiredAccesses = getRequiredAccesses(config, call); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not also using the SecurityRule API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SecurityRule
implies knowledge of HttpRequest
and RouteMatch
:
https://github.com/micronaut-projects/micronaut-security/blob/master/security/src/main/java/io/micronaut/security/rules/SecurityRule.java#L57
I don't have either of those in this context.
I was attempting to use the @Secured
annotation, but failed since I didn't have RouteMatch
accessible...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SecurityRule
API is also using ANT
style patch matching, which wouldn't sync up with the GRPC method names. I'm using regex in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianwyka OK makes sense. In that case I think a new API to support the same use cases should be created. It should in general work the same as the existing logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'll take a look into that. @jameskleeh should that new API live in this repository or in micronaut-security
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I was thinking, @jameskleeh maybe there should be some MethodSecurityRule
interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to get access to the potential @Secured
annotation on the method as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can model after the SecurityFilter
approach. Create one high-level GrpcServerSecurityInterceptor
and have all the method rules collected there and interrogated. Sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graemerocher , @jameskleeh, I'm realizing there will be a fair amount of duplication due to coupling with HttpRequest
unless there is some level of abstraction in micronaut-security
, which would require breaking changes and a 3.0
release of that module.
Here is what I am thinking (demonstration of subset of changes):
https://github.com/micronaut-projects/micronaut-security/compare/master...brianwyka:security-abstraction?expand=1
Obviously, this does not encompass all changes, but just parameterizing some of the main interfaces and abstract classes can add a ton of reusability to this module. It may be even useful to have micronaut-security
and micronaut-security-http
separation...
Let me know if you would like me to move forward with a lot of duplication here or begin working on some abstractions and breaking changes in the security module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to do some of the duplication in this repository... Here is the start of it: 7c676d0. I bypassed creating the generic token stuff and focused on just JWT for now. Thoughts?
@brianwyka Any updates on this? |
@StephanePaulus, waiting for feedback on design direction from Micronaut team. After that, happy to move forward with changes. @jameskleeh, @graemerocher let me know if you have any recommendations based on my latest comments in this thread... |
@brianwyka I haven't forgot about this. I'll try to make it a priority next week |
Thanks @jameskleeh. Let me know what you need from me. Happy to do the implementation as much as possible. |
@graemerocher @sdelamo @brianwyka This looks very useful feature, I have an interest in it for one of my projects. I think it can be nice to have it in the next major release. If needs any help with this PR, I can help to finish all work here. |
@sdelamo maybe it's better for me to help you with decoupling first, as I'm using |
|
1 similar comment
|
@donbeave |
Main Changes
grpc-server-security-jwt
module@Secured
annotation at this pointI've added a
serverSecurity.adoc
to document its usage.Other Changes
Closes #164