You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2022/08/24 17:11:27 UTC
[GitHub] [knox] MrtnBalazs opened a new pull request, #624: KNOX-2790 - Added a new funtion to verifier, this way the session lim…
MrtnBalazs opened a new pull request, #624:
URL: https://github.com/apache/knox/pull/624
## What changes were proposed in this pull request?
Changed the verifier, introduced a new function called `registerToken`, this function is checking the session limits for the given user and stores the given token. Changed `verifySessionForUser` function, now this function only checks the limit but do not store token. These changes were needed, because previously in case of an attack we would generate tokens needlessly before verifying the session and checking the limit. Now we check the limit without a token before token generation, so in case of an attack we do not waste resources. And we add the token after token generation and also check the limit because thread safety requires it.
## How was this patch tested?
I have changed the unit tests in `InMemoryConcurrentSessionVerifierTest` and `WebSSOResourceTest` to test the new usage of the verifier.
I also tested it manually with this configuration:
```
<property>
<name>gateway.session.verification.unlimited.users</name>
<value>admin</value>
</property>
<property>
<name>gateway.session.verification.privileged.users</name>
<value>tom</value>
</property>
<property>
<name>gateway.session.verification.privileged.user.limit</name>
<value>2</value>
</property>
<property>
<name>gateway.session.verification.non.privileged.user.limit</name>
<value>1</value>
</property>
<property>
<name>gateway.session.verification.expired.tokens.cleaning.period</name>
<value>80</value>
</property>
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value>org.apache.knox.gateway.session.control.InMemoryConcurrentSessionVerifier</value>
</property>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] MrtnBalazs commented on pull request #624: KNOX-2790 - Added a new funtion to verifier, this way the session lim…
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on PR #624:
URL: https://github.com/apache/knox/pull/624#issuecomment-1226840414
@smolnar82 @zeroflag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #624: KNOX-2790 - Added a new funtion to verifier, this way the session lim…
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #624:
URL: https://github.com/apache/knox/pull/624#discussion_r954590973
##########
gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java:
##########
@@ -264,9 +264,14 @@ private Response getAuthenticationToken(int statusCode) {
original = originalUrlCookies.get(0).getValue();
}
+ Principal p = request.getUserPrincipal();
+ ConcurrentSessionVerifier verifier = services.getService(ServiceType.CONCURRENT_SESSION_VERIFIER);
+ if (!verifier.verifySessionForUser(p.getName())) {
Review Comment:
oh...my bad. These are not the same lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #624: KNOX-2790 - Added a new funtion to verifier, this way the session lim…
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #624:
URL: https://github.com/apache/knox/pull/624#discussion_r954564130
##########
gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java:
##########
@@ -264,9 +264,14 @@ private Response getAuthenticationToken(int statusCode) {
original = originalUrlCookies.get(0).getValue();
}
+ Principal p = request.getUserPrincipal();
+ ConcurrentSessionVerifier verifier = services.getService(ServiceType.CONCURRENT_SESSION_VERIFIER);
+ if (!verifier.verifySessionForUser(p.getName())) {
Review Comment:
Now we have this same 3-line code block in two places; I'd create a new private void method that throws the web app exception if needed (to avoid code duplication).
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -51,10 +51,16 @@ public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerif
private long cleaningPeriod;
private final ScheduledExecutorService expiredTokenRemover = Executors.newSingleThreadScheduledExecutor();
+ /**
+ * This function is used after the verifySessionForUser function.
+ * It checks the session limits even though verifySessionForUser already checked them,
+ * because in high stress situations there might be some threads which get verified even though they are over the limit in
+ * verifySessionForUser function, so we catch them here.
+ */
@Override
- public boolean verifySessionForUser(String username, JWT jwtToken) {
+ public boolean registerToken(String username, JWT jwtToken) {
Review Comment:
This could be simplified like that:
```
if (verifySessionForUser(userName)) {
//lock
concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
//unlock
return true;
}
return false;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 merged pull request #624: KNOX-2790 - Added a new funtion to verifier, this way the session lim…
Posted by GitBox <gi...@apache.org>.
smolnar82 merged PR #624:
URL: https://github.com/apache/knox/pull/624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org