Replace EOL Google Oauth library #409

jtnord · 2024-10-02T14:12:59Z

This changes the Google OAuth library which is in maintenance mode with a supported library (nimbusds via pac4j)

Warning
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 specification. As such users must first update to 4.355.v3a_fb_fca_b_96d4 to set the Issuer before updating to this version.
Additionally this removes the option to send the scopes when requesting the access token. users of non conformant OPs that require this functionality should remain on the previous version until the provider fixes their implementation.

The change no longer determines how often to refresh the well-known settings (if used), they are now refreshed every hour.

fixes: #313

Testing done

mvn hpi:run (java11) against a local AD FS server (login, logout) (disable TLS verification) (well known config)
mvn hpi:run (java11) against a local AD FS server (login, logout) (trusted TLS cert) (well known config)
java -jar jenkins-2.479.war --httpListenAddress=127.0.0.1 --prefix=/jenkins (java17) against a local AD FS server (login, logout) (disable TLS verification) (well known config)
JENKINS_VERSION=2.462.3 LOCAL_JARS=/path/to/oic-auth-plugin/target/oic-auth.hpi mvn -Dtest=OicAuthPluginTest
JENKINS_VERSION=2.479 LOCAL_JARS=/path/to/oic-auth-plugin/target/oic-auth.hpi mvn test -Dtest=OicAuthPluginTest
mvn hpi:run (java11) against an AWS Cognito server (login, logout) (well known config)
mvn hpi:run (java11) against an AWS Cognito server (login, logout) (manual config)
oidcc-client-basic-certification-test-plan passed

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

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

jtnord · 2024-10-02T17:24:00Z

src/main/java/org/jenkinsci/plugins/oic/ssl/AnythingGoesTrustManager.java

