8000 Updating SAML defualt signature validation securely when settings are… by PadzerButch · Pull Request #54766 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Updating SAML defualt signature validation securely when settings are… #54766

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

Draft
PadzerButch wants to merge 1 commit into
base: main
Choose a base branch
from
+999 −46
Draft

Updating SAML defualt signature validation securely when settings are… #54766

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
7 changes: 6 additions & 1 deletion js/apps/admin-ui/src/identity-providers/add/DescriptorSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ const Fields = ({ readOnly }: DescriptorSettingsProps) => {
name: "config.validateSignature",
});

const wantAssertionsSigned = useWatch({
control,
name: "config.wantAssertionsSigned",
});

const useMetadataDescriptorUrl = useWatch({
control,
name: "config.useMetadataDescriptorUrl",
Expand Down Expand Up @@ -279,7 +284,7 @@ const Fields = ({ readOnly }: DescriptorSettingsProps) => {
isDisabled={readOnly}
stringify
/>
{validateSignature === "true" && (
10BC8 {(validateSignature === "true" || wantAssertionsSigned === "true") && (
<>
<TextControl
name="config.metadataDescriptorUrl"
Expand Down
41 changes: 27 additions & 14 deletions js/apps/admin-ui/src/identity-providers/add/DetailSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ const Header = ({ onChange, value, save, toggleDeleteDialog }: HeaderProps) => {
name: "config.validateSignature",
});

const wantAssertionsSigned = useWatch({
control,
name: "config.wantAssertionsSigned",
});

const useMetadataDescriptorUrl = useWatch({
control,
name: "config.useMetadataDescriptorUrl",
Expand Down Expand Up @@ -198,7 +203,7 @@ const Header = ({ onChange, value, save, toggleDeleteDialog }: HeaderProps) => {
divider={false}
dropdownItems={[
...(provider?.providerId?.includes("saml") &&
validateSignature === "true" &&
(validateSignature === "true" || wantAssertionsSigned === "true") &&
useMetadataDescriptorUrl === "true" &&
metadataDescriptorUrl &&
!formState.isDirty &&
Expand All @@ -212,7 +217,8 @@ const Header = ({ onChange, value, save, toggleDeleteDialog }: HeaderProps) => {
</DropdownItem>,
]
: provider?.providerId?.includes("saml") &&
validateSignature === "true" &&
(validateSignature === "true" ||
wantAssertionsSigned === "true") &&
useMetadataDescriptorUrl !== "true" &&
metadataDescriptorUrl &&
!formState.isDirty
Expand Down Expand Up @@ -289,18 +295,25 @@ export default function DetailSettings() {

const toFormValues = (
p: IdentityProviderRepresentation,
): IdentityProviderRepresentation =>
p.config
? {
...p,
config: Object.fromEntries(
Object.entries(p.config).map(([key, value]) => [
beerify(key),
value,
]),
),
}
: p;
): IdentityProviderRepresentation => {
if (!p.config) return p;

const transformedConfig = Object.fromEntries(
Object.entries(p.config).map(([key, value]) => [beerify(key), value]),
);

if (p.providerId?.includes("saml")) {
// Header (outside the Controller) reads these to show Reload/Import keys,
// and the server treats an absent value as true.
if (transformedConfig.validateSignature === undefined) {
transformedConfig.validateSignature = "true";
}
if (transformedConfig.wantAssertionsSigned === undefined) {
transformedConfig.wantAssertionsSigned = "true";
}
}
return { ...p, config: transformedConfig };
};

const [selectedMapper, setSelectedMapper] =
useState<IdPWithMapperAttributes>();
Expand Down
241 changes: 238 additions & 3 deletions js/apps/admin-ui/test/identity-providers/saml-signature-defaults.spec.ts
View file
Open in desktop
10BC8
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import { expect, test } from "@playwright/test";
import { type Page, expect, test } from "@playwright/test";
import { v4 as uuid } from "uuid";
import adminClient from "../utils/AdminClient.ts";
import { login } from "../utils/login.ts";
import { SERVER_URL } from "../utils/constants.ts";
import { goToIdentityProviders } from "../utils/sidebar.ts";
import { clickTableRowItem } from "../utils/table.ts";

const addSamlProviderUrl = `${SERVER_URL}/admin/master/console/#/master/identity-providers/saml/add`;
const realmName = `idp-saml-defaults-${uuid()}`;
const addSamlProviderUrl = `${SERVER_URL}/admin/master/console/#/${realmName}/identity-providers/saml/add`;
const samlDescriptorUrl = `${SERVER_URL}/realms/${realmName}/protocol/saml/descriptor`;

test.beforeAll(() => adminClient.createRealm(realmName));
test.afterAll(() => adminClient.deleteRealm(realmName));

test("should enable SAML signature switches by default", async ({ page }) => {
await login(page);
await login(page, { to: { pathname: `/${realmName}/identity-providers` } });
await page.goto(addSamlProviderUrl);

const wantAssertionsSigned = page.getByTestId("config.wantAssertionsSigned");
Expand All @@ -16,3 +25,229 @@ test("should enable SAML signature switches by default", async ({ page }) => {
await validateSignature.waitFor({ state: "attached" });
await expect(validateSignature).toBeChecked();
});

test.describe("SAML provider with missing signature config", () => {
const samlProviderName = "SAML v2.0";
const alias = `saml-missing-sig-config-${uuid()}`;

test.afterAll(async () => {
if (await adminClient.getIdentityProvider(alias, realmName)) {
await adminClient.deleteIdentityProvider(alias, realmName);
}
});

test("should show key controls when signature fields are absent in backend", async ({
page,
}) => {
await adminClient.createIdentityProvider(
samlProviderName,
alias,
realmName,
);

await login(page, { to: { pathname: `/${realmName}/identity-providers` } });
await goToIdentityProviders(page);
await clickTableRowItem(page, samlProviderName);

const wantAssertionsSigned = page.getByTestId(
"config.wantAssertionsSigned",
);
await expect(wantAssertionsSigned).toBeChecked();

const validateSignature = page.getByTestId("config.validateSignature");
await expect(validateSignature).toBeChecked();

const signingCertificate = page.getByTestId("config.signingCertificate");
await expect(signingCertificate).toBeVisible();

const metadataDescriptorUrl = page.getByTestId(
"config.metadataDescriptorUrl",
);
await expect(metadataDescriptorUrl).toBeVisible();
});
});

test.describe("SAML key management actions visibility", () => {
const samlProviderName = "SAML v2.0";

test.describe("Reload action", () => {
const alias = `saml-reload-keys-${uuid()}`;

test.afterAll(async () => {
if (await adminClient.getIdentityProvider(alias, realmName)) {
await adminClient.deleteIdentityProvider(alias, realmName);
}
});

test("should show Reload keys action when useMetadataDescriptorUrl is true", async ({
page,
}) => {
await adminClient.createIdentityProvider(
samlProviderName,
alias,
realmName,
{
useMetadataDescriptorUrl: "true",
metadataDescriptorUrl: samlDescriptorUrl,
},
);

await login(page, {
to: { pathname: `/${realmName}/identity-providers` },
});
await goToIdentityProviders(page);
await clickTableRowItem(page, samlProviderName);

await page.getByTestId("action-dropdown").click();

await expect(page.getByText("Reload keys")).toBeVisible();

await expect(page.getByText("Import keys")).toBeHidden();
});
});

test.describe("Import action", () => {
const alias = `saml-import-keys-${uuid()}`;

test.afterAll(async () => {
if (await adminClient.getIdentityProvider(alias, realmName)) {
await adminClient.deleteIdentityProvider(alias, realmName);
}
});

test("should show Import keys action when useMetadataDescriptorUrl is not true", async ({
page,
}) => {
await adminClient.createIdentityProvider(
samlProviderName,
alias,
realmName,
{
metadataDescriptorUrl: samlDescriptorUrl,
},
);

await login(page, {
to: { pathname: `/${realmName}/identity-providers` },
});
await goToIdentityProviders(page);
await clickTableRowItem(page, samlProviderName);

await page.getByTestId("action-dropdown").click();

await expect(page.getByText("Import keys")).toBeVisible();

await expect(page.getByText("Reload keys")).toBeHidden();
});
});
});

test.describe("SAML provider with explicit signature config", () => {
const samlProviderName = "SAML v2.0";

const createProvider = async (
alias: string,
validateSignature: string,
wantAssertionsSigned: string,
) => {
await adminClient.createIdentityProvider(
samlProviderName,
alias,
realmName,
{
validateSignature,
wantAssertionsSigned,
metadataDescriptorUrl: samlDescriptorUrl,
},
);
};

const openProvider = async (page: Page) => {
await login(page, { to: { pathname: `/${realmName}/identity-providers` } });
await goToIdentityProviders(page);
await clickTableRowItem(page, samlProviderName);
await page.getByTestId("config.validateSignature").waitFor({
state: "attached",
});
};

const cleanUp = async (alias: string) => {
if (await adminClient.getIdentityProvider(alias, realmName)) {
await adminClient.deleteIdentityProvider(alias, realmName);
}
};

test.describe("validateSignature=false and wantAssertionsSigned=true", () => {
const alias = `saml-explicit-sig-${uuid()}`;

test.afterAll(() => cleanUp(alias));

test("should show the signing key controls", async ({ page }) => {
await createProvider(alias, "false", "true");
await openProvider(page);

await expect(
page.getByTestId("config.validateSignature"),
).not.toBeChecked();
await expect(
page.getByTestId("config.wantAssertionsSigned"),
).toBeChecked();

// Assertion verification needs keys even when the response is not validated
await expect(page.getByTestId("config.signingCertificate")).toBeVisible();
await expect(
page.getByTestId("config.metadataDescriptorUrl"),
).toBeVisible();
await page.getByTestId("action-dropdown").click();
await expect(page.getByText("Import keys")).toBeVisible();
});
});

test.describe("validateSignature=true and wantAssertionsSigned=false", () => {
const alias = `saml-explicit-sig-${uuid()}`;

test.afterAll(() => cleanUp(alias));

test("should show the signing key controls", async ({ page }) => {
await createProvider(alias, "true", "false");
await openProvider(page);

await expect(page.getByTestId("config.validateSignature")).toBeChecked();
await expect(
page.getByTestId("config.wantAssertionsSigned"),
).not.toBeChecked();

await expect(page.getByTestId("config.signingCertificate")).toBeVisible();
await expect(
page.getByTestId("config.metadataDescriptorUrl"),
).toBeVisible();
await page.getByTestId("action-dropdown").click();
await expect(page.getByText("Import keys")).toBeVisible();
});
});

test.describe("validateSignature=false and wantAssertionsSigned=false", () => {
const alias = `saml-explicit-sig-${uuid()}`;

test.afterAll(() => cleanUp(alias));

test("should hide the signing key controls", async ({ page }) => {
await createProvider(alias, "false", "false");
await openProvider(page);

await expect(
page.getByTestId("config.validateSignature"),
).not.toBeChecked();
await expect(
page.getByTestId("config.wantAssertionsSigned"),
).not.toBeChecked();

await expect(page.getByTestId("config.signingCertificate")).toBeHidden();
await expect(
page.getByTestId("config.metadataDescriptorUrl"),
).toBeHidden();
await page.getByTestId("action-dropdown").click();
await expect(page.getByText("Import keys")).toBeHidden();
});
});
});
11 changes: 11 additions & 0 deletions js/apps/admin-ui/test/utils/AdminClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,17 @@ class AdminClient {
);
}

async getIdentityProvider(
idpAlias: string,
realm: string = this.#client.realmName,
) {
await this.#login();
return await this.#client.identityProviders.findOne({
alias: idpAlias,
realm,
});
}

async deleteIdentityProvider(
idpAlias: string,
realm: string = this.#client.realmName,
Expand Down
2 changes: 1 addition & 1 deletion services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ protected Response handleLoginResponse(String samlResponse, SAMLDocumentHolder h

boolean signed = AssertionUtil.isSignedElement(assertionElement);
final boolean assertionSignatureNotExistsWhenRequired = config.isWantAssertionsSigned() && !signed;
final boolean signatureNotValid = signed && config.isValidateSignature() && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator());
final boolean signatureNotValid = signed && (config.isValidateSignature() || config.isWantAssertionsSigned()) && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator());
final boolean hasNoSignatureWhenRequired = !signed && config.isValidateSignature() && !isMessageFullySigned(holder);

if (assertionSignatureNotExistsWhenRequired || signatureNotValid || hasNoSignatureWhenRequired) {
Expand Down
4 changes: 2 additions & 2 deletions services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public void setSingleLogoutServiceUrl(String singleLogoutServiceUrl) {
}

public boolean isValidateSignature() {
return Boolean.valueOf(getConfig().get(VALIDATE_SIGNATURE));
return Boolean.valueOf(getConfig().getOrDefault(VALIDATE_SIGNATURE, "true"));
}

public void setValidateSignature(boolean validateSignature) {
Expand Down Expand Up @@ -189,7 +189,7 @@ public void setWantAuthnRequestsSigned(boolean wantAuthnRequestsSigned) {
}

public boolean isWantAssertionsSigned() {
return Boolean.valueOf(getConfig().get(WANT_ASSERTIONS_SIGNED));
return Boolean.valueOf(getConfig().getOrDefault(WANT_ASSERTIONS_SIGNED, "true"));
}

public void setWantAssertionsSigned(boolean wantAssertionsSigned) {
Expand Down
2 changes: 1 addition & 1 deletion services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public Map<String, String> parseConfig(KeycloakSession session, String config) {
samlIdentityProviderConfig.setSingleSignOnServiceUrl(singleSignOnServiceUrl);
samlIdentityProviderConfig.setWantAuthnRequestsSigned(idpDescriptor.isWantAuthnRequestsSigned());
samlIdentityProviderConfig.setAddExtensionsElementWithKeyInfo(false);
samlIdentityProviderConfig.setValidateSignature(idpDescriptor.isWantAuthnRequestsSigned());
samlIdentityProviderConfig.setValidateSignature(true);
samlIdentityProviderConfig.setPostBindingResponse(postBindingResponse);
samlIdentityProviderConfig.setArtifactBindingResponse(artifactBindingResponse);
samlIdentityProviderConfig.setPostBindingAuthnRequest(postBindingResponse);
Expand Down
Loading
Loading
0