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.

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

Add optional setting to set a ceiling on how old a SAML response is allowed to be #577

Merged
merged 10 commits into from Apr 28, 2021
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -128,6 +128,7 @@ type Profile = {
- `identifierFormat`: optional name identifier format to request from identity provider (default: `urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress`)
- `wantAssertionsSigned`: if truthy, add `WantAssertionsSigned="true"` to the metadata, to specify that the IdP should always sign the assertions.
- `acceptedClockSkewMs`: Time in milliseconds of skew that is acceptable between client and server when checking `OnBefore` and `NotOnOrAfter` assertion condition validity timestamps. Setting to `-1` will disable checking these conditions entirely. Default is `0`.
- `maxAssertionAgeMs`: Amount of time after which the framework should consider an assertion expired. If the limit imposed by this variable is stricter than the limit imposed by `NotOnOrAfter`, this limit will be used when determining if an assertion is expired.
- `attributeConsumingServiceIndex`: optional `AttributeConsumingServiceIndex` attribute to add to AuthnRequest to instruct the IDP which attribute set to attach to the response ([link](http://blog.aniljohn.com/2014/01/data-minimization-front-channel-saml-attribute-requests.html))
- `disableRequestedAuthnContext`: if truthy, do not request a specific authentication context. This is [known to help when authenticating against Active Directory](https://github.com/node-saml/passport-saml/issues/226) (AD FS) servers.
- `authnContext`: if truthy, name identifier format to request auth context (default: `urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport`); array of values is also supported
Expand Down
77 changes: 71 additions & 6 deletions src/node-saml/saml.ts
Expand Up @@ -140,6 +140,7 @@ class SAML {
skipRequestCompression: ctorOptions.skipRequestCompression ?? false,
disableRequestAcsUrl: ctorOptions.disableRequestAcsUrl ?? false,
acceptedClockSkewMs: ctorOptions.acceptedClockSkewMs ?? 0,
maxAssertionAgeMs: ctorOptions.maxAssertionAgeMs ?? 0,
path: ctorOptions.path ?? "/saml/consume",
host: ctorOptions.host ?? "localhost",
issuer: ctorOptions.issuer ?? "onelogin_saml",
Expand Down Expand Up @@ -1075,11 +1076,17 @@ class SAML {
if (confirmData && confirmData.$) {
const subjectNotBefore = confirmData.$.NotBefore;
const subjectNotOnOrAfter = confirmData.$.NotOnOrAfter;
const maxTimeLimitMs = this.processMaxAgeAssertionTime(
this.options.maxAssertionAgeMs,
subjectNotOnOrAfter,
assertion.$.IssueInstant
);

const subjErr = this.checkTimestampsValidityError(
nowMs,
subjectNotBefore,
subjectNotOnOrAfter
subjectNotOnOrAfter,
maxTimeLimitMs
);
if (subjErr) {
throw subjErr;
Expand Down Expand Up @@ -1126,10 +1133,16 @@ class SAML {
throw new Error(msg);
}
if (conditions && conditions.$) {
const maxTimeLimitMs = this.processMaxAgeAssertionTime(
this.options.maxAssertionAgeMs,
conditions.$.NotOnOrAfter,
assertion.$.IssueInstant
);
const conErr = this.checkTimestampsValidityError(
nowMs,
conditions.$.NotBefore,
conditions.$.NotOnOrAfter
conditions.$.NotOnOrAfter,
maxTimeLimitMs
);
if (conErr) throw conErr;
}
Expand Down Expand Up @@ -1190,18 +1203,27 @@ class SAML {
return { profile, loggedOut: false };
}

private checkTimestampsValidityError(nowMs: number, notBefore: string, notOnOrAfter: string) {
private checkTimestampsValidityError(
nowMs: number,
notBefore: string,
notOnOrAfter: string,
maxTimeLimitMs?: number
) {
if (this.options.acceptedClockSkewMs == -1) return null;

if (notBefore) {
const notBeforeMs = Date.parse(notBefore);
const notBeforeMs = this.dateStringToTimestamp(notBefore, "NotBefore");
if (nowMs + this.options.acceptedClockSkewMs < notBeforeMs)
return new Error("SAML assertion not yet valid");
}
if (notOnOrAfter) {
const notOnOrAfterMs = Date.parse(notOnOrAfter);
const notOnOrAfterMs = this.dateStringToTimestamp(notOnOrAfter, "NotOnOrAfter");
if (nowMs - this.options.acceptedClockSkewMs >= notOnOrAfterMs)
return new Error("SAML assertion expired");
return new Error("SAML assertion expired: clocks skewed too much");
}
if (maxTimeLimitMs) {
if (nowMs - this.options.acceptedClockSkewMs >= maxTimeLimitMs)
cjbarth marked this conversation as resolved.
Show resolved Hide resolved
return new Error("SAML assertion expired: assertion too old");
}

return null;
Expand Down Expand Up @@ -1404,6 +1426,49 @@ class SAML {

throw new Error("Invalid key");
}

/**
* Process max age assertion and use it if it is more restrictive than the NotOnOrAfter age
* assertion received in the SAMLResponse.
*
* @param maxAssertionAgeMs Max time after IssueInstant that we will accept assertion, in Ms.
* @param notOnOrAfter Expiration provided in response.
* @param issueInstant Time when response was issued.
* @returns {*} The expiration time to be used, in Ms.
*/
private processMaxAgeAssertionTime(
maxAssertionAgeMs: number,
notOnOrAfter: string,
issueInstant: string
): number {
const notOnOrAfterMs = this.dateStringToTimestamp(notOnOrAfter, "NotOnOrAfter");
const issueInstantMs = this.dateStringToTimestamp(issueInstant, "IssueInstant");

if (maxAssertionAgeMs === 0) {
return notOnOrAfterMs;
}

const maxAssertionTimeMs = issueInstantMs + maxAssertionAgeMs;
return maxAssertionTimeMs < notOnOrAfterMs ? maxAssertionTimeMs : notOnOrAfterMs;
}

/**
* Convert a date string to a timestamp (in milliseconds).
*
* @param dateString A string representation of a date
* @param label Descriptive name of the date being passed in, e.g. "NotOnOrAfter"
* @throws Will throw an error if parsing `dateString` returns `NaN`
* @returns {number} The timestamp (in milliseconds) representation of the given date
*/
private dateStringToTimestamp(dateString: string, label: string): number {
cjbarth marked this conversation as resolved.
Show resolved Hide resolved
const dateMs = Date.parse(dateString);

if (isNaN(dateMs)) {
throw new Error(`Error parsing ${label}: '${dateString}' is not a valid date`);
}

return dateMs;
}
}

export { SAML };
1 change: 1 addition & 0 deletions src/node-saml/types.ts
Expand Up @@ -109,6 +109,7 @@ export interface SamlOptions extends Partial<SamlSigningOptions>, MandatorySamlO
audience?: string;
scoping?: SamlScopingConfig;
wantAssertionsSigned?: boolean;
maxAssertionAgeMs: number;
cjbarth marked this conversation as resolved.
Show resolved Hide resolved

// InResponseTo Validation
validateInResponseTo: boolean;
Expand Down
8 changes: 4 additions & 4 deletions test/node-saml/test-signatures.spec.ts
Expand Up @@ -25,7 +25,7 @@ describe("Signatures", function () {

//== Run the test in `func`
await assert.rejects(samlObj.validatePostResponseAsync(samlResponseBody), {
message: shouldErrorWith || "SAML assertion expired",
message: shouldErrorWith || "SAML assertion expired: clocks skewed too much",
});
//== Assert times `validateSignature` was called
validateSignatureSpy.callCount.should.eql(amountOfSignatureChecks);
Expand Down Expand Up @@ -210,15 +210,15 @@ describe("Signatures", function () {
describe("Signatures on saml:Response - 1 saml:Assertion + 1 saml:Advice containing 2 saml:Assertion", () => {
//== VALID
it(
"R1A2Ad - signed root+asrt+advi => error",
"R1A2Ad - signed root+asrt+advi => valid",
cjbarth marked this conversation as resolved.
Show resolved Hide resolved
testOneResponse("/valid/response.root-signed.assertion-signed.2advice-signed.xml", false, 1)
);
it(
"R1A2Ad - signed root+asrt => error",
"R1A2Ad - signed root+asrt => valid",
testOneResponse("/valid/response.root-signed.assertion-signed.2advice-unsigned.xml", false, 1)
);
it(
"R1A2Ad - signed root => error",
"R1A2Ad - signed root => valid",
testOneResponse(
"/valid/response.root-signed.assertion-unsigned.2advice-unsigned.xml",
false,
Expand Down