+import jenkins.security.FIPS140;
+
+@SuppressFBWarnings(value = "WEAK_TRUST_MANAGER", justification = "Opt in by user")
+final class AnythingGoesTrustManager implements X509TrustManager {


there are enough of these in all plugins this should really be in Jenkins core.

jtnord · 2024-10-02T17:24:38Z

src/main/java/org/jenkinsci/plugins/oic/ssl/IgnoringHostNameVerifier.java

+ * {@link HostnameVerifier} that accepts any presented hostanme including incorrect ones.
+ */
+@Restricted(NoExternalUse.class)
+public final class IgnoringHostNameVerifier implements HostnameVerifier {


Again, enough plugins use something like this that it should be offered from Jenkins core.

jtnord · 2024-10-02T17:27:40Z

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

                    + "\"token_endpoint\":\"http://localhost:%1$d/token\","
                    + "\"userinfo_endpoint\":\"http://localhost:%1$d/user\","
                    + "\"jwks_uri\":\"http://localhost:%1$d/jwks\","
-                    + "\"scopes_supported\": null,"


the OpenID spec says if something is empty is it ommitted completely and does not contain "null"
may as well say we support the default.

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

-                    }
-
-                    flow.createAndStoreCredential(response, null);
+    public void doCommenceLogin(@QueryParameter String from, @Header("Referer") final String referer)


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

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

src/main/java/org/jenkinsci/plugins/oic/ssl/TLSUtils.java

jtnord · 2024-10-03T13:42:37Z

src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.jelly

@@ -1,6 +1,11 @@
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">

+
+    <f:entry title="${%Issuer}" field="issuer">


moved up as it is now mandatory to make it more prominent vs the other fields.

jtnord · 2024-10-03T15:32:37Z

pom.xml

    <jenkins.version>2.426.3</jenkins.version>
    <spotless.check.skip>false</spotless.check.skip>
    <spotbugs.effort>Max</spotbugs.effort>
    <configuration-as-code.version>1836.vccda_4a_122a_a_e</configuration-as-code.version>
-    <hpi.compatibleSinceVersion>4.347</hpi.compatibleSinceVersion>
+    <hpi.compatibleSinceVersion>4.356</hpi.compatibleSinceVersion>


needs to be checked and updated before final merge.

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

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

jtnord · 2024-10-04T11:45:29Z

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

+    }
+
+    private OidcConfiguration buildOidcConfiguration() {
+        // TODO cache this and use the well known if available.


for a future PR. We can cache the client and invalidate it every 1 hour if using the well-known (as the well known refresh is now hard coded to 1hr).

jtnord · 2024-10-04T11:46:48Z

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

+        // TODO what do we prefer?
+        // conf.setPreferredJwsAlgorithm(JWSAlgorithm.HS256);
+        // set many more as needed...


not intending for this to happen in this PR but a follow-up that exposes some more advanced options.
e.g. #408

codecov · 2024-10-04T11:46:59Z

Codecov Report

Attention: Patch coverage is 68.48875% with 98 lines in your changes missing coverage. Please review.

Project coverage is 71.80%. Comparing base (b48a299) to head (d5e1522).
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	50.00%	18 Missing and 6 partials ⚠️
...nsci/plugins/oic/OicServerManualConfiguration.java	76.47%	5 Missing and 3 partials ⚠️
...kinsci/plugins/oic/AnythingGoesTokenValidator.java	66.66%	7 Missing ⚠️
...jenkinsci/plugins/oic/CustomOidcConfiguration.java	65.00%	4 Missing and 3 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     #409      +/-   ##
============================================
- Coverage     74.16%   71.80%   -2.37%     
+ Complexity      261      199      -62     
============================================
  Files            15       16       +1     
  Lines          1022      883     -139     
  Branches        147      120      -27     
============================================
- Hits            758      634     -124     
+ Misses          191      185       -6     
+ Partials         73       64       -9

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

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

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

jtnord · 2024-10-07T13:12:22Z

Managed to get a Cognito server setup (FFR. you can use http if the URL is for localhost)
The code works if using well-known discovery, but not if using manual config, and I have tracked down one simple error (2941d99) and it is working for me now (tested simple login and logout)

refresh tokens do not work with cognito as the return from the token refresh is missing the refresh token so it gets overwritten with null here and then the profile blows up.

This was fixed in 6.0.4.1 but needs java 17 which means we need to bump the baseline...

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.

jtnord · 2024-10-08T10:59:11Z

fixing the NPE, shows there is another more serious issue here, the token validator is used to cehck the id token when refreshing (actually creating a new!) Profile.
The id token may not contain a nonce (in fact "SHOULD NOT") which is the case in AWS Cognito. However the library (infact the token verifier) is expecting a nonce and is then failing that it does not exist.

FINE: Missing JWT nonce (nonce) claim
com.nimbusds.jwt.proc.BadJWTException: Missing JWT nonce (nonce) claim
        at com.nimbusds.openid.connect.sdk.validators.BadJWTExceptions.<clinit>(BadJWTExceptions.java:70)
        at com.nimbusds.openid.connect.sdk.validators.IDTokenClaimsVerifier.verify(IDTokenClaimsVerifier.java:248)
        at com.nimbusds.jwt.proc.DefaultJWTProcessor.verifyClaims(DefaultJWTProcessor.java:271)
        at com.nimbusds.jwt.proc.DefaultJWTProcessor.process(DefaultJWTProcessor.java:373)
        at com.nimbusds.openid.connect.sdk.validators.IDTokenValidator.validate(IDTokenValidator.java:321)
        at com.nimbusds.openid.connect.sdk.validators.IDTokenValidator.validate(IDTokenValidator.java:254)
        at org.pac4j.oidc.profile.creator.TokenValidator.validate(TokenValidator.java:108)
        at org.pac4j.oidc.profile.creator.OidcProfileCreator.create(OidcProfileCreator.java:108)
        at org.pac4j.core.client.BaseClient.retrieveUserProfile(BaseClient.java:126)
        at org.pac4j.core.client.BaseClient.getUserProfile(BaseClient.java:105)
        at org.pac4j.oidc.client.OidcClient.renewUserProfile(OidcClient.java:69)
        at org.jenkinsci.plugins.oic.OicSecurityRealm.refreshExpiredToken(OicSecurityRealm.java:1220)

This appears to be a bug with pac4j. We ae not using the latest version as that requires java17 which is not required for Jenkins prior to 2.463 (which will mean the upcoming LTS version of 2.479).
Whilst I have not found any indication that pac4j 6.x fixes this, it should be tested as this is the active (comminuity) supported line.

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.

jtnord · 2024-10-08T14:42:32Z

This appears to be a bug with pac4j. We ae not using the latest version as that requires java17 which is not required for Jenkins prior to 2.463 (which will mean the upcoming LTS version of 2.479). Whilst I have not found any indication that pac4j 6.x fixes this, it should be tested as this is the active (comminuity) supported line.

workaround added in fb4ce0c

@fcojfernandez is attempting to reproduce this with pac4j v6 to either upgrade or file a report upstream

For extra security disable the "none" algorithm if the server claims to support it. Whilst we are using code flow, it is not needed, but providers MUST support RS256 so there would always be a more secure option we can use.

jtnord · 2024-10-08T15:55:59Z

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

            }
