Fix redirect loop when oic credentials have expired but jenkins session is still valid #357

mikecirioli · 2024-07-23T14:10:58Z

to reproduce:

configure a jenkins instance to auth using the plugin, do not enable refresh tokens
assume your oic crentials lifetime is 120s (at least that is the case for my test provider)
assume default skew allowance of 60s
login to your jenkins instance
notice that after only 60s, your OIC credentials have now expired!
any subsequent attempt to access the instance results in a redirect loop between the provider and jenkins

Although the proposed PR fixes the issue AFAICT, I am not 100% confident it is the correct fix. This issue first appears in release 4.297.vcddb_d8a_e4694, but i have not been able to identify exactly what i believe changed to cause this behavior. My theory is that because the jenkins web session is still valid, it allows the request, which is then flagged as invalid because the OIC credentials have expired, kicking the whole loop off again. Hopefully you can confirm or deny this @krezovic

I also believe that the clock skew was mistakenly being subtracted from the credentials "expires in X seconds" calculation - my understanding was the clock skew should add an additional "buffer" to the lifetime of the credentials in order to accommodate slight variances in different clocks.

Testing done

Lots of manual testing....
I've also fixed 2 tests that i believe cover the changes made in this PR:

testRefreshTokenAndTokenExpiration_withoutRefreshToken()
The test was originally using JenkinsRule.WebClient to check for a 302 respoonse when the OicCredentials are expired. Normally WebClient would follow redirects, and the test was only passing with the expected Blocked - 302 due to the redirect loop that was occurring. Fixing that caused this test to fail, so i've replaced that bit of test code with a java HttpClient set to not follow redirects.
the expire helper method was using the wrong number of ms to force the credentials to be expired. This apparently worked previously because the clock skew was actually being subtracted from the credential lifetime (instead of augmenting it). Since all we care about is ensuring the credentials appear expired I simply set the creation instant to 1 (ie. the first millisecond of time ever).

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

…redirect loops

mikecirioli · 2024-07-23T16:03:20Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

