Merge pull request #49 from morincer/csrf-token-2020

Csrf token 2020
morincer · Jul 24, 2020 · e6526af · e6526af
2 parents 29546ca + eeb5b2f
commit e6526af
Show file tree

Hide file tree

Showing 18 changed files with 194 additions and 32 deletions.
diff --git a/README.md b/README.md
@@ -49,17 +49,7 @@ Please refer the example of set up for [Okta](./docs/OktaSetup.md) if you need s
 
 * Make sure you have properly set up users on the TeamCity side - their usernames should match the SAML assertion name ID (usually - e-mail). 
 
-* **Make sure your CORS settings allow posts from the IdP site**. Go to the Adminstration -> Diagnostics -> Internal Properties -> Edit internal property and add/edit line:
-
-```
-rest.cors.origins=<value of the remote host address> 
-```
-
-> **Teamcity 2020.1+ Update** 
->
->Starting from version 2020.1 Teamcity changed the way they do CSRF protection: along with standard origin check they now require additional token to be acquired and sent as a header by remote host (read details and rationale [here](https://www.jetbrains.com/help/teamcity/csrf-protection.html)).
->
->Unfortunately, at the moment it is not clear how to make this mechanism work with SAML flows - as IdPs don't normally send custom headers to SPs consumers. If you configured CORS and get "token is missing" error, the possible workaround would be to disable the token checking by setting internal property teamcity.csrf.paranoid=false 
+* (Optional) Review CORS configuration - the plugin adds exception to the Teamcity's CORS filter - if request is POST sent to the SAML login callback URL and it contains a SAML message - it is considered CORS safe. If such behavior is by some reason is not acceptable the filter exception can be disabled on the plugin configuration page (Misc -> CORS Filter Exception checkbox) 
 
 * Logout and click the "Login with SSO" button to test. 
 

diff --git a/build/pom.xml b/build/pom.xml
@@ -4,7 +4,7 @@
   <parent>
     <artifactId>saml-authentication</artifactId>
     <groupId>org.gromozeka.teamcity</groupId>
-    <version>1.2</version>
+    <version>1.2.1</version>
   </parent>
 
   <artifactId>build</artifactId>

diff --git a/pom.xml b/pom.xml
@@ -3,10 +3,9 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>org.gromozeka.teamcity</groupId>
   <artifactId>saml-authentication</artifactId>
-  <version>1.2</version>
+  <version>1.2.1</version>
   <packaging>pom</packaging>
   <properties>
-      <pluginVersion>1.2</pluginVersion>
       <teamcity-version-lib>2020.1</teamcity-version-lib>
       <teamcity-version-runtime>2020.1.2</teamcity-version-runtime>
       <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

diff --git a/saml-authentication-server/pom.xml b/saml-authentication-server/pom.xml
@@ -5,7 +5,7 @@
     <parent>
         <artifactId>saml-authentication</artifactId>
         <groupId>org.gromozeka.teamcity</groupId>
-        <version>1.2</version>
+        <version>1.2.1</version>
     </parent>
     <artifactId>saml-authentication-server</artifactId>
     <packaging>jar</packaging>

diff --git a/...server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlAuthenticationScheme.java b/...server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlAuthenticationScheme.java
@@ -46,15 +46,18 @@ public class SamlAuthenticationScheme extends HttpAuthenticationSchemeAdapter {
     private RootUrlHolder rootUrlHolder;
     private SamlPluginSettingsStorage settingsStorage;
     private UserModel userModel;
+    private LoginConfiguration loginConfiguration;
 
 
     public SamlAuthenticationScheme(
             @NotNull RootUrlHolder rootUrlHolder,
             @NotNull final SamlPluginSettingsStorage settingsStorage,
-            @NotNull UserModel userModel) {
+            @NotNull UserModel userModel,
+            @NotNull LoginConfiguration loginConfiguration) {
         this.rootUrlHolder = rootUrlHolder;
         this.settingsStorage = settingsStorage;
         this.userModel = userModel;
+        this.loginConfiguration = loginConfiguration;
     }
 
     @NotNull
@@ -262,7 +265,7 @@ public String getCertificateBase64Encoded(X509Certificate cert) throws Certifica
         return "";
     }
 
-    public static boolean isConfigured(LoginConfiguration loginConfiguration) {
+    public boolean isConfigured() {
         boolean authModuleConfigured = loginConfiguration
                 .getConfiguredAuthModules(AuthModuleType.class).stream()
                 .anyMatch(t -> t.getType().getClass().getName().equals(SamlAuthenticationScheme.class.getName()));

diff --git a/...n-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlLoginPageExtension.java b/...n-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlLoginPageExtension.java
@@ -20,23 +20,24 @@ public class SamlLoginPageExtension extends SimplePageExtension {
     private final SamlPluginSettingsStorage settingsStorage;
     private static final String PAGE_EXTENSION_ID = "SamlLogin";
     private LoginConfiguration loginConfiguration;
+    private SamlAuthenticationScheme scheme;
 
     public SamlLoginPageExtension(@NotNull PagePlaces pagePlaces,
                                   @NotNull PluginDescriptor descriptor,
                                   @NotNull final SamlPluginSettingsStorage settingsStorage,
-                                  @NotNull LoginConfiguration loginConfiguration) {
+                                  @NotNull SamlAuthenticationScheme scheme) {
         super(pagePlaces,
                 PlaceId.LOGIN_PAGE,
                 PAGE_EXTENSION_ID,
                 descriptor.getPluginResourcesPath("SamlLogin.jsp"));
-        this.loginConfiguration = loginConfiguration;
+        this.scheme = scheme;
         register();
         this.settingsStorage = settingsStorage;
     }
 
     @Override
     public boolean isAvailable(@NotNull HttpServletRequest request) {
-        return SamlAuthenticationScheme.isConfigured(loginConfiguration);
+        return scheme.isConfigured();
     }
 
     @Override

diff --git a/...-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlPluginConfiguration.java b/...-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlPluginConfiguration.java
@@ -6,6 +6,7 @@
 import jetbrains.buildServer.serverSide.ServerPaths;
 import jetbrains.buildServer.serverSide.auth.LoginConfiguration;
 import jetbrains.buildServer.users.UserModel;
+import jetbrains.buildServer.web.SamlCsrfCheck;
 import jetbrains.buildServer.web.openapi.PagePlaces;
 import jetbrains.buildServer.web.openapi.PluginDescriptor;
 import jetbrains.buildServer.web.openapi.WebControllerManager;
@@ -23,15 +24,15 @@ public class SamlPluginConfiguration {
 
     @Bean
     SamlAuthenticationScheme samlAuthenticationScheme(LoginConfiguration loginConfiguration, SamlPluginSettingsStorage samlPluginSettingsStorage, UserModel userModel, RootUrlHolder rootUrlHolder) {
-        SamlAuthenticationScheme samlAuthenticationScheme = new SamlAuthenticationScheme(rootUrlHolder, samlPluginSettingsStorage, userModel);
+        SamlAuthenticationScheme samlAuthenticationScheme = new SamlAuthenticationScheme(rootUrlHolder, samlPluginSettingsStorage, userModel, loginConfiguration);
         loginConfiguration.registerAuthModuleType(samlAuthenticationScheme);
 
         return samlAuthenticationScheme;
     }
 
     @Bean
-    SamlLoginPageExtension samlLoginPageExtension(@NotNull PagePlaces pagePlaces, @NotNull PluginDescriptor descriptor, SamlPluginSettingsStorage settingsStorage, LoginConfiguration loginConfiguration) {
-        return new SamlLoginPageExtension(pagePlaces, descriptor, settingsStorage, loginConfiguration);
+    SamlLoginPageExtension samlLoginPageExtension(@NotNull PagePlaces pagePlaces, @NotNull PluginDescriptor descriptor, SamlPluginSettingsStorage settingsStorage, SamlAuthenticationScheme scheme) {
+        return new SamlLoginPageExtension(pagePlaces, descriptor, settingsStorage, scheme);
     }
 
     @Bean
@@ -45,8 +46,8 @@ SamlCallbackController samlCallbackController(SBuildServer server, WebController
     }
 
     @Bean
-    SamlSettingsAdminPage samlPluginAdminPage(@NotNull PagePlaces pagePlaces, @NotNull PluginDescriptor descriptor, SamlPluginSettingsStorage settingsStorage, LoginConfiguration loginConfiguration) {
-        return new SamlSettingsAdminPage(pagePlaces, descriptor, settingsStorage, loginConfiguration);
+    SamlSettingsAdminPage samlPluginAdminPage(@NotNull PagePlaces pagePlaces, @NotNull PluginDescriptor descriptor, SamlPluginSettingsStorage settingsStorage, SamlAuthenticationScheme samlAuthenticationScheme) {
+        return new SamlSettingsAdminPage(pagePlaces, descriptor, settingsStorage, samlAuthenticationScheme);
     }
 
     @Bean
@@ -61,4 +62,9 @@ SamlPluginSettingsStorage samlPluginSettingsStorage(ServerPaths serverPaths) thr
     SamlSettingsJsonController samlSettingsAjaxController(WebControllerManager controllerManager, SamlAuthenticationScheme samlAuthenticationScheme) throws IOException {
         return new SamlSettingsJsonController(samlAuthenticationScheme, samlPluginSettingsStorage(null), controllerManager);
     }
+
+    @Bean
+    SamlCsrfCheck samlCsrfCheck(SamlAuthenticationScheme scheme, SamlPluginSettingsStorage settingsStorage) {
+        return new SamlCsrfCheck(scheme, settingsStorage);
+    }
 }
diff --git a/...on-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlSettingsAdminPage.java b/...on-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/SamlSettingsAdminPage.java
@@ -17,18 +17,18 @@
 
 public class SamlSettingsAdminPage extends AdminPage {
     private final PluginDescriptor pluginDescriptor;
-    private LoginConfiguration loginConfiguration;
+    private SamlAuthenticationScheme samlAuthenticationScheme;
     private final Logger LOG = Loggers.SERVER;
     private SamlPluginSettingsStorage settingsStorage;
 
     protected SamlSettingsAdminPage(@NotNull PagePlaces pagePlaces,
                                     @NotNull PluginDescriptor pluginDescriptor,
                                     @NotNull SamlPluginSettingsStorage settingsStorage,
-                                    @NotNull LoginConfiguration loginConfiguration) {
+                                    @NotNull SamlAuthenticationScheme samlAuthenticationScheme) {
         super(pagePlaces);
         this.settingsStorage = settingsStorage;
         this.pluginDescriptor = pluginDescriptor;
-        this.loginConfiguration = loginConfiguration;
+        this.samlAuthenticationScheme = samlAuthenticationScheme;
         setPluginName(SamlPluginConstants.PLUGIN_NAME);
         setIncludeUrl(pluginDescriptor.getPluginResourcesPath("SamlPluginAdminPage.jsp"));
         setTabTitle("SAML Settings");
@@ -59,6 +59,6 @@ public void fillModel(@NotNull Map<String, Object> model, @NotNull HttpServletRe
     public boolean isAvailable(@NotNull HttpServletRequest request) {
         return super.isAvailable(request)
                 && checkHasGlobalPermission(request, Permission.CHANGE_SERVER_SETTINGS)
-                && SamlAuthenticationScheme.isConfigured(loginConfiguration);
+                && samlAuthenticationScheme.isConfigured();
     }
 }
diff --git a/...-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/pojo/SamlPluginSettings.java b/...-server/src/main/java/jetbrains/buildServer/auth/saml/plugin/pojo/SamlPluginSettings.java
@@ -34,6 +34,7 @@ public class SamlPluginSettings {
     private String allowedPostfixes = null;
     private boolean compressRequest = true;
     private boolean strict = true;
+    private boolean samlCorsFilter = true;
 
     SamlAttributeMappingSettings emailAttributeMapping = new SamlAttributeMappingSettings();
     SamlAttributeMappingSettings nameAttributeMapping = new SamlAttributeMappingSettings();

diff --git a/saml-authentication-server/src/main/java/jetbrains/buildServer/web/SamlCsrfCheck.java b/saml-authentication-server/src/main/java/jetbrains/buildServer/web/SamlCsrfCheck.java
@@ -0,0 +1,67 @@
+package jetbrains.buildServer.web;
+
+import jetbrains.buildServer.auth.saml.plugin.SamlAuthenticationScheme;
+import jetbrains.buildServer.auth.saml.plugin.SamlPluginConstants;
+import jetbrains.buildServer.auth.saml.plugin.SamlPluginSettingsStorage;
+import jetbrains.buildServer.log.Loggers;
+import jetbrains.buildServer.serverSide.auth.LoginConfiguration;
+import lombok.var;
+import org.jetbrains.annotations.NotNull;
+import org.springframework.util.StringUtils;
+
+import javax.servlet.http.HttpServletRequest;
+import java.net.URL;
+
+public class SamlCsrfCheck implements CsrfCheck {
+
+    private SamlAuthenticationScheme scheme;
+    private SamlPluginSettingsStorage settingsStorage;
+
+    public SamlCsrfCheck(SamlAuthenticationScheme scheme, SamlPluginSettingsStorage settingsStorage) {
+        this.scheme = scheme;
+        this.settingsStorage = settingsStorage;
+    }
+
+    @Override
+    public CheckResult isSafe(@NotNull HttpServletRequest request) {
+
+        if (!scheme.isConfigured()) return UNKNOWN;
+
+        try {
+            if (!this.settingsStorage.load().isSamlCorsFilter()) {
+                Loggers.AUTH.debug("SAML CORS filter is disabled by plugin configuration - skipping");
+                return UNKNOWN;
+            }
+
+            Loggers.AUTH.debug("Evaluating SAML CORS filter conditions");
+
+            URL callbackUrl = scheme.getCallbackUrl();
+            var requestURL = new URL(request.getRequestURL().toString());
+
+            if (callbackUrl == null) return UNKNOWN;
+
+            if ("POST".equals(request.getMethod()) && callbackUrl.getPath().equals(requestURL.getPath())) {
+                var parameter = request.getParameter(SamlPluginConstants.SAML_RESPONSE_REQUEST_PARAMETER);
+                if (StringUtils.isEmpty(parameter)) {
+                    Loggers.AUTH.debug("SAML CORS Check: " + SamlPluginConstants.SAML_RESPONSE_REQUEST_PARAMETER + " is not found in the request - responding with UNKNOWN result");
+                    return UNKNOWN;
+                }
+
+                Loggers.AUTH.debug("CSRF validated via SAML callback target");
+
+                return CheckResult.safe();
+            }
+
+        } catch (Exception e) {
+            Loggers.AUTH.error(e.getMessage(), e);
+        }
+
+        return UNKNOWN;
+    }
+
+    @NotNull
+    @Override
+    public String describe(boolean b) {
+        return "SAML login requests to SSO callback URL are CORS-safe";
+    }
+}
diff --git a/saml-authentication-server/src/main/vue/admin-ui/src/services/ISettingsApiService.ts b/saml-authentication-server/src/main/vue/admin-ui/src/services/ISettingsApiService.ts
@@ -34,12 +34,15 @@ export interface SamlSettings {
     ssoLoginButtonName?: string;
     strict?: boolean;
 
+    samlCorsFilter?: boolean;
     compressRequest?: boolean;
 
     createUsersAutomatically?: boolean;
     limitToPostfixes?: boolean;
     allowedPostfixes?: string;
 
+
+
     emailAttributeMapping?: SamlAttributeMapping;
     nameAttributeMapping?: SamlAttributeMapping;
 }

diff --git a/saml-authentication-server/src/main/vue/admin-ui/src/views/SamlPluginSettings.vue b/saml-authentication-server/src/main/vue/admin-ui/src/views/SamlPluginSettings.vue
@@ -69,6 +69,11 @@
                 <template v-slot:content><input type="checkbox" v-model="settings.strict"/></template>
                 <template v-slot:note>Runs additional checks on SAML message</template>
             </RunnerFormRow>
+            <RunnerFormRow>
+                <template v-slot:label>SAML Callback CORS Filer Exception</template>
+                <template v-slot:content><input type="checkbox" v-model="settings.samlCorsFilter"/></template>
+                <template v-slot:note>Adds CORS filter exception for POST requests sent to the login callback URL</template>
+            </RunnerFormRow>
             <RunnerFormRow>
                 <template v-slot:label>Hide Login Form</template>
                 <template v-slot:content><input type="checkbox" v-model="settings.hideLoginForm"/></template>

diff --git a/...er/src/test/java/jetbrains/buildServer/auth/saml/plugin/SamlAuthenticationSchemeTest.java b/...er/src/test/java/jetbrains/buildServer/auth/saml/plugin/SamlAuthenticationSchemeTest.java
@@ -5,6 +5,7 @@
 import jetbrains.buildServer.auth.saml.plugin.pojo.SamlAttributeMappingSettings;
 import jetbrains.buildServer.auth.saml.plugin.pojo.SamlPluginSettings;
 import jetbrains.buildServer.controllers.interceptors.auth.HttpAuthenticationResult;
+import jetbrains.buildServer.serverSide.auth.LoginConfiguration;
 import jetbrains.buildServer.users.SUser;
 import jetbrains.buildServer.users.UserModel;
 import lombok.var;
@@ -69,7 +70,9 @@ public void setUp() throws Exception {
 
         this.settingsStorage.settings.setStrict(false);
 
-        this.scheme = new SamlAuthenticationScheme(rootUrlHolder, settingsStorage, userModel);
+        LoginConfiguration loginConfiguration = mock(LoginConfiguration.class);
+
+        this.scheme = new SamlAuthenticationScheme(rootUrlHolder, settingsStorage, userModel, loginConfiguration);
     }
 
     @After

diff --git a/...-server/src/test/java/jetbrains/buildServer/auth/saml/plugin/SamlLoginControllerTest.java b/...-server/src/test/java/jetbrains/buildServer/auth/saml/plugin/SamlLoginControllerTest.java
@@ -3,6 +3,7 @@
 import com.onelogin.saml2.util.Util;
 import jetbrains.buildServer.controllers.AuthorizationInterceptor;
 import jetbrains.buildServer.serverSide.SBuildServer;
+import jetbrains.buildServer.serverSide.auth.LoginConfiguration;
 import jetbrains.buildServer.users.UserModel;
 import jetbrains.buildServer.web.openapi.WebControllerManager;
 import lombok.var;
@@ -48,7 +49,9 @@ public void setUp() throws Exception {
         this.interceptor = mock(AuthorizationInterceptor.class);
         this.userModel = mock(UserModel.class);
 
-        scheme = new SamlAuthenticationScheme(server, settingsStorage, userModel);
+        LoginConfiguration loginConfiguration = mock(LoginConfiguration.class);
+
+        scheme = new SamlAuthenticationScheme(server, settingsStorage, userModel, loginConfiguration);
         controller = new SamlLoginController(this.server, this.webControllerManager, this.interceptor, this.scheme, this.settingsStorage);
     }
 

diff --git a/.../src/test/java/jetbrains/buildServer/auth/saml/plugin/SamlSettingsJsonControllerTest.java b/.../src/test/java/jetbrains/buildServer/auth/saml/plugin/SamlSettingsJsonControllerTest.java
@@ -3,6 +3,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import jetbrains.buildServer.RootUrlHolder;
 import jetbrains.buildServer.auth.saml.plugin.pojo.MetadataImport;
+import jetbrains.buildServer.serverSide.auth.LoginConfiguration;
 import jetbrains.buildServer.users.UserModel;
 import jetbrains.buildServer.web.openapi.WebControllerManager;
 import lombok.var;
@@ -45,7 +46,8 @@ public void setUp() throws Exception {
 
         this.userModel = mock(UserModel.class);
 
-        samlAuthenticationScheme = new SamlAuthenticationScheme(rootUrlHolder, settingsStorage, userModel);
+        LoginConfiguration loginConfiguration = mock(LoginConfiguration.class);
+        samlAuthenticationScheme = new SamlAuthenticationScheme(rootUrlHolder, settingsStorage, userModel, loginConfiguration);
 
         controller = new SamlSettingsJsonController(this.samlAuthenticationScheme, this.settingsStorage, this.webControllerManager);
     }