From 85b77e71752a7d8afa2b59334b5bc867b1be3d22 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 09:31:23 +0200 Subject: [PATCH 1/8] This is actually non-null --- .../java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java index d7cef9be..b067d178 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java @@ -41,7 +41,7 @@ public class EscapeHatch extends OidcProperty { public static final Pattern B_CRYPT_PATTERN = Pattern.compile("\\A\\$[^$]+\\$\\d+\\$[./0-9A-Za-z]{53}"); - @CheckForNull + @NonNull private final String username; @CheckForNull From ad2e6b129b9b23aa3fdac44b83a17f21c2b90202 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 09:31:57 +0200 Subject: [PATCH 2/8] No need for isApplicable, use a static factory method instead. --- .../org/jenkinsci/plugins/oic/OicSecurityRealm.java | 4 +--- .../plugins/oic/OidcPropertyDescriptor.java | 7 ------- .../oic/properties/DisableTokenVerification.java | 12 ++++++++---- .../plugins/oic/properties/EscapeHatch.java | 11 +++++++---- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index cc2be653..ed59c96a 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -1614,9 +1614,7 @@ public IdStrategy defaultGroupIdStrategy() { @SuppressWarnings("unused") // stapler public List getPropertiesDescriptors() { - return ExtensionList.lookup(OidcPropertyDescriptor.class).stream() - .filter(OidcPropertyDescriptor::isApplicable) - .toList(); + return ExtensionList.lookup(OidcPropertyDescriptor.class); } } } diff --git a/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java b/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java index 21ae8547..ce8ca001 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java @@ -11,13 +11,6 @@ public static ExtensionList all() { return ExtensionList.lookup(OidcPropertyDescriptor.class); } - /** - * Allows the property to restrict its applicability depending on the context (for example, FIPS) - */ - public boolean isApplicable() { - return true; - } - /** * This method gets called if the property is not configured explicitly. For example, providing a default value. */ diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java index 44f3b085..41185b59 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java @@ -1,5 +1,6 @@ package org.jenkinsci.plugins.oic.properties; +import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import jenkins.security.FIPS140; @@ -52,11 +53,14 @@ public void customizeClient(@NonNull OidcClient client) { } } - @Extension public static class DescriptorImpl extends OidcPropertyDescriptor { - @Override - public boolean isApplicable() { - return !FIPS140.useCompliantAlgorithms(); + @Extension + @CheckForNull + public static DescriptorImpl createIfApplicable() { + if (FIPS140.useCompliantAlgorithms()) { + return null; + } + return new DescriptorImpl(); } @Override diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java index b067d178..3061ec8f 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java @@ -124,11 +124,14 @@ public Optional authenticate(@NonNull Authentication authenticat return Optional.empty(); } - @Extension public static class DescriptorImpl extends OidcPropertyDescriptor { - @Override - public boolean isApplicable() { - return !FIPS140.useCompliantAlgorithms(); + @Extension + @CheckForNull + public static DescriptorImpl createIfApplicable() { + if (FIPS140.useCompliantAlgorithms()) { + return null; + } + return new DescriptorImpl(); } @NonNull From 70a38fa8ff5895ba5e93a995f58c7f8eadf947f5 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 09:36:55 +0200 Subject: [PATCH 3/8] Only throw ObjectStreamException in readResolve --- .../plugins/oic/OicSecurityRealm.java | 54 +++++++++++-------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index ed59c96a..7f14f5ed 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -372,7 +372,7 @@ public OicSecurityRealm( } @SuppressWarnings("deprecated") - protected Object readResolve() throws IOException, FormException { + protected Object readResolve() throws ObjectStreamException { if (properties == null) { properties = new DescribableList<>(Saveable.NOOP); } @@ -388,31 +388,43 @@ protected Object readResolve() throws IOException, FormException { throw new IllegalStateException(Messages.OicSecurityRealm_EscapeHatchFipsMode()); } } - if (nonceDisabled) { - properties.replace(new DisableNonce()); - } - if (pkceEnabled) { - properties.replace(new Pkce()); - } - if (disableTokenVerification) { - properties.replace(new DisableTokenVerification()); - } - if (allowedTokenExpirationClockSkewSeconds != null && allowedTokenExpirationClockSkewSeconds != 60L) { - properties.replace(new AllowedTokenExpirationClockSkew(allowedTokenExpirationClockSkewSeconds.intValue())); - } - if (loginQueryParameters != null) { - properties.replace(new LoginQueryParameters(loginQueryParameters)); - } - if (logoutQueryParameters != null) { - properties.replace(new LogoutQueryParameters(logoutQueryParameters)); - } - if (escapeHatchEnabled) { - properties.replace(new EscapeHatch(escapeHatchUsername, escapeHatchGroup, escapeHatchSecret)); + try { + if (nonceDisabled) { + properties.replace(new DisableNonce()); + } + if (pkceEnabled) { + properties.replace(new Pkce()); + } + if (disableTokenVerification) { + properties.replace(new DisableTokenVerification()); + } + if (allowedTokenExpirationClockSkewSeconds != null && allowedTokenExpirationClockSkewSeconds != 60L) { + properties.replace(new AllowedTokenExpirationClockSkew(allowedTokenExpirationClockSkewSeconds.intValue())); + } + if (loginQueryParameters != null) { + properties.replace(new LoginQueryParameters(loginQueryParameters)); + } + if (logoutQueryParameters != null) { + properties.replace(new LogoutQueryParameters(logoutQueryParameters)); + } + if (escapeHatchEnabled) { + properties.replace(new EscapeHatch(escapeHatchUsername, escapeHatchGroup, escapeHatchSecret)); + } + } catch (IOException e) { + var ose = new InvalidObjectException("Error while migrating properties"); + ose.initCause(e); + throw ose; + } catch (FormException e) { + var ose = new InvalidObjectException(e.getFormField() + ": " + e.getMessage()); + ose.initCause(e); + throw ose; } + if (!Strings.isNullOrEmpty(endSessionUrl)) { this.endSessionEndpoint = endSessionUrl + "/"; } + // backward compatibility with wrong groupsFieldName split if (Strings.isNullOrEmpty(this.groupsFieldName) && !Strings.isNullOrEmpty(this.simpleGroupsFieldName)) { String originalGroupFieldName = this.simpleGroupsFieldName; From 6e4127b16d246c6e37a38908e62aaf5acad356dd Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 09:48:58 +0200 Subject: [PATCH 4/8] Fix reviews --- .../plugins/oic/OicSecurityRealm.java | 118 ++---------------- .../properties/DisableTokenVerification.java | 9 ++ .../plugins/oic/properties/EscapeHatch.java | 10 ++ .../org/jenkinsci/plugins/oic/TestRealm.java | 3 +- 4 files changed, 31 insertions(+), 109 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 7f14f5ed..acf783c7 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -381,10 +381,10 @@ protected Object readResolve() throws ObjectStreamException { if (isDisableSslVerification()) { throw new IllegalStateException(Messages.OicSecurityRealm_DisableSslVerificationFipsMode()); } - if (isDisableTokenVerification() || properties.get(DisableTokenVerification.class) != null) { + if (disableTokenVerification) { throw new IllegalStateException(Messages.OicSecurityRealm_DisableTokenVerificationFipsMode()); } - if (isEscapeHatchEnabled() || properties.get(EscapeHatch.class) != null) { + if (escapeHatchEnabled) { throw new IllegalStateException(Messages.OicSecurityRealm_EscapeHatchFipsMode()); } } @@ -398,8 +398,11 @@ protected Object readResolve() throws ObjectStreamException { if (disableTokenVerification) { properties.replace(new DisableTokenVerification()); } - if (allowedTokenExpirationClockSkewSeconds != null && allowedTokenExpirationClockSkewSeconds != 60L) { - properties.replace(new AllowedTokenExpirationClockSkew(allowedTokenExpirationClockSkewSeconds.intValue())); + if (allowedTokenExpirationClockSkewSeconds != null) { + var value = allowedTokenExpirationClockSkewSeconds.intValue(); + if (value != 60) { + properties.replace(new AllowedTokenExpirationClockSkew(value)); + } } if (loginQueryParameters != null) { properties.replace(new LoginQueryParameters(loginQueryParameters)); @@ -475,26 +478,6 @@ protected Object readResolve() throws ObjectStreamException { return this; } - @Deprecated - public void setLoginQueryParameters(List values) { - this.loginQueryParameters = values; - } - - @Deprecated - public List getLoginQueryParameters() { - return loginQueryParameters; - } - - @Deprecated - public void setLogoutQueryParameters(List values) { - this.logoutQueryParameters = values; - } - - @Deprecated - public List getLogoutQueryParameters() { - return logoutQueryParameters; - } - public String getClientId() { return clientId; } @@ -567,26 +550,6 @@ public String getPostLogoutRedirectUrl() { return postLogoutRedirectUrl; } - @Deprecated - public boolean isEscapeHatchEnabled() { - return escapeHatchEnabled; - } - - @Deprecated - public String getEscapeHatchUsername() { - return escapeHatchUsername; - } - - @Deprecated - public Secret getEscapeHatchSecret() { - return escapeHatchSecret; - } - - @Deprecated - public String getEscapeHatchGroup() { - return escapeHatchGroup; - } - public boolean isRootURLFromRequest() { return rootURLFromRequest; } @@ -615,15 +578,6 @@ public boolean isAllowTokenAccessWithoutOicSession() { return allowTokenAccessWithoutOicSession; } - /** - * @deprecated use {@link AllowedTokenExpirationClockSkew} instead. - * @return - */ - @Deprecated - public Long getAllowedTokenExpirationClockSkewSeconds() { - return allowedTokenExpirationClockSkewSeconds; - } - public DescribableList getProperties() { return properties; } @@ -749,19 +703,6 @@ public void setPostLogoutRedirectUrl(String postLogoutRedirectUrl) { this.postLogoutRedirectUrl = Util.fixEmptyAndTrim(postLogoutRedirectUrl); } - @Deprecated - public void setEscapeHatchEnabled(boolean escapeHatchEnabled) throws FormException { - if (FIPS140.useCompliantAlgorithms() && escapeHatchEnabled) { - throw new FormException("Escape Hatch cannot be enabled in FIPS environment", "escapeHatchEnabled"); - } - this.escapeHatchEnabled = escapeHatchEnabled; - } - - @Deprecated - public void setEscapeHatchUsername(String escapeHatchUsername) { - this.escapeHatchUsername = Util.fixEmptyAndTrim(escapeHatchUsername); - } - @DataBoundSetter public void setEscapeHatchSecret(Secret escapeHatchSecret) { if (escapeHatchSecret != null) { @@ -777,18 +718,6 @@ public void setEscapeHatchSecret(Secret escapeHatchSecret) { this.escapeHatchSecret = escapeHatchSecret; } - @Deprecated - protected boolean checkEscapeHatch(String username, String password) { - final boolean isUsernameMatch = username.equals(this.escapeHatchUsername); - final boolean isPasswordMatch = BCrypt.checkpw(password, Secret.toString(this.escapeHatchSecret)); - return isUsernameMatch & isPasswordMatch; - } - - @Deprecated - public void setEscapeHatchGroup(String escapeHatchGroup) { - this.escapeHatchGroup = Util.fixEmptyAndTrim(escapeHatchGroup); - } - @DataBoundSetter public void setRootURLFromRequest(boolean rootURLFromRequest) { this.rootURLFromRequest = rootURLFromRequest; @@ -799,26 +728,6 @@ public void setSendScopesInTokenRequest(boolean sendScopesInTokenRequest) { this.sendScopesInTokenRequest = sendScopesInTokenRequest; } - @Deprecated - public void setPkceEnabled(boolean pkceEnabled) { - this.pkceEnabled = pkceEnabled; - } - - @DataBoundSetter - @Deprecated - public void setDisableTokenVerification(boolean disableTokenVerification) throws FormException { - if (FIPS140.useCompliantAlgorithms() && disableTokenVerification) { - throw new FormException( - Messages.OicSecurityRealm_DisableTokenVerificationFipsMode(), "disableTokenVerification"); - } - this.disableTokenVerification = disableTokenVerification; - } - - @Deprecated - public void setNonceDisabled(boolean nonceDisabled) { - this.nonceDisabled = nonceDisabled; - } - @DataBoundSetter public void setTokenExpirationCheckDisabled(boolean tokenExpirationCheckDisabled) { this.tokenExpirationCheckDisabled = tokenExpirationCheckDisabled; @@ -829,14 +738,6 @@ public void setAllowTokenAccessWithoutOicSession(boolean allowTokenAccessWithout this.allowTokenAccessWithoutOicSession = allowTokenAccessWithoutOicSession; } - /** - * @deprecated use {@link AllowedTokenExpirationClockSkew} instead. - */ - @Deprecated - public void setAllowedTokenExpirationClockSkewSeconds(Long allowedTokenExpirationClockSkewSeconds) { - this.allowedTokenExpirationClockSkewSeconds = allowedTokenExpirationClockSkewSeconds; - } - @Override public String getLoginUrl() { // Login begins with our doCommenceLogin(String,String) method @@ -1285,13 +1186,14 @@ public void doFinishLogin(StaplerRequest2 request, StaplerResponse2 response) th throw new FailedCheckOfTokenException(client.getConfiguration().findLogoutUrl()); } + OicCredentials oicCredentials = new OicCredentials( accessToken == null ? null : accessToken.getValue(), // XXX (how) can the access token be null? idToken.getParsedString(), refreshToken != null ? refreshToken.getValue() : null, accessToken == null ? 0 : accessToken.getLifetime(), CLOCK.millis(), - getAllowedTokenExpirationClockSkewSeconds()); + (long) client.getConfiguration().getMaxClockSkew()); loginAndSetUserData(username, idToken, profile.getAttributes(), oicCredentials); @@ -1468,7 +1370,7 @@ private boolean refreshExpiredToken( refreshToken.getValue(), accessToken.getLifetime(), CLOCK.millis(), - getAllowedTokenExpirationClockSkewSeconds()); + (long) client.getConfiguration().getMaxClockSkew()); loginAndSetUserData(username, idToken, profile.getAttributes(), refreshedCredentials); return true; diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java index 41185b59..ab81d5b4 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java @@ -3,6 +3,7 @@ import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; +import java.io.Serial; import jenkins.security.FIPS140; import org.apache.commons.lang.Validate; import org.jenkinsci.plugins.oic.AnythingGoesTokenValidator; @@ -31,6 +32,14 @@ public OidcPropertyExecution newExecution(@NonNull OicServerConfiguration server return new ExecutionImpl(serverConfiguration); } + @Serial + protected Object readResolve() { + if (FIPS140.useCompliantAlgorithms()) { + throw new IllegalStateException(org.jenkinsci.plugins.oic.Messages.OicSecurityRealm_DisableTokenVerificationFipsMode()); + } + return this; + } + private record ExecutionImpl(OicServerConfiguration serverConfiguration) implements OidcPropertyExecution { @Override public void customizeConfiguration(@NonNull OidcConfiguration configuration) { diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java index 3061ec8f..6ea9cdb1 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java @@ -15,6 +15,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; +import java.io.Serial; import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -22,6 +23,7 @@ import java.util.regex.Pattern; import jenkins.security.FIPS140; import jenkins.security.SecurityListener; +import org.jenkinsci.plugins.oic.Messages; import org.jenkinsci.plugins.oic.OicUserDetails; import org.jenkinsci.plugins.oic.OidcProperty; import org.jenkinsci.plugins.oic.OidcPropertyDescriptor; @@ -141,6 +143,14 @@ public String getDisplayName() { } } + @Serial + protected Object readResolve() { + if (FIPS140.useCompliantAlgorithms()) { + throw new IllegalStateException(Messages.OicSecurityRealm_EscapeHatchFipsMode()); + } + return this; + } + /** * Excluding the escapeHatch login from CSRF protection as the crumb is calculated based on the authentication * mirroring behavior of the normal login page. diff --git a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java index 84097af2..871ade9c 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java +++ b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java @@ -6,6 +6,7 @@ import hudson.util.Secret; import io.burt.jmespath.Expression; import java.io.IOException; +import java.io.ObjectStreamException; import java.io.Serial; import java.text.ParseException; import java.util.ArrayList; @@ -327,7 +328,7 @@ public String getStringFieldFromJMESPath(Object object, String jmespathField) { @Serial @Override - public Object readResolve() throws IOException, Descriptor.FormException { + public Object readResolve() throws ObjectStreamException { return super.readResolve(); } From e2b8e198ea5ae1e72726d8bc8251d5ae72adf55e Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 12:33:44 +0200 Subject: [PATCH 5/8] Can't use the static factory for DisableTokenVerification, as there is a default implementation that is needed. --- .../org/jenkinsci/plugins/oic/OicSecurityRealm.java | 4 +++- .../plugins/oic/OidcPropertyDescriptor.java | 7 +++++++ .../oic/properties/DisableTokenVerification.java | 12 +++++------- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index acf783c7..096162e2 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -1528,7 +1528,9 @@ public IdStrategy defaultGroupIdStrategy() { @SuppressWarnings("unused") // stapler public List getPropertiesDescriptors() { - return ExtensionList.lookup(OidcPropertyDescriptor.class); + return ExtensionList.lookup(OidcPropertyDescriptor.class).stream() + .filter(OidcPropertyDescriptor::isApplicable) + .toList(); } } } diff --git a/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java b/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java index ce8ca001..21ae8547 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OidcPropertyDescriptor.java @@ -11,6 +11,13 @@ public static ExtensionList all() { return ExtensionList.lookup(OidcPropertyDescriptor.class); } + /** + * Allows the property to restrict its applicability depending on the context (for example, FIPS) + */ + public boolean isApplicable() { + return true; + } + /** * This method gets called if the property is not configured explicitly. For example, providing a default value. */ diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java index ab81d5b4..16b6d1e5 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java @@ -62,14 +62,12 @@ public void customizeClient(@NonNull OidcClient client) { } } + @Extension public static class DescriptorImpl extends OidcPropertyDescriptor { - @Extension - @CheckForNull - public static DescriptorImpl createIfApplicable() { - if (FIPS140.useCompliantAlgorithms()) { - return null; - } - return new DescriptorImpl(); + + @Override + public boolean isApplicable() { + return !FIPS140.useCompliantAlgorithms(); } @Override From 56c562ac35c1b9b3c9e5d6aae10d6ddff368fbd0 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 12:35:07 +0200 Subject: [PATCH 6/8] Simplify readResolve, these are handled in corresponding properties contructors. --- .../org/jenkinsci/plugins/oic/OicSecurityRealm.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 096162e2..9d2ffa73 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -377,16 +377,8 @@ protected Object readResolve() throws ObjectStreamException { properties = new DescribableList<>(Saveable.NOOP); } // Fail if migrating to a FIPS non-compliant config - if (FIPS140.useCompliantAlgorithms()) { - if (isDisableSslVerification()) { - throw new IllegalStateException(Messages.OicSecurityRealm_DisableSslVerificationFipsMode()); - } - if (disableTokenVerification) { - throw new IllegalStateException(Messages.OicSecurityRealm_DisableTokenVerificationFipsMode()); - } - if (escapeHatchEnabled) { - throw new IllegalStateException(Messages.OicSecurityRealm_EscapeHatchFipsMode()); - } + if (FIPS140.useCompliantAlgorithms() && isDisableSslVerification()) { + throw new IllegalStateException(Messages.OicSecurityRealm_DisableSslVerificationFipsMode()); } try { if (nonceDisabled) { From 3332e15c227baf7d0eee8123a7635ee3afe73b9f Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 13:09:40 +0200 Subject: [PATCH 7/8] Clean up test references of removed methods --- .../plugins/oic/OicSecurityRealm.java | 12 ------ .../properties/DisableTokenVerification.java | 4 +- .../plugins/oic/properties/EscapeHatch.java | 14 +++++-- .../plugins/oic/ConfigurationAsCodeTest.java | 13 ++----- .../plugins/oic/OicSecurityRealmTest.java | 38 ------------------- .../SecurityRealmConfigurationFIPSTest.java | 16 ++++---- .../org/jenkinsci/plugins/oic/TestRealm.java | 30 ++++++--------- .../oic/properties/EscapeHatchTest.java | 27 +++++++++++++ 8 files changed, 64 insertions(+), 90 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 9d2ffa73..90cdb75e 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -550,18 +550,6 @@ public boolean isSendScopesInTokenRequest() { return sendScopesInTokenRequest; } - public boolean isPkceEnabled() { - return pkceEnabled; - } - - public boolean isDisableTokenVerification() { - return disableTokenVerification; - } - - public boolean isNonceDisabled() { - return nonceDisabled; - } - public boolean isTokenExpirationCheckDisabled() { return tokenExpirationCheckDisabled; } diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java index 16b6d1e5..b99906a3 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/DisableTokenVerification.java @@ -1,6 +1,5 @@ package org.jenkinsci.plugins.oic.properties; -import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import java.io.Serial; @@ -35,7 +34,8 @@ public OidcPropertyExecution newExecution(@NonNull OicServerConfiguration server @Serial protected Object readResolve() { if (FIPS140.useCompliantAlgorithms()) { - throw new IllegalStateException(org.jenkinsci.plugins.oic.Messages.OicSecurityRealm_DisableTokenVerificationFipsMode()); + throw new IllegalStateException( + org.jenkinsci.plugins.oic.Messages.OicSecurityRealm_DisableTokenVerificationFipsMode()); } return this; } diff --git a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java index 6ea9cdb1..eabcf75b 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java +++ b/src/main/java/org/jenkinsci/plugins/oic/properties/EscapeHatch.java @@ -56,7 +56,7 @@ public class EscapeHatch extends OidcProperty { public EscapeHatch(@NonNull String username, @CheckForNull String group, @NonNull Secret secret) throws Descriptor.FormException { if (FIPS140.useCompliantAlgorithms()) { - throw new IllegalStateException("Cannot use Escape Hatch in FIPS-140 mode"); + throw new Descriptor.FormException("Cannot use Escape Hatch in FIPS-140 mode", "escapeHatch"); } var sanitizedUsername = Util.fixEmptyAndTrim(username); if (sanitizedUsername == null) { @@ -106,8 +106,9 @@ private void randomWait() { public Optional authenticate(@NonNull Authentication authentication) { if (authentication instanceof UsernamePasswordAuthenticationToken) { randomWait(); // to slowdown brute forcing - if (authentication.getPrincipal().toString().equals(this.username) - && BCrypt.checkpw(authentication.getCredentials().toString(), Secret.toString(this.secret))) { + if (check( + authentication.getPrincipal().toString(), + authentication.getCredentials().toString())) { List grantedAuthorities = new ArrayList<>(); grantedAuthorities.add(SecurityRealm.AUTHENTICATED_AUTHORITY2); if (isNotBlank(group)) { @@ -126,6 +127,13 @@ public Optional authenticate(@NonNull Authentication authenticat return Optional.empty(); } + /** + * Check a given username and password against the configured ones. + */ + public boolean check(@NonNull String username, @CheckForNull String password) { + return username.equals(this.username) && BCrypt.checkpw(password, Secret.toString(this.secret)); + } + public static class DescriptorImpl extends OidcPropertyDescriptor { @Extension @CheckForNull diff --git a/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java b/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java index 9e6b058a..e231cfc2 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java @@ -8,6 +8,7 @@ import static io.jenkins.plugins.casc.misc.Util.toStringFromYamlFile; import static io.jenkins.plugins.casc.misc.Util.toYamlString; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -87,7 +88,6 @@ void testConfig(JenkinsConfiguredWithCodeRule j) { assertEquals("userNameField", oicSecurityRealm.getUserNameField()); assertTrue(oicSecurityRealm.isRootURLFromRequest()); assertEquals("http://localhost/jwks", serverConf.getJwksServerUrl()); - assertFalse(oicSecurityRealm.isDisableTokenVerification()); var loginQueryParameters = oicSecurityRealm.getProperties().get(LoginQueryParameters.class); assertThat(loginQueryParameters, notNullValue()); assertEquals( @@ -145,7 +145,6 @@ void testMinimal(JenkinsConfiguredWithCodeRule j) { assertEquals("clientSecret", Secret.toString(oicSecurityRealm.getClientSecret())); assertFalse(oicSecurityRealm.isDisableSslVerification()); assertNull(oicSecurityRealm.getEmailFieldName()); - assertFalse(oicSecurityRealm.isEscapeHatchEnabled()); assertNull(oicSecurityRealm.getFullNameFieldName()); assertNull(oicSecurityRealm.getGroupsFieldName()); assertEquals("openid email", serverConf.getScopes()); @@ -155,9 +154,7 @@ void testMinimal(JenkinsConfiguredWithCodeRule j) { assertTrue(oicSecurityRealm.isLogoutFromOpenidProvider()); assertFalse(oicSecurityRealm.isRootURLFromRequest()); assertNull(serverConf.getJwksServerUrl()); - assertFalse(oicSecurityRealm.isDisableTokenVerification()); - assertNull(oicSecurityRealm.getLoginQueryParameters()); - assertNull(oicSecurityRealm.getLogoutQueryParameters()); + assertThat(oicSecurityRealm.getProperties(), empty()); } @Test @@ -175,7 +172,6 @@ void testMinimalWellKnown(JenkinsConfiguredWithCodeRule j) { assertFalse(oicSecurityRealm.isDisableSslVerification()); assertNull(oicSecurityRealm.getEmailFieldName()); - assertFalse(oicSecurityRealm.isEscapeHatchEnabled()); assertNull(oicSecurityRealm.getFullNameFieldName()); assertNull(oicSecurityRealm.getGroupsFieldName()); @@ -184,12 +180,9 @@ void testMinimalWellKnown(JenkinsConfiguredWithCodeRule j) { assertEquals("sub", oicSecurityRealm.getUserNameField()); assertTrue(oicSecurityRealm.isLogoutFromOpenidProvider()); - assertFalse(oicSecurityRealm.isDisableTokenVerification()); assertEquals(urlBase + "/well.known", serverConf.getWellKnownOpenIDConfigurationUrl()); - - assertNull(oicSecurityRealm.getLoginQueryParameters()); - assertNull(oicSecurityRealm.getLogoutQueryParameters()); + assertThat(oicSecurityRealm.getProperties(), empty()); } /** Class to setup WellKnownMockExtension for well known with stub and setting port in env variable diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java index 53ecf3fe..a9e1eafc 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java @@ -7,10 +7,8 @@ import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import hudson.util.Secret; @@ -32,7 +30,6 @@ import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; -import org.springframework.security.crypto.bcrypt.BCrypt; @WithJenkins class OicSecurityRealmTest { @@ -118,41 +115,6 @@ void testShouldReturnRootUrlWhenRedirectUrlIsInvalid(JenkinsRule jenkinsRule) th assertEquals(rootUrl, realm.getValidRedirectUrl("http://localhost/jenkins/../bar/")); } - @Test - void testShouldCheckEscapeHatchWithPlainPassword(JenkinsRule jenkinsRule) throws Exception { - final String escapeHatchUsername = "aUsername"; - final String escapeHatchPassword = "aSecretPassword"; - - TestRealm realm = new TestRealm.Builder(wireMock) - .WithMinimalDefaults() - .WithEscapeHatch(true, escapeHatchUsername, escapeHatchPassword, "Group") - .build(); - - assertEquals(escapeHatchUsername, realm.getEscapeHatchUsername()); - assertNotEquals(escapeHatchPassword, Secret.toString(realm.getEscapeHatchSecret())); - assertTrue(realm.doCheckEscapeHatch(escapeHatchUsername, escapeHatchPassword)); - assertFalse(realm.doCheckEscapeHatch("otherUsername", escapeHatchPassword)); - assertFalse(realm.doCheckEscapeHatch(escapeHatchUsername, "wrongPassword")); - } - - @Test - void testShouldCheckEscapeHatchWithHashedPassword(JenkinsRule jenkinsRule) throws Exception { - final String escapeHatchUsername = "aUsername"; - final String escapeHatchPassword = "aSecretPassword"; - final String escapeHatchCryptedPassword = BCrypt.hashpw(escapeHatchPassword, BCrypt.gensalt()); - - TestRealm realm = new TestRealm.Builder(wireMock) - .WithMinimalDefaults() - .WithEscapeHatch(true, escapeHatchUsername, escapeHatchCryptedPassword, "Group") - .build(); - - assertEquals(escapeHatchUsername, realm.getEscapeHatchUsername()); - assertEquals(escapeHatchCryptedPassword, Secret.toString(realm.getEscapeHatchSecret())); - assertTrue(realm.doCheckEscapeHatch(escapeHatchUsername, escapeHatchPassword)); - assertFalse(realm.doCheckEscapeHatch("otherUsername", escapeHatchPassword)); - assertFalse(realm.doCheckEscapeHatch(escapeHatchUsername, "wrongPassword")); - } - @Test @WithoutJenkins public void testMaybeOpenIdLogoutEndpoint() throws Exception { diff --git a/src/test/java/org/jenkinsci/plugins/oic/SecurityRealmConfigurationFIPSTest.java b/src/test/java/org/jenkinsci/plugins/oic/SecurityRealmConfigurationFIPSTest.java index d8518005..dc8fea6a 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/SecurityRealmConfigurationFIPSTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/SecurityRealmConfigurationFIPSTest.java @@ -1,11 +1,13 @@ package org.jenkinsci.plugins.oic; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertThrows; import hudson.model.Descriptor; +import hudson.util.Secret; import jenkins.security.FIPS140; +import org.jenkinsci.plugins.oic.properties.EscapeHatch; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -31,22 +33,22 @@ static void tearDown() { @Test void escapeHatchThrowsException() { assertThrows( - Descriptor.FormException.class, - () -> new OicSecurityRealm("clientId", null, null, null, null, null).setEscapeHatchEnabled(true)); + Descriptor.FormException.class, () -> new OicSecurityRealm("clientId", null, null, null, null, null) + .getProperties() + .add(new EscapeHatch("admin", null, Secret.fromString("very-secret")))); } @Test void escapeHatchToFalse() throws Exception { OicSecurityRealm oicSecurityRealm = new OicSecurityRealm("clientId", null, null, null, null, null); - oicSecurityRealm.setEscapeHatchEnabled(false); - assertThat(oicSecurityRealm.isEscapeHatchEnabled(), is(false)); + assertThat(oicSecurityRealm.getProperties().get(EscapeHatch.class), nullValue()); } @Test void readResolve() throws Exception { OicSecurityRealm oicSecurityRealm = new OicSecurityRealm("clientId", null, null, null, null, null); - oicSecurityRealm.setEscapeHatchEnabled(false); - assertThat(oicSecurityRealm.isEscapeHatchEnabled(), is(false)); + assertThat(oicSecurityRealm.getProperties().get(EscapeHatch.class), nullValue()); oicSecurityRealm.readResolve(); + assertThat(oicSecurityRealm.getProperties().get(EscapeHatch.class), nullValue()); } } diff --git a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java index 871ade9c..63d94f6a 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java +++ b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java @@ -12,7 +12,9 @@ import java.util.ArrayList; import java.util.List; import jenkins.model.IdStrategy; +import org.jenkinsci.plugins.oic.properties.DisableNonce; import org.jenkinsci.plugins.oic.properties.DisableTokenVerification; +import org.jenkinsci.plugins.oic.properties.EscapeHatch; import org.jenkinsci.plugins.oic.properties.LoginQueryParameters; import org.jenkinsci.plugins.oic.properties.LogoutQueryParameters; import org.kohsuke.stapler.StaplerRequest2; @@ -54,10 +56,6 @@ public static class Builder { public Boolean logoutFromOpenidProvider = false; public String endSessionEndpoint = null; public String postLogoutRedirectUrl = null; - public boolean escapeHatchEnabled = false; - public String escapeHatchUsername = null; - public Secret escapeHatchSecret = null; - public String escapeHatchGroup = null; public boolean automanualconfigure = false; public IdStrategy userIdStrategy; public IdStrategy groupIdStrategy; @@ -151,11 +149,14 @@ public Builder WithEscapeHatch( boolean escapeHatchEnabled, String escapeHatchUsername, String escapeHatchSecret, - String escapeHatchGroup) { - this.escapeHatchEnabled = escapeHatchEnabled; - this.escapeHatchUsername = escapeHatchUsername; - this.escapeHatchSecret = escapeHatchSecret == null ? null : Secret.fromString(escapeHatchSecret); - this.escapeHatchGroup = escapeHatchGroup; + String escapeHatchGroup) + throws Descriptor.FormException { + if (escapeHatchEnabled) { + this.properties.add( + new EscapeHatch(escapeHatchUsername, escapeHatchGroup, Secret.fromString(escapeHatchSecret))); + } else { + this.properties.removeIf(EscapeHatch.class::isInstance); + } return this; } @@ -234,10 +235,6 @@ public TestRealm(Builder builder) throws Exception { this.setGroupsFieldName(builder.groupsFieldName); this.setLogoutFromOpenidProvider(builder.logoutFromOpenidProvider); this.setPostLogoutRedirectUrl(builder.postLogoutRedirectUrl); - this.setEscapeHatchEnabled(builder.escapeHatchEnabled); - this.setEscapeHatchUsername(builder.escapeHatchUsername); - this.setEscapeHatchSecret(builder.escapeHatchSecret); - this.setEscapeHatchGroup(builder.escapeHatchGroup); this.setProperties(builder.properties); // need to call the following method annotated with @PostConstruct and called // from readResolve and as such @@ -307,7 +304,8 @@ public void doFinishLogin(StaplerRequest2 request, StaplerResponse2 response) th /* * PluginTest uses a hardCoded nonce "nonce" */ - if (!isNonceDisabled()) { + + if (getProperties().get(DisableNonce.class) == null) { // only hack the nonce if the nonce is enabled FrameworkParameters parameters = new JEEFrameworkParameters(request, response); WebContext webContext = JEEContextFactory.INSTANCE.newContext(parameters); @@ -331,8 +329,4 @@ public String getStringFieldFromJMESPath(Object object, String jmespathField) { public Object readResolve() throws ObjectStreamException { return super.readResolve(); } - - public boolean doCheckEscapeHatch(String username, String password) { - return super.checkEscapeHatch(username, password); - } } diff --git a/src/test/java/org/jenkinsci/plugins/oic/properties/EscapeHatchTest.java b/src/test/java/org/jenkinsci/plugins/oic/properties/EscapeHatchTest.java index 3f230aea..858cfcbd 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/properties/EscapeHatchTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/properties/EscapeHatchTest.java @@ -1,14 +1,18 @@ package org.jenkinsci.plugins.oic.properties; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import hudson.util.Secret; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; import org.jenkinsci.plugins.oic.MockHttpServletRequest; import org.junit.jupiter.api.Test; +import org.springframework.security.crypto.bcrypt.BCrypt; class EscapeHatchTest { private EscapeHatch.CrumbExclusionImpl crumb = new EscapeHatch.CrumbExclusionImpl(); @@ -47,4 +51,27 @@ void process_WithGoodPath() throws IOException, ServletException { MockHttpServletRequest request = newRequestWithPath("/securityRealm/escapeHatch"); assertTrue(crumb.process(request, response, chain)); } + + @Test + void testShouldCheckEscapeHatchWithPlainPassword() throws Exception { + final String escapeHatchUsername = "aUsername"; + final String escapeHatchPassword = "aSecretPassword"; + var escapeHatch = new EscapeHatch(escapeHatchUsername, null, Secret.fromString(escapeHatchPassword)); + assertEquals(escapeHatchUsername, escapeHatch.getUsername()); + assertNotEquals(escapeHatchPassword, Secret.toString(escapeHatch.getSecret())); + assertTrue(escapeHatch.check(escapeHatchUsername, escapeHatchPassword)); + assertFalse(escapeHatch.check("otherUsername", escapeHatchPassword)); + assertFalse(escapeHatch.check(escapeHatchUsername, "wrongPassword")); + } + + @Test + void testShouldCheckEscapeHatchWithHashedPassword() throws Exception { + final String escapeHatchUsername = "aUsername"; + final String escapeHatchPassword = "aSecretPassword"; + final String escapeHatchCryptedPassword = BCrypt.hashpw(escapeHatchPassword, BCrypt.gensalt()); + + var escapeHatch = new EscapeHatch(escapeHatchUsername, null, Secret.fromString(escapeHatchCryptedPassword)); + assertEquals(escapeHatchUsername, escapeHatch.getUsername()); + assertEquals(escapeHatchCryptedPassword, Secret.toString(escapeHatch.getSecret())); + } } From 9a9a85de4bd5cdac408cba557302432999556a38 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 10 Sep 2025 14:18:09 +0200 Subject: [PATCH 8/8] Spotless --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 90cdb75e..195bf28d 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -419,7 +419,6 @@ protected Object readResolve() throws ObjectStreamException { this.endSessionEndpoint = endSessionUrl + "/"; } - // backward compatibility with wrong groupsFieldName split if (Strings.isNullOrEmpty(this.groupsFieldName) && !Strings.isNullOrEmpty(this.simpleGroupsFieldName)) { String originalGroupFieldName = this.simpleGroupsFieldName; @@ -1166,7 +1165,6 @@ public void doFinishLogin(StaplerRequest2 request, StaplerResponse2 response) th throw new FailedCheckOfTokenException(client.getConfiguration().findLogoutUrl()); } - OicCredentials oicCredentials = new OicCredentials( accessToken == null ? null : accessToken.getValue(), // XXX (how) can the access token be null? idToken.getParsedString(),