CRL checking to optionally try all URLs #992
Conversation
|
bump |
|
Heads up. Merging by Wednesday morning, unless there are comments. |
|
Bump |
|
@leleuj have u had a chance to review this one? I'll merge with master of course. |
| } | ||
| } | ||
|
|
||
| if (revokedCrls.size() == crls.size() && !revokedCrls.isEmpty()) { |
There was a problem hiding this comment.
Just nitpicking on this: if we are here, the crls.size() does not equal the expiredCrls.size() so after removing all expired CRL (crls.removeAll(expiredCrls);), the crls cannot be empty, so again, if you want the revokedCrls list to be the same size as the crls list, you don't need the !revokedCrls.isEmpty() check.
There was a problem hiding this comment.
Good point. Let me rework the conditions and I'll add a few more test cases to cover all cases.
There was a problem hiding this comment.
I had a 2nd look at this.
This check here:
(revokedCrls.size() == crls.size() && !revokedCrls.isEmpty())
This check here says: if all remaining crls (after checking and removing those that are expired) are considered revoked, then throw the exception and apply the policy. Otherwise, proceed. Because you may find that 5 crls are remaining, and only 2 of them have been revoked. So the 3 that remain are valid and we wouldn't need to apply the policy to those. The policy is only applied if everything is considered revoked; thus the check.
Does that make sense?
There was a problem hiding this comment.
I understand this check: revokedCrls.size() == crls.size(): if all crls are revoked, let's throw an exception. What I say if this condition is true, the revokedCrls being equals to the crls which cannot be empty, then the second check is not necessary.
There was a problem hiding this comment.
Let me propose this:
You have 5 CRLs initially. Once you go through the loop, you realize 5 are expired. So you remove the expired 5 from the original set. At this point:
crls.size = 0
and because of that:
revokedCrls.size = 0
So it turns out that revokedCrls.isEmpty() is needed. If you take this check out, a revoked certificate exception will be thrown when all CRLs are expired and removed, and that is not what you want.
There was a problem hiding this comment.
I think this is the last remaining issue here before we merge; let me know if that helps to clarify the logic here.
There was a problem hiding this comment.
If all crls are expired, you are trapped in the first if check (https://github.com/Jasig/cas/pull/992/files#diff-1cc175d11f2a81cb0773374cd6d0038cR91) and never goes through the else clause we are talking about.
There was a problem hiding this comment.
You are perfectly right! Removed the check. Thanks for bearing with me.
|
@leleuj I am assuming all feedback is accounted for? |
|
Yes, +1 |
CRL checking to optionally try all URLs
Closes #927
The present behavior stops at the first entry found. Optionally, this pull allows CAS to check all and only report back if all fail.