@@ -1420,7 +1419,8 @@ public boolean handleTokenExpiration(HttpServletRequest httpRequest, HttpServlet
    private void redirectOrRejectRequest(HttpServletRequest req, HttpServletResponse res)
            throws IOException, ServletException {
        if (req.getSession(false) != null || Strings.isNullOrEmpty(req.getHeader("Authorization"))) {
-            WebApp.get(Jenkins.get().servletContext).getSomeStapler().invoke(req, res, Jenkins.get(), getLoginUrl());


@krezovic i was not familiar with this - was it essentially meant to perform a redirect?

@mikecirioli I believe I was addressing feedback from the review where it was noted that it would be user-friendly to redirect user back to login when the token expires. Original implementation contained 401 only.

codecov · 2024-07-23T19:15:09Z

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.44%. Comparing base (b7205f1) to head (ad6b064).
Report is 3 commits behind head on master.

Files	Patch %	Lines
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #357      +/-   ##
============================================
- Coverage     72.02%   71.44%   -0.58%     
+ Complexity      234      232       -2     
============================================
  Files            11       11              
  Lines           990      991       +1     
  Branches        142      142              
============================================
- Hits            713      708       -5     
- Misses          199      205       +6     
  Partials         78       78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amuniz · 2024-07-24T09:49:25Z

src/test/java/org/jenkinsci/plugins/oic/PluginTest.java

-        webClient.assertFails(jenkins.getSecurityRealm().getLoginUrl(), 302);
-
+        // use an actual HttpClient to make checking redirects easier
+        HttpResponse<String> rsp = getPageWithGet("/manage");


Is JenkinsRule#createWebClient not enough for this?

not the way the test was originally written - AFAICT JenkinsRule.WebClient follows http redirects, which was causing the test to fail. The test was passing before because JenkinsRule.WebClient was erroring out due to the redirect loop. I used the HttpClient because you can tell it not to follow redirects.

@amuniz just to be certain, i took a look at the original test and confirmed that it does indeed suffer from the same redirect loop

michael-doubez

LGTM

mikecirioli · 2024-07-26T12:42:37Z

@michael-doubez i realize that i did not add a label to this PR before it was merged, and i don't have permissions to trigger a release - is that something you can do?

michael-doubez · 2024-07-26T13:19:01Z

@mikecirioli my mistake - it didn't check

mprausa · 2024-07-29T14:47:50Z

src/main/java/org/jenkinsci/plugins/oic/OicCredentials.java

@@ -48,7 +48,7 @@ public OicCredentials(

        if (expiresInSeconds != null && expiresInSeconds > 0) {
            long allowedClockSkewFixed = Util.fixNull(allowedClockSkewSeconds, 60L);
-            this.expiresAtMillis = currentTimestamp + (expiresInSeconds - allowedClockSkewFixed) * 1000;
+            this.expiresAtMillis = (currentTimestamp + expiresInSeconds + allowedClockSkewFixed) * 1000;


This change seems to be incorrect. currentTimestamp is already in millis. Therefore, this line should be this.expiresAtMillis = currentTimestamp + (expiresInSeconds + allowedClockSkewFixed) * 1000.

mikecirioli added 2 commits July 23, 2024 09:53

clock skew should add extra buffer time, not remove it

63bd8e1

id oicSession has expired, also expire jenkins websession to prevent …

104fcbe

…redirect loops

mikecirioli requested a review from a team as a code owner July 23, 2024 14:10

oops

186abf5

mikecirioli marked this pull request as draft July 23, 2024 14:20

fix test

5f24f40

mikecirioli commented Jul 23, 2024

View reviewed changes

fix broken test

ad6b064

mikecirioli marked this pull request as ready for review July 23, 2024 19:05

amuniz reviewed Jul 24, 2024

View reviewed changes

slabre-conecteo mentioned this pull request Jul 25, 2024

API Token expired too quickly since 4.299.v5ca_eb_6a_f3e6d #358

Closed

amuniz approved these changes Jul 26, 2024

View reviewed changes

michael-doubez approved these changes Jul 26, 2024

View reviewed changes

michael-doubez merged commit bba32e0 into jenkinsci:master Jul 26, 2024
16 of 18 checks passed

michael-doubez assigned mikecirioli Jul 26, 2024

michael-doubez added the bug label Jul 26, 2024

mprausa reviewed Jul 29, 2024

View reviewed changes

mprausa mentioned this pull request Jul 30, 2024

Fix calculation of expiration timestamp #359

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix redirect loop when oic credentials have expired but jenkins session is still valid #357

Fix redirect loop when oic credentials have expired but jenkins session is still valid #357

mikecirioli commented Jul 23, 2024 •

edited

Loading

mikecirioli Jul 23, 2024

krezovic Jul 23, 2024

codecov bot commented Jul 23, 2024

amuniz Jul 24, 2024

mikecirioli Jul 25, 2024

mikecirioli Jul 25, 2024

michael-doubez left a comment

mikecirioli commented Jul 26, 2024

michael-doubez commented Jul 26, 2024

mprausa Jul 29, 2024

Fix redirect loop when oic credentials have expired but jenkins session is still valid #357

Fix redirect loop when oic credentials have expired but jenkins session is still valid #357

Conversation

mikecirioli commented Jul 23, 2024 • edited Loading

Testing done

Submitter checklist

mikecirioli Jul 23, 2024

Choose a reason for hiding this comment

krezovic Jul 23, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2024

Codecov Report

amuniz Jul 24, 2024

Choose a reason for hiding this comment

mikecirioli Jul 25, 2024

Choose a reason for hiding this comment

mikecirioli Jul 25, 2024

Choose a reason for hiding this comment

michael-doubez left a comment

Choose a reason for hiding this comment

mikecirioli commented Jul 26, 2024

michael-doubez commented Jul 26, 2024

mprausa Jul 29, 2024

Choose a reason for hiding this comment

mikecirioli commented Jul 23, 2024 •

edited

Loading