-
-            setWellKnownExpires(response.getHeaders());
+            // do not allow the "none" singing algorithm for security


Whilst we are using code flow, it is not needed, but providers MUST support RS256 so there would always be a more secure option we can use.

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

fcojfernandez · 2024-10-09T11:42:45Z

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

+    public OIDCProviderMetadata toProviderMetadata() {
+        // we perform this download manually rather than letting pac4j perform it
+        // so that we can cache and expire the result.
+        // pac4j will cache the result yet never expire it.


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

jglick · 2024-10-09T12:09:21Z

We [are] not using the latest version as that requires java17 which is not required for Jenkins prior to 2.463 (which will mean the upcoming LTS version of 2.479).

I do not think you should hesitate to switch the plugin baseline to 2.479, especially for a major change like this one.

PereBueno · 2024-10-09T11:01:05Z

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

+    CustomOidcConfiguration(boolean disableTLS) {
+        this.disableTLS = disableTLS;
+        if (FIPS140.useCompliantAlgorithms() && disableTLS) {
+            throw new IllegalStateException("Cannot disable TLS validation in FIPS-140 mode");


For my own learning.
IllegalState instead of IllegalArgument because this is only used during login, so it means configuration was already wrong (and should have been raised when creating the realm once jtnord#1 is merged) isn't it?

and should have been raised when creating the realm once jtnord#1 is merged

correct

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

Co-authored-by: Pere <pbuenoyerbes@cloudbees.com>

fcojfernandez · 2024-10-09T14:47:21Z

We [are] not using the latest version as that requires java17 which is not required for Jenkins prior to 2.463 (which will mean the upcoming LTS version of 2.479).

I do not think you should hesitate to switch the plugin baseline to 2.479, especially for a major change like this one.

@jglick I'm doing it as part of #409 (comment)

jtnord · 2024-10-09T15:51:49Z

@michael-doubez would you like to take a look at this before I merge?

jtnord mentioned this pull request Oct 2, 2024

Unexpected signing algorithm PS512 after the last updates #408

Open

jtnord commented Oct 2, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java Outdated Show resolved Hide resolved

jtnord commented Oct 2, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java Outdated Show resolved Hide resolved

jtnord commented Oct 2, 2024

View reviewed changes

jtnord force-pushed the pac4j branch from 6e17311 to 3b0103a Compare October 3, 2024 12:41

jtnord added documentation breaking and removed documentation labels Oct 3, 2024

github-advanced-security bot found potential problems Oct 3, 2024

View reviewed changes

jtnord force-pushed the pac4j branch from 3b0103a to d7120c5 Compare October 3, 2024 13:41

jtnord commented Oct 3, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java Outdated Show resolved Hide resolved

jtnord force-pushed the pac4j branch 5 times, most recently from a8f5665 to 3fec21b Compare October 4, 2024 11:19

jtnord mentioned this pull request Oct 4, 2024

Log WWW-Authenticate header on errors if present #318

Open

jtnord force-pushed the pac4j branch from 3fec21b to b6041de Compare October 4, 2024 11:31

github-advanced-security bot found potential problems Oct 4, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Dismissed Show dismissed Hide dismissed

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Dismissed Show dismissed Hide dismissed

jtnord commented Oct 4, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Show resolved Hide resolved

jtnord commented Oct 4, 2024

View reviewed changes

jtnord force-pushed the pac4j branch from b6041de to 60fc090 Compare October 4, 2024 12:00

jtnord commented Oct 4, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Show resolved Hide resolved

jtnord added 3 commits October 7, 2024 14:49

the JWKSetURL set in the provider config was not set in manual mode.

2941d99

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.

update help for sendScopesInTokenRequest to show this is not supported.

227d072

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.

remove unused field

5b51704

jtnord mentioned this pull request Oct 7, 2024

Redirect to login page in case token is expired instead of showing an error page #395

Merged

6 tasks

jtnord added 4 commits October 8, 2024 12:03

Prevent NPE if GrantTypes is empty for refresh

7b753d6

Merge remote-tracking branch 'origin/master' into pac4j

659ecfc

IDToken and refresh token may not be present when refreshing

0ebc81b

jtnord mentioned this pull request Oct 8, 2024

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

Closed

6 tasks

jtnord added 2 commits October 8, 2024 15:53

update to pac4j 5.7.7 to pick up security fix

a594a27

Do not allow the "none" signing algorithm

7116a7b

For extra security disable the "none" algorithm if the server claims to support it. Whilst we are using code flow, it is not needed, but providers MUST support RS256 so there would always be a more secure option we can use.

jtnord commented Oct 8, 2024

View reviewed changes

update compatable since

f8230ef

jtnord marked this pull request as ready for review October 8, 2024 16:05

fcojfernandez reviewed Oct 9, 2024

View reviewed changes

PereBueno reviewed Oct 9, 2024

View reviewed changes

FormValidation should be an error as value is required

769f395

Co-authored-by: Pere <pbuenoyerbes@cloudbees.com>

PereBueno approved these changes Oct 9, 2024

View reviewed changes

fcojfernandez previously approved these changes Oct 9, 2024

View reviewed changes

fix empty strings

d5e1522

jtnord dismissed fcojfernandez’s stale review via d5e1522 October 9, 2024 15:46

fcojfernandez approved these changes Oct 9, 2024

View reviewed changes

jtnord mentioned this pull request Oct 9, 2024

HTTP ERROR 401 Unauthorized with Azure App Registration #415

Open

olamy approved these changes Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace EOL Google Oauth library #409

Replace EOL Google Oauth library #409

jtnord commented Oct 2, 2024 •

edited

Loading

jtnord Oct 2, 2024

jtnord Oct 2, 2024

jtnord Oct 2, 2024

jtnord Oct 3, 2024 •

edited

Loading

jtnord Oct 3, 2024

jtnord Oct 4, 2024

jtnord Oct 4, 2024

codecov bot commented Oct 4, 2024 •

edited

Loading

jtnord commented Oct 7, 2024 •

edited

Loading

jtnord commented Oct 8, 2024

jtnord commented Oct 8, 2024

jtnord Oct 8, 2024

fcojfernandez Oct 9, 2024

jglick commented Oct 9, 2024

PereBueno Oct 9, 2024

jtnord Oct 9, 2024 •

edited

Loading

fcojfernandez commented Oct 9, 2024

jtnord commented Oct 9, 2024

Replace EOL Google Oauth library #409

Are you sure you want to change the base?

Replace EOL Google Oauth library #409

Conversation

jtnord commented Oct 2, 2024 • edited Loading

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

jtnord commented Oct 7, 2024 • edited Loading

jtnord commented Oct 8, 2024

jtnord commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Oct 9, 2024

Choose a reason for hiding this comment

jtnord Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

fcojfernandez commented Oct 9, 2024

jtnord commented Oct 9, 2024

jtnord commented Oct 2, 2024 •

edited

Loading

jtnord Oct 3, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

jtnord commented Oct 7, 2024 •

edited

Loading

jtnord Oct 9, 2024 •

edited

Loading