Skip to content
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

Replace EOL Google Oauth library, disable nonce check during token refresh #419

Closed

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Oct 8, 2024

This is #409 with 2 additional commits.

Testing done

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

jtnord added 9 commits October 4, 2024 13:00
This changes the Google OAuth library which is in maintainance mode with
a supported library (nimbusds via pac4j)

The library requires that the Issuer is set to enforce security and
there is no option to disable this requirement as it is mandated in the
specificiation.  As such users must first update to 4.355.v3a_fb_fca_b_96d4
to set the Issuer before updating to this version.

fixes: jenkinsci#313
The OidcAuthenticator was not using the resource retreiver to talk to
servers. As such when used against a server with a self signed
certificate and disableTLS checks was set it would still fail.

Whilst we could implement our own Authenticator, there may be other
places where we need to modify the HttpRequest.  Therefore  we just
create a custom configuration that will set the proxy and TLS options as
required.
The provider config did not contain the jsksServerUrl if it was present
in the manual configuration.  This caused signed tokens to be rejected
when in manual configuration mode.
The option is not removed here, so that it can staty in the config.
This will at least allow users to downgrade as the option would be
retained.
Pac4j setups the token validators, which during the refresh lifecycle
will attempt to check an ID tokens nonce.
However a provider should not set the nonce in the idtoken during a
refresh, and in this case the validator fails because the nonce is
missing from the token!

we disable the nonce check for the refresh call.  it can be optionally
re-enabled by setting the system property
org.jenkinsci.plugins.oic.OicSecurityRealm.checkNonceInRefreshFlow to
true.
this is not exposed as a config option in the UI as
1) providers should not be sending the nonce anyway
2) this should be a short lived workaround whilst the issue with the
   library is filed and fixed.
throw new FailedCheckOfTokenException(
maybeOpenIdLogoutEndpoint(response.getIdToken(), state, buildOauthCommenceLogin()));
}
public void doCommenceLogin(@QueryParameter String from, @Header("Referer") final String referer)

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If OicSecurityRealm#doCommenceLogin connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 69.30693% with 93 lines in your changes missing coverage. Please review.

Project coverage is 72.20%. Comparing base (b48a299) to head (fb4ce0c).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java 75.48% 26 Missing and 12 partials ⚠️
...i/plugins/oic/OicServerWellKnownConfiguration.java 53.48% 15 Missing and 5 partials ⚠️
...kinsci/plugins/oic/AnythingGoesTokenValidator.java 66.66% 7 Missing ⚠️
...jenkinsci/plugins/oic/CustomOidcConfiguration.java 65.00% 4 Missing and 3 partials ⚠️
...nsci/plugins/oic/OicServerManualConfiguration.java 77.41% 5 Missing and 2 partials ⚠️
...insci/plugins/oic/ProxyAwareResourceRetriever.java 70.58% 3 Missing and 2 partials ⚠️
...nsci/plugins/oic/ssl/AnythingGoesTrustManager.java 37.50% 4 Missing and 1 partial ⚠️
...n/java/org/jenkinsci/plugins/oic/ssl/TLSUtils.java 50.00% 2 Missing and 1 partial ⚠️
...nsci/plugins/oic/ssl/IgnoringHostNameVerifier.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #419      +/-   ##
============================================
- Coverage     74.16%   72.20%   -1.96%     
+ Complexity      261      199      -62     
============================================
  Files            15       16       +1     
  Lines          1022      878     -144     
  Branches        147      119      -28     
============================================
- Hits            758      634     -124     
+ Misses          191      182       -9     
+ Partials         73       62      -11     

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

@jtnord jtnord closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant