-
Notifications
You must be signed in to change notification settings - Fork 829
JAV-501 Public key authentication between consumer and provider #330
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
Conversation
fix bug and add sample
2. bug gix 3. 增加UT
2. fix ut
# Conflicts: # samples/pom.xml
|
Changes Unknown when pulling cf76cbe on coolhongluo:serviceauth_rsatoken into ** on ServiceComb:master**. |
| kf = KeyFactory.getInstance(RSA_ALG); | ||
| }catch(NoSuchAlgorithmException e) | ||
| { | ||
| LOGGER.error("init keyfactory error"); |
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.
Code not well formated
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.
new code ,forget to format
| public static String sign(String content, PrivateKey privateKey) | ||
| throws NoSuchAlgorithmException, InvalidKeySpecException, SignatureException, InvalidKeyException { | ||
| Signature signature = Signature.getInstance(SIGN_ALG); | ||
| signature.initSign(privateKey); |
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.
Public and private key may not change in every call, for performance we can cache the init values. It is fine now and need more test data to evaluate
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.
this is a good idea,but i think the RSAUtils is a common function;
if have performance problem ,i will refactor it
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (null == obj || !(obj instanceof RSAAuthenticationToken)) { |
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.
redundant expression
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.
accept
| public void handle(Invocation invocation, AsyncResponse asyncResp) throws Exception { | ||
|
|
||
| Optional<String> token = Optional.ofNullable(athenticationTokenManager.getToken()); | ||
| if(!token.isPresent()) |
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, we can delete all these instructions. 38-44
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.
maybe,but i think can reduce send request to server if do it
| } catch (InvalidKeyException | NoSuchAlgorithmException | InvalidKeySpecException | SignatureException e) { | ||
| logger.error("create token error", e); | ||
| throw new Error("create token error"); | ||
| } |
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.
lock is not properly released in some situation. I think we do not need lock at all at this part. e.g. we just add a one time task to do the creation of the key. And we need to do some protection when service center service is not available at the creation moment, that's to say, we need use the old key & new key for 15 minutes.
| } | ||
|
|
||
| private String getPublicKey(String instanceId, String serviceId) { | ||
| Optional<MicroserviceInstance> instances = Optional |
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.
This is just a null check, why need using Optional?
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.
accept
| private static Cache<String, MicroserviceInstance> instances = CacheBuilder.newBuilder().maximumSize(1000) | ||
| .expireAfterAccess(30, TimeUnit.MINUTES).build(); | ||
|
|
||
| public static MicroserviceInstance getOrCreate(String serviceId, String instanceId) { |
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.
do you use the code template under $HOME/etc folder?
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.
accept ; forget to format
| @@ -0,0 +1,32 @@ | |||
| package io.servicecomb.foundation.common.utils; | |||
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.
copy right
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.
accept
coolhongluo
left a comment
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.
maybe need double check here
As a developer, I need to have authentication in service provider to protect the microservice from unauthorized access