-
Notifications
You must be signed in to change notification settings - Fork 60
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
Cluster Api security #125
Cluster Api security #125
Conversation
|
||
ServerConfigProperties props = new ServerConfigProperties(); | ||
props.setKey(key); | ||
if (key.contains("password") || key.contains("license")) { | ||
if (key.contains("password") |
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.
If I'm not mistaken String.contains is case sensitive. Depending on what is returned for "key", should that be considered 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.
@reethi-kotti-aiven Yes, but it may not be considered here. All the keys are already lower case now.
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.
Btw, thank you for the review.
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.
LGTM from my side
I left a minor comment
@muralibasani super curious, since you are using Spring Boot, why not to onboard Spring Security for JWT (https://docs.spring.io/spring-security/reference/servlet/oauth2/resource-server/jwt.html)? |
@reta this introduces another component 'Authorization/Resource server' to issue tokens. In this PR, trying to keep it simple. |
About this change - What it does
Connects to Klaw ClusterApi with a jwt token
getServerConfig security issue fixed. normal users should not get any server configuration
Actuator endpoints to show fewer than wildcard
Resolves:
Why this way : token based authentication is better. getServerconfig restriction based on role.