Better management of the CSP header

Closes #24568 Signed-off-by: rmartinc <rmartinc@redhat.com> (cherry picked from commit 2b769e5)
keycloak · Apr 18, 2024 · 047e804 · 047e804
1 parent f911967
commit 047e804
Show file tree

Hide file tree

Showing 8 changed files with 304 additions and 125 deletions.
diff --git a/server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java b/server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java
@@ -1,48 +1,129 @@
+/*
+ * Copyright 2023 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 package org.keycloak.models;
 
+import java.util.LinkedHashMap;
+import java.util.Map;
+
 public class ContentSecurityPolicyBuilder {
 
-    private String frameSrc = "'self'";
-    private String frameAncestors = "'self'";
-    private String objectSrc = "'none'";
+    // constants for directive names used in the class
+    public static final String DIRECTIVE_NAME_FRAME_SRC = "frame-src";
+    public static final String DIRECTIVE_NAME_FRAME_ANCESTORS = "frame-ancestors";
+    public static final String DIRECTIVE_NAME_OBJECT_SRC = "object-src";
+
+    // constants for specific directive value keywords
+    public static final String DIRECTIVE_VALUE_SELF = "'self'";
+    public static final String DIRECTIVE_VALUE_NONE = "'none'";
 
-    private boolean first;
-    private StringBuilder sb;
+    private final Map<String, String> directives = new LinkedHashMap<>();
 
     public static ContentSecurityPolicyBuilder create() {
-        return new ContentSecurityPolicyBuilder();
+        return new ContentSecurityPolicyBuilder()
+                .add(DIRECTIVE_NAME_FRAME_SRC, DIRECTIVE_VALUE_SELF)
+                .add(DIRECTIVE_NAME_FRAME_ANCESTORS, DIRECTIVE_VALUE_SELF)
+                .add(DIRECTIVE_NAME_OBJECT_SRC, DIRECTIVE_VALUE_NONE);
+    }
+
+    public static ContentSecurityPolicyBuilder create(String directives) {
+        return new ContentSecurityPolicyBuilder().parse(directives);
     }
 
     public ContentSecurityPolicyBuilder frameSrc(String frameSrc) {
-        this.frameSrc = frameSrc;
+        if (frameSrc == null) {
+            directives.remove(DIRECTIVE_NAME_FRAME_SRC);
+        } else {
+            put(DIRECTIVE_NAME_FRAME_SRC, frameSrc);
+        }
         return this;
     }
 
+    public ContentSecurityPolicyBuilder addFrameSrc(String frameSrc) {
+        return add(DIRECTIVE_NAME_FRAME_SRC, frameSrc);
+    }
+
+    public boolean isDefaultFrameAncestors() {
+        return DIRECTIVE_VALUE_SELF.equals(directives.get(DIRECTIVE_NAME_FRAME_ANCESTORS));
+    }
+
     public ContentSecurityPolicyBuilder frameAncestors(String frameancestors) {
-        this.frameAncestors = frameancestors;
+        if (frameancestors == null) {
+            directives.remove(DIRECTIVE_NAME_FRAME_ANCESTORS);
+        } else {
+            put(DIRECTIVE_NAME_FRAME_ANCESTORS, frameancestors);
+        }
         return this;
     }
 
-    public String build() {
-        sb = new StringBuilder();
-        first = true;
-
-        build("frame-src", frameSrc);
-        build("frame-ancestors", frameAncestors);
-        build("object-src", objectSrc);
+    public ContentSecurityPolicyBuilder addFrameAncestors(String frameancestors) {
+        return add(DIRECTIVE_NAME_FRAME_ANCESTORS, frameancestors);
+    }
 
+    public String build() {
+        StringBuilder sb = new StringBuilder();
+        if (!directives.isEmpty()) {
+            for (Map.Entry<String, String> entry : directives.entrySet()) {
+                sb.append(entry.getKey());
+                if (!entry.getValue().isEmpty()) {
+                    sb.append(" ").append(entry.getValue());
+                }
+                sb.append("; ");
+            }
+            sb.setLength(sb.length() - 1);
+        }
         return sb.toString();
     }
 
-    private void build(String k, String v) {
-        if (v != null) {
-            if (!first) {
-                sb.append(" ");
-            }
-            first = false;
+    private ContentSecurityPolicyBuilder put(String name, String value) {
+        if (name != null && value != null) {
+            directives.put(name, value);
+        }
+        return this;
+    }
 
-            sb.append(k).append(" ").append(v).append(";");
+    private ContentSecurityPolicyBuilder add(String name, String value) {
+        if (name != null && value != null) {
+            String current = directives.get(name);
+            if (current != null && !current.isEmpty()) {
+                value = current + " " + value;
+            }
+            directives.put(name, value);
         }
+        return this;
     }
 
+    // W3C Working Draft: https://www.w3.org/TR/CSP/
+    // Only managing spaces not the other whitespaces defined in the spec
+    private ContentSecurityPolicyBuilder parse(String value) {
+        if (value == null) {
+            return this;
+        }
+        String[] values = value.split(";");
+        if (values != null) {
+            for (String directive : values) {
+                directive = directive.trim();
+                int idx = directive.indexOf(' ');
+                if (idx > 0) {
+                    add(directive.substring(0, idx), directive.substring(idx + 1, directive.length()).trim());
+                } else if (!directive.isEmpty()) {
+                    add(directive, "");
+                }
+            }
+        }
+        return this;
+    }
 }
diff --git a/server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java b/server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java
@@ -24,6 +24,24 @@ public void contentSecurityPolicyBuilderTest() {
         assertEquals("frame-ancestors 'self'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameSrc(null).build());
         assertEquals("frame-src 'self'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameAncestors(null).build());
         assertEquals("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameSrc("'custom-frame-src'").frameAncestors("'custom-frame-ancestors'").build());
+        assertEquals("frame-src localhost; frame-ancestors 'self'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameSrc("localhost").build());
+        assertEquals("frame-src 'self' localhost; frame-ancestors 'self'; object-src 'none';",
+                ContentSecurityPolicyBuilder.create().addFrameSrc("localhost").build());
+    }
+
+    private void assertParsedDirectives(String directives) {
+        assertEquals(directives, ContentSecurityPolicyBuilder.create(directives).build());
+    }
+
+    @Test
+    public void parseSecurityPolicyBuilderTest() {
+        assertParsedDirectives("frame-src 'self'; frame-ancestors 'self'; object-src 'none';");
+        assertParsedDirectives("frame-ancestors 'self'; object-src 'none';");
+        assertParsedDirectives("frame-src 'self'; object-src 'none';");
+        assertParsedDirectives("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none';");
+        assertParsedDirectives("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none'; style-src 'self';");
+        assertParsedDirectives("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none'; sandbox;");
+        assertEquals("frame-src 'custom-frame-src'; sandbox;", ContentSecurityPolicyBuilder.create("frame-src   'custom-frame-src' ; sandbox ;  ").build());
     }
 
     @Test

diff --git a/services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java b/services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java
@@ -106,22 +106,24 @@ private void addHtmlHeaders(MultivaluedMap<String, Object> headers) {
 
         // TODO This will be refactored as part of introducing a more strict CSP header
         if (options != null) {
-            ContentSecurityPolicyBuilder csp = ContentSecurityPolicyBuilder.create();
+            ContentSecurityPolicyBuilder csp = ContentSecurityPolicyBuilder.create(
+                    headers.getFirst(CONTENT_SECURITY_POLICY.getHeaderName()).toString());
 
             if (options.isAllowAnyFrameAncestor()) {
                 headers.remove(BrowserSecurityHeaders.X_FRAME_OPTIONS.getHeaderName());
 
-                csp.frameAncestors(null);
+                if (csp.isDefaultFrameAncestors()) {
+                    // only remove frame ancestors if defined to default 'self'
+                    csp.frameAncestors(null);
+                }
             }
 
             String allowedFrameSrc = options.getAllowedFrameSrc();
             if (allowedFrameSrc != null) {
-                csp.frameSrc(allowedFrameSrc);
+                csp.addFrameSrc(allowedFrameSrc);
             }
 
-            if (CONTENT_SECURITY_POLICY.getDefaultValue().equals(headers.getFirst(CONTENT_SECURITY_POLICY.getHeaderName()))) {
-                headers.putSingle(CONTENT_SECURITY_POLICY.getHeaderName(), csp.build());
-            }
+            headers.putSingle(CONTENT_SECURITY_POLICY.getHeaderName(), csp.build());
         }
     }
 

diff --git a/services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java b/services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java
@@ -67,7 +67,6 @@ private void configureCSP() {
             allowFrameSrc.append(client.frontChannelLogoutUrl.getAuthority()).append(' ');
         }
 
-        session.getProvider(SecurityHeadersProvider.class).options().allowAnyFrameAncestor();
         session.getProvider(SecurityHeadersProvider.class).options().allowFrameSrc(allowFrameSrc.toString());
     }
 

diff --git a/...lian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java b/...lian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java
@@ -68,6 +68,11 @@ public ClientAttributeUpdater setClientId(String clientId) {
         return this;
     }
 
+    public ClientAttributeUpdater setName(String name) {
+        this.rep.setName(name);
+        return this;
+    }
+
     public ClientAttributeUpdater setAttribute(String name, String value) {
         this.rep.getAttributes().put(name, value);
         if (value != null && !this.origRep.getAttributes().containsKey(name)) {

diff --git a/...llian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java b/...llian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java
@@ -169,4 +169,9 @@ public RealmAttributeUpdater setSmtpServer(String name, String value) {
         rep.getSmtpServer().put(name, value);
         return this;
     }
+
+    public RealmAttributeUpdater setBrowserSecurityHeader(String name, String value) {
+        rep.getBrowserSecurityHeaders().put(name, value);
+        return this;
+    }
 }