Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better management of the CSP header #24577

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Better management of the CSP header #24577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 103 additions & 22 deletions server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
}
18 changes: 18 additions & 0 deletions server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 8 additions & 6 deletions services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmartinc Can you elaborate on why this is the case? Isn't the intention of options.isAllowAnyFrameAncestor() so that we can allow some resources to be loaded under any third-party domains? If that is the case, then we should remove frame ancestors regardless whether it is defined to be default or not, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of this part is to just force the frame-ancestors if it's not modified by the admins. If admins are setting some default ancestors for example self https://*.example.com I suppose the admins know what they are doing and limiting to those ancestors on purpose (because they know the clients are always from those URLs or similar). But not writing in stone, if you have a better idea just let me know. This part is only used to get the IFrame for login.

Copy link
Contributor

@jichen-amplify jichen-amplify Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that currently there are three resources that are using the options.isAllowAnyFrameAncestor() feature to remove frame ancestors, so that they can be loaded in any iFrames:

It seems to me that it is safe to allow these resources to be hosted in any iFrames all the time. That is why I am thinking we should continue to treat these resources differently from the other pages (such as the actual login pages), even when the admins specifically are limiting the frame ancestors on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all those resources are used by the keycloak-js client when the checkLoginIframe is default true. So, just used for the keycloak-js client login. I'm OK to always remove ancestors but then we should take in mind that this cannot be overridden.

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());
}
}

Expand Down
1 change: 0 additions & 1 deletion services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
5 changes: 5 additions & 0 deletions ...lian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
5 changes: 5 additions & 0 deletions ...llian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,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;
}
}
Loading
Loading