SECURITY-3441

jenkinsci · Sep 25, 2024 · 3afbfca · 3afbfca
1 parent 321ce67
commit 3afbfca
Show file tree

Hide file tree

Showing 13 changed files with 125 additions and 6 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
@@ -823,12 +823,15 @@ private OicJsonWebTokenVerifier getJwksVerifier() {
         if (jwtVerifier == null) {
             jwtVerifier = new OicJsonWebTokenVerifier(
                     serverConfiguration.getJwksServerUrl(),
-                    new OicJsonWebTokenVerifier.Builder().setHttpTransportFactory(new HttpTransportFactory() {
-                        @Override
-                        public HttpTransport create() {
-                            return httpTransport;
-                        }
-                    }));
+                    new OicJsonWebTokenVerifier.Builder()
+                            .setHttpTransportFactory(new HttpTransportFactory() {
+                                @Override
+                                public HttpTransport create() {
+                                    return httpTransport;
+                                }
+                            })
+                            .setIssuer(getServerConfiguration().getIssuer())
+                            .setAudience(List.of(clientId)));
         }
         return jwtVerifier;
     }

diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicServerConfiguration.java b/src/main/java/org/jenkinsci/plugins/oic/OicServerConfiguration.java
@@ -33,5 +33,8 @@ public abstract class OicServerConfiguration extends AbstractDescribableImpl<Oic
     @CheckForNull
     public abstract String getEndSessionUrl();
 
+    @CheckForNull
+    public abstract String getIssuer();
+
     public abstract boolean isUseRefreshTokens();
 }
diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java b/src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java
@@ -32,6 +32,7 @@ public class OicServerManualConfiguration extends OicServerConfiguration {
     private String scopes = "openid email";
     private String userInfoServerUrl;
     private boolean useRefreshTokens;
+    private String issuer;
 
     @DataBoundConstructor
     public OicServerManualConfiguration(String tokenServerUrl, String authorizationServerUrl) throws FormException {
@@ -49,6 +50,11 @@ public void setEndSessionUrl(@Nullable String endSessionUrl) {
         this.endSessionUrl = endSessionUrl;
     }
 
+    @DataBoundSetter
+    public void setIssuer(@Nullable String issuer) {
+        this.issuer = Util.fixEmptyAndTrim(issuer);
+    }
+
     @DataBoundSetter
     public void setJwksServerUrl(@Nullable String jwksServerUrl) {
         this.jwksServerUrl = jwksServerUrl;
@@ -80,6 +86,12 @@ public String getEndSessionUrl() {
         return endSessionUrl;
     }
 
+    @Override
+    @CheckForNull
+    public String getIssuer() {
+        return issuer;
+    }
+
     @Override
     public boolean isUseRefreshTokens() {
         return useRefreshTokens;
@@ -154,6 +166,15 @@ public FormValidation doCheckEndSessionUrl(@QueryParameter String value) {
             }
         }
 
+        @POST
+        public FormValidation doCheckIssuer(@QueryParameter String issuer) {
+            Jenkins.get().checkPermission(Jenkins.ADMINISTER);
+            if (Util.fixEmptyAndTrim(issuer) == null) {
+                return FormValidation.warning(Messages.OicSecurityRealm_IssuerRecommended());
+            }
+            return FormValidation.ok();
+        }
+
         @POST
         public FormValidation doCheckJwksServerUrl(@QueryParameter String value) {
             Jenkins.get().checkPermission(Jenkins.ADMINISTER);

diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java b/src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java
@@ -48,6 +48,7 @@ public class OicServerWellKnownConfiguration extends OicServerConfiguration {
     private transient String userInfoServerUrl;
     private transient boolean useRefreshTokens;
     private transient TokenAuthMethod tokenAuthMethod;
+    private transient String issuer;
 
     /**
      * Time of the wellknown configuration expiration
@@ -77,6 +78,13 @@ public String getEndSessionUrl() {
         return endSessionUrl;
     }
 
+    @Override
+    @CheckForNull
+    public String getIssuer() {
+        loadWellKnownConfigIfNeeded();
+        return issuer;
+    }
+
     @Override
     public String getJwksServerUrl() {
         loadWellKnownConfigIfNeeded();
@@ -158,6 +166,7 @@ private void loadWellKnownConfigIfNeeded() {
                             WellKnownOpenIDConfigurationResponse.class);
 
             this.authorizationServerUrl = config.getAuthorizationEndpoint();
+            this.issuer = config.getIssuer();
             this.tokenServerUrl = config.getTokenEndpoint();
             this.jwksServerUrl = config.getJwksUri();
             this.tokenAuthMethod = config.getPreferredTokenAuthMethod();

diff --git a/src/main/java/org/jenkinsci/plugins/oic/WellKnownOpenIDConfigurationResponse.java b/src/main/java/org/jenkinsci/plugins/oic/WellKnownOpenIDConfigurationResponse.java
@@ -21,6 +21,9 @@ public class WellKnownOpenIDConfigurationResponse extends GenericJson {
     @Key("authorization_endpoint")
     private String authorizationEndpoint;
 
+    @Key("issuer")
+    private String issuer;
+
     @Key("token_endpoint")
     private String tokenEndpoint;
 
@@ -42,6 +45,10 @@ public class WellKnownOpenIDConfigurationResponse extends GenericJson {
     @Key("end_session_endpoint")
     private String endSessionEndpoint;
 
+    public String getIssuer() {
+        return issuer;
+    }
+
     public String getAuthorizationEndpoint() {
         return authorizationEndpoint;
     }
@@ -120,6 +127,9 @@ public boolean equals(Object o) {
         if (!Objects.equal(authorizationEndpoint, obj.getAuthorizationEndpoint())) {
             return false;
         }
+        if (!Objects.equal(issuer, obj.getIssuer())) {
+            return false;
+        }
         if (!Objects.equal(tokenEndpoint, obj.getTokenEndpoint())) {
             return false;
         }
@@ -148,6 +158,7 @@ public boolean equals(Object o) {
     @Override
     public int hashCode() {
         return (authorizationEndpoint
+                        + issuer
                         + tokenAuthMethods
                         + tokenEndpoint
                         + userinfoEndpoint

diff --git a/src/main/resources/org/jenkinsci/plugins/oic/Messages.properties b/src/main/resources/org/jenkinsci/plugins/oic/Messages.properties
@@ -8,6 +8,7 @@ OicSecurityRealm.NotAValidURL = Not a valid url.
 OicSecurityRealm.CouldNotRetreiveWellKnownConfig = Could not retrieve well-known config {0,number,integer} {1}
 OicSecurityRealm.CouldNotParseResponse = Could not parse response
 OicSecurityRealm.ErrorRetreivingWellKnownConfig = Error when retrieving well-known config
+OicSecurityRealm.IssuerRecommended = For security reasons it is strongly recommended to provide an issuer.
 OicSecurityRealm.TokenServerURLKeyRequired = Token Server Url Key is required.
 OicSecurityRealm.TokenAuthMethodRequired = Token auth method is required.
 OicSecurityRealm.UsingDefaultUsername = Using ''sub''.

diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.jelly b/src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.jelly
@@ -12,6 +12,9 @@
             checked="${instance.tokenAuthMethod == null || instance.tokenAuthMethod == 'client_secret_post'}" value="client_secret_post" inline="true" help="${null}"/>
     </f:entry>
 
+    <f:entry title="${%Issuer}" field="issuer">
+      <f:textbox/>
+    </f:entry>
     <f:entry title="${%AuthorizationServerUrl}" field="authorizationServerUrl">
         <f:textbox />
     </f:entry>

diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.properties b/src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.properties
@@ -1,6 +1,7 @@
 AuthorizationServerUrl=Authorization server url
 Basic=Basic
 EndSessionUrl=End session URL for OpenID Provider
+Issuer=Issuer
 JwksServerUrl=Jwks server url
 Post=Post
 Scopes=Scopes

diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/help-issuer.html b/src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/help-issuer.html
@@ -0,0 +1,3 @@
+<div>
+    Strongly recommended. The received ID Token's issuer must match the specified issuer.
+</div>
diff --git a/...main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/help-issuer_fr.html b/...main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/help-issuer_fr.html
@@ -0,0 +1,3 @@
+<div>
+    Fortement recommandé. Le Token ID reçu doit avoir l'émetteur indiqué.
+</div>
diff --git a/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java b/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java
@@ -47,6 +47,7 @@
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.Url;
 import org.kohsuke.stapler.Stapler;
@@ -987,6 +988,58 @@ public void loginWithCheckTokenFailure() throws Exception {
         assertAnonymous();
     }
 
+    @Test
+    @Issue("SECURITY-3441")
+    public void loginWithIncorrectIssuerFails() throws Exception {
+        mockAuthorizationRedirectsToFinishLogin();
+        mockTokenReturnsIdTokenWithGroup();
+        jenkins.setSecurityRealm(
+                new TestRealm.Builder(wireMockRule).WithIssuer("another_issuer").build());
+        assertAnonymous();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        browseLoginPage();
+        assertAnonymous();
+    }
+
+    @Test
+    @Issue("SECURITY-3441")
+    public void loginWithoutIssuerSetSucceeds() throws Exception {
+        mockAuthorizationRedirectsToFinishLogin();
+        mockTokenReturnsIdTokenWithGroup();
+        jenkins.setSecurityRealm(
+                new TestRealm.Builder(wireMockRule).WithIssuer(null).build());
+        assertAnonymous();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        browseLoginPage();
+        assertTestUser();
+    }
+
+    @Test
+    @Issue("SECURITY-3441")
+    public void loginWithEmptyIssuerSetSucceeds() throws Exception {
+        mockAuthorizationRedirectsToFinishLogin();
+        mockTokenReturnsIdTokenWithGroup();
+        jenkins.setSecurityRealm(
+                new TestRealm.Builder(wireMockRule).WithIssuer(null).build());
+        assertAnonymous();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        browseLoginPage();
+        assertTestUser();
+    }
+
+    @Test
+    @Issue("SECURITY-3441")
+    public void loginWithIncorrectAudienceFails() throws Exception {
+        mockAuthorizationRedirectsToFinishLogin();
+        mockTokenReturnsIdTokenWithGroup();
+        jenkins.setSecurityRealm(new TestRealm.Builder(wireMockRule)
+                .WithClient("another_client_id", "client_secret").build());
+        assertAnonymous();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        browseLoginPage();
+        assertAnonymous();
+    }
+
     @Test
     public void testAccessUsingJenkinsApiTokens() throws Exception {
         mockAuthorizationRedirectsToFinishLogin();

diff --git a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java
@@ -20,6 +20,7 @@ public class TestRealm extends OicSecurityRealm {
     public static class Builder {
         public String clientId = CLIENT_ID;
         public Secret clientSecret = Secret.fromString("secret");
+        public String issuer = "issuer";
         public String wellKnownOpenIDConfigurationUrl;
         public String tokenServerUrl;
         public String jwksServerUrl = null;
@@ -59,6 +60,11 @@ public Builder WithClient(String clientId, String clientSecret) {
             return this;
         }
 
+        public Builder WithIssuer(String issuer) {
+            this.issuer = issuer;
+            return this;
+        }
+
         public Builder WithUserInfoServerUrl(String userInfoServerUrl) {
             this.userInfoServerUrl = userInfoServerUrl;
             return this;
@@ -139,6 +145,7 @@ public OicServerConfiguration buildServerConfiguration() {
                 }
                 conf.setJwksServerUrl(jwksServerUrl);
                 conf.setEndSessionUrl(endSessionEndpoint);
+                conf.setIssuer(issuer);
                 return conf;
             } catch (Exception e) {
                 throw new IllegalArgumentException(e);

diff --git a/src/test/java/org/jenkinsci/plugins/oic/WellKnownOpenIDConfigurationResponseTest.java b/src/test/java/org/jenkinsci/plugins/oic/WellKnownOpenIDConfigurationResponseTest.java
@@ -75,6 +75,7 @@ public class WellKnownOpenIDConfigurationResponseTest {
             Set.of("token_endpoint_auth_methods_supported", "scopes_supported", "grant_types_supported");
     private static final List<String> FIELDS = List.of(
             "authorization_endpoint",
+            "issuer",
             "token_endpoint",
             "userinfo_endpoint",
             "jwks_uri",