-
Notifications
You must be signed in to change notification settings - Fork 282
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
fetch verification key from server during access token validation #2323
fetch verification key from server during access token validation #2323
Conversation
Signed-off-by: Toshiaki Aoike <taoike@yahoo-corp.jp>
@@ -45,6 +45,9 @@ public class JwtsSigningKeyResolver implements SigningKeyResolver { | |||
private static final ObjectMapper JSON_MAPPER = initJsonMapper(); | |||
private final SSLContext sslContext; | |||
private final String jwksUri; | |||
private final boolean skipConfig; | |||
private static long lastZtsJwkFetchTime; | |||
private static long millisBetweenZtsCalls; |
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.
what is the default value for this setting? this change would require every user of the library to set the value otherwise with unknown key ids the library will be making a call to the server every time. we can probably start with a conserve once a day value.
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 default value is set to 30 minutes. This setting can be modified in the property file.
setMillisBetweenZtsCalls(Long.parseLong(System.getProperty(ZPE_PROP_MILLIS_BETWEEN_ZTS_CALLS, Long.toString(30 * 1000 * 60)))); |
In order to avoid increasing the server load by sending requests to ZTS every time an unknown key ID is specified, we have set it to make a request every 30 minutes.
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.
you're incorrectly assuming that it's the only place that the library is used. it's not a server library. it's a common client shared library so there are other applications that are using that functionality.
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 apologize for the mistake. There was an error in the comment. It's not a property file, but a system property.
When need to modify the settings while using the client library, can make use of system properties.
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.
Doesn't matter if it can be setup using a system property. We cannot make changes in shared client libraries where we change its current behavior and technically could result in DOS attacks. You must set the default value for the millisBetweenZtsCalls field to one day and then you can change that value where it's needed (e.g. with zpu)
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.
Thank you for your suggestion.
I have made the necessary modifications to initialize millisBetweenZtsCalls in JwtsSigningKeyResolver.java.
@@ -45,6 +45,9 @@ public class JwtsSigningKeyResolver implements SigningKeyResolver { | |||
private static final ObjectMapper JSON_MAPPER = initJsonMapper(); | |||
private final SSLContext sslContext; | |||
private final String jwksUri; | |||
private final boolean skipConfig; |
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.
any reason why we're not storing this field if we're not using it anywhere else?
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.
Thank you, as you mentioned, since we're no longer using this field from anywhere else, we decided to remove it.
Signed-off-by: Toshiaki Aoike <taoike@yahoo-corp.jp>
… (1 day). Signed-off-by: Toshiaki Aoike <taoike@lycorp.co.jp>
By merging this PR, the main Screwdriver job has stopped. This issue is caused by insufficient coverage in athenz-zpe-java-client.
The problem occurs intermittently. The jobs that reacted to the PR have been successful, as shown below. |
interesting since the last PR build shows that it passed successfully. I'm somewhat surprised about the intermittent comment unless there is some bug in the jococo plugin when calculating the percentages. |
description
Currently, AuthZpeClient is limited to dynamically fetching public keys exclusively during RoleToken verifications.
We aim to expand this functionality to also dynamically retrieve public keys when verifying AccessToken.