From 36b4b7534e9859adb68b345f1ed4af7347894ab9 Mon Sep 17 00:00:00 2001 From: Ivan Date: Fri, 3 Nov 2023 21:13:45 +0000 Subject: [PATCH] Add support for clock skew tolerance between SP and IdP. Introduced a new configuration variable 'saml_sp_clock_skew' to define the maximum allowed time difference between NGINX and Identity Provider clocks. The default value is set to 120 seconds to accommodate minor discrepancies in system times. --- README.md | 6 +++- saml_sp.js | 88 ++++++++++++++++++++++++++++++++++------------------- t/js_saml.t | 33 +++++++++++++++++--- 3 files changed, 90 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 8e4eace..cc8413e 100644 --- a/README.md +++ b/README.md @@ -190,7 +190,10 @@ The Assertion is validated using the same approach as the Response, with the exc The `Subject` element specifies the principle that is the subject of the statements in the assertion. It must contain a `NameID` element, which represents the authenticated user. The `NameID` is a unique identifier for the user within the context of the Identity Provider, while the `NameID Format` describes the format or namespace of the `NameID`. When processing the Subject, NGINX Plus parses both the NameID and the NameID Format, which are then stored in the `$saml_name_id` and `$saml_name_id_format` variables, respectively. ### Conditions -The `Conditions` element defines the conditions under which the SAML Assertion is considered valid. It is a mandatory element, and an assertion without it will be deemed invalid. NGINX Plus checks the values of the `NotBefore` and `NotOnOrAfter` attributes to ensure the assertion is being used within the specified time window. It does not account for any time difference between itself and the Identity Provider nor does it add any buffer to these time values. +The `Conditions` element defines the conditions under which the SAML Assertion is considered valid. It is a mandatory element, and an assertion without it will be deemed invalid. NGINX Plus checks the values of the `NotBefore` and `NotOnOrAfter` attributes to ensure the assertion is being used within the specified time window. + +NGINX Plus accommodates potential clock discrepancies between the Service Provider and the Identity Provider with the `$saml_allowed_clock_skew` variable. This variable defines an acceptable range of time skew in seconds, allowing NGINX Plus to adjust the validation window for slight time differences between systems. +If `$saml_allowed_clock_skew` is not defined, NGINX Plus applies a default value of `120` seconds. ### Audience If the `AudienceRestriction` element is present, it restricts the assertion's applicability to specific intended audiences, or Service Providers, to which it may be sent. NGINX Plus verifies that the Service Provider's Entity ID, specified by the `$saml_sp_entity_id` variable, is listed as an acceptable audience for the assertion. This step ensures that the assertion is intended for the correct Service Provider and prevents unauthorized access to resources. @@ -389,6 +392,7 @@ Manual configuration involves reviewing the following files so that they match y - Modify all of the `map…$saml_idp_` blocks to match your IdP configuration - Modify the URI defined in `map…$saml_logout_redirect` to specify an unprotected resource to be displayed after requesting the `/logout` location - If NGINX Plus is deployed behind another proxy or load balancer, modify the `map…$redirect_base` and `map…$proto` blocks to define how to obtain the original protocol and port number. + - If you need to adjust the default allowable clock skew from the standard 120 seconds to accommodate time differences between the SP and IdP, add the `map…$saml_sp_clock_skew` block and specify the desired value in seconds. - **frontend.conf** - this is the reverse proxy configuration - Modify the upstream group to match your backend site or app diff --git a/saml_sp.js b/saml_sp.js index f8a2d77..dfea088 100644 --- a/saml_sp.js +++ b/saml_sp.js @@ -75,11 +75,13 @@ async function handleSAMLMessage(messageType, r) { node = root.Assertion.Subject.NameID ? root.Assertion.Subject.NameID : root.Assertion.Subject.EncryptedID || null; nameID = await extractNameID(node, opt.decryptKey); - checkSubjectConfirmation(root.Assertion.Subject.SubjectConfirmation); + checkSubjectConfirmation(root.Assertion.Subject.SubjectConfirmation, + opt.allowedClockSkew); /* Parse the Asserttion Conditions and Authentication Statement */ - checkConditions(root.Assertion.Conditions, opt.spEntityID); - const authnStatement = parseAuthnStatement(root.Assertion.AuthnStatement); + checkConditions(root.Assertion.Conditions, opt.spEntityID, opt.allowedClockSkew); + const authnStatement = parseAuthnStatement(root.Assertion.AuthnStatement, null, + opt.allowedClockSkew); /* Set session cookie and save SAML variables and attributes */ const sessionCookie = setSessionCookie(r); @@ -168,16 +170,13 @@ function processSAMLMessageHeader(r, opt, root) { } /* Check the date and time when the SAML message was issued (Required) */ - const currentTime = new Date(); const issueInstant = root.$attr$IssueInstant; if (!issueInstant) { throw Error("IssueInstant attribute is missing in the SAML message"); } const issueInstantDate = new Date(issueInstant); - if (issueInstantDate > currentTime) { - throw Error(`IssueInstant is in the future. Check clock skew of SP and IdP`); - } + checkTimeValidity(issueInstantDate, null, opt.allowedClockSkew); /* Check SAML message ID (Required) */ const id = root.$attr$ID; @@ -294,9 +293,10 @@ async function extractNameID(root, keyData) { * "urn:oasis:names:tc:SAML:2.0:cm:bearer". * * @param {Object} root - A SAML SubjectConfirmation XMLDoc object returned by xml.parse(). + * @param {number} [allowedClockSkew] - The allowed clock skew in seconds. * @throws {Error} - If the SubjectConfirmationData is missing or the subject has expired. */ -function checkSubjectConfirmation(root) { +function checkSubjectConfirmation(root, allowedClockSkew) { if (!root) { return; } @@ -308,12 +308,8 @@ function checkSubjectConfirmation(root) { 'SubjectConfirmation'); } - const now = new Date(); const notOnOrAfter = root.$attr$NotOnOrAfter ? new Date(root.$attr$NotOnOrAfter) : null; - if (notOnOrAfter && notOnOrAfter < now) { - throw new Error(`The Subject has expired. Current time is ${now} ` + - `and NotOnOrAfter is ${notOnOrAfter}`); - } + checkTimeValidity(null, notOnOrAfter, allowedClockSkew); } } @@ -327,27 +323,19 @@ function checkSubjectConfirmation(root) { * * @param {Object} root - A SAML Conditions XMLDoc object returned by xml.parse(). * @param {string} spEntityId - The EntityID of the Service Provider (SP). + * @param {number} [allowedClockSkew] - The allowed clock skew in seconds. * @throws {Error} - If Conditions element is missing or the assertion is not valid or expired. * Also throws an error if the audience restriction is not satisfied. */ -function checkConditions(root, spEntityId) { +function checkConditions(root, spEntityId, allowedClockSkew) { if (!root) { throw Error("Conditions element is missing in the Assertion"); } - const now = new Date(); const notBefore = root.$attr$NotBefore ? new Date(root.$attr$NotBefore) : null; const notOnOrAfter = root.$attr$NotOnOrAfter ? new Date(root.$attr$NotOnOrAfter) : null; - if (notBefore && notBefore > now) { - throw Error(`The Assertion is not yet valid. Current time is ${now} and ` + - `NotBefore is ${notBefore}`); - } - - if (notOnOrAfter && notOnOrAfter < now) { - throw Error(`The Assertion has expired. Current time is ${now} and ` + - `NotOnOrAfter is ${notOnOrAfter}`); - } + checkTimeValidity(notBefore, notOnOrAfter, allowedClockSkew); /* Check the audience restriction */ if (root.AudienceRestriction && root.AudienceRestriction.Audience) { @@ -376,11 +364,12 @@ function checkConditions(root, spEntityId) { * @param {number} [maxAuthenticationAge] - The maximum age (in seconds) of the authentication * statement. If provided, the function will check * if the AuthnInstant is within the allowed age. + * @param {number} [allowedClockSkew] - The allowed clock skew in seconds. * @throws {Error} - If AuthnInstant, SessionNotOnOrAfter, or AuthnContext elements are missing, * invalid, or expired. * @returns {Object} - An object with SessionIndex and AuthnContextClassRef properties. */ -function parseAuthnStatement(root, maxAuthenticationAge) { +function parseAuthnStatement(root, maxAuthenticationAge, allowedClockSkew) { /* AuthnStatement element is optional */ if (!root) { return; @@ -401,14 +390,10 @@ function parseAuthnStatement(root, maxAuthenticationAge) { } const sessionIndex = root.$attr$SessionIndex || null; - - const now = new Date(); const sessionNotOnOrAfter = root.$attr$SessionNotOnOrAfter ? new Date(root.$attr$SessionNotOnOrAfter) : null; - if (sessionNotOnOrAfter && sessionNotOnOrAfter < now) { - throw Error(`The Assertion Session has expired. Current time is ${now} and ` + - `SessionNotOnOrAfter is ${sessionNotOnOrAfter}`); - } + + checkTimeValidity(null, sessionNotOnOrAfter, allowedClockSkew); root = root.AuthnContext; @@ -425,6 +410,31 @@ function parseAuthnStatement(root, maxAuthenticationAge) { return [sessionIndex, authnContextClassRef]; } +/** + * Checks if the current time is within the allowed time range specified by + * notBefore and notOnOrAfter. + * + * @param {Date} notBefore - The notBefore time. + * @param {Date} notOnOrAfter - The notOnOrAfter time. + * @param {number} [allowedClockSkew] - Allowed clock skew in seconds. + * @throws {Error} - If the current time is outside the allowed time range. + */ +function checkTimeValidity(notBefore, notOnOrAfter, allowedClockSkew) { + const now = new Date(); + + if (notBefore && notBefore > new Date(now.getTime() + allowedClockSkew * 1000)) { + throw Error(`The Assertion is not yet valid. Current time is ${now} ` + + `and NotBefore is ${notBefore}. ` + + `Allowed clock skew is ${allowedClockSkew} seconds.`); + } + + if (notOnOrAfter && notOnOrAfter < new Date(now.getTime() - allowedClockSkew * 1000)) { + throw Error(`The Assertion has expired. Current time is ${now} ` + + `and NotOnOrAfter is ${notOnOrAfter}. ` + + `Allowed clock skew is ${allowedClockSkew} seconds.`); + } +} + function saveSAMLVariables(r, nameID, authnStatement) { r.variables.saml_name_id = nameID[0]; r.variables.saml_name_id_format = nameID[1]; @@ -1202,6 +1212,7 @@ function parseConfigurationOptions(r, messageType) { opt.wantSignedResponse = validateTrueOrFalse('saml_sp_want_signed_response'); opt.wantSignedAssertion = validateTrueOrFalse('saml_sp_want_signed_assertion'); opt.wantEncryptedAssertion = validateTrueOrFalse('saml_sp_want_encrypted_assertion'); + opt.allowedClockSkew = validateClockSkew('saml_allowed_clock_skew', 120); } if (messageType === 'AuthnRequest') { @@ -1223,6 +1234,7 @@ function parseConfigurationOptions(r, messageType) { } opt.relayState = r.variables.saml_logout_landing_page; opt.nameID = r.variables.saml_name_id; + opt.allowedClockSkew = validateClockSkew('saml_allowed_clock_skew', 120); } if (opt.wantSignedResponse || opt.wantSignedAssertion) { @@ -1291,6 +1303,20 @@ function parseConfigurationOptions(r, messageType) { return value; } + function validateClockSkew(name, defaultValue) { + const value = r.variables[name]; + if (value === undefined) { + return defaultValue; + } + + const parsedValue = parseInt(value, 10); + if (isNaN(parsedValue) || parsedValue.toString() !== value) { + throw Error(`${prefix} Invalid "${name}": "${value}", must be a valid integer.`); + } + + return parsedValue; + } + return opt; } diff --git a/t/js_saml.t b/t/js_saml.t index 38987e0..8a0f90f 100644 --- a/t/js_saml.t +++ b/t/js_saml.t @@ -133,6 +133,9 @@ http { keyval_zone zone=saml_cookie_flags:1M state=%%TESTDIR%%/saml_cookie_flags.json; keyval $host $saml_cookie_flags zone=saml_cookie_flags; + keyval_zone zone=saml_allowed_clock_skew:1M state=%%TESTDIR%%/saml_allowed_clock_skew.json; + keyval $host $saml_allowed_clock_skew zone=saml_allowed_clock_skew; + keyval_zone zone=redirect_base:1M state=%%TESTDIR%%/redirect_base.json; keyval $host $redirect_base zone=redirect_base; @@ -282,7 +285,7 @@ my $sp_pub = $t->read_file('sp.example.com.crt'); my $js_filename = 'saml_sp.js'; $t->write_file($js_filename, read_file("../$js_filename")); -$t->try_run('no njs available')->plan(128); +$t->try_run('no njs available')->plan(132); my $api_version = (sort { $a <=> $b } @{ api() })[-1]; my $kv = "/api/$api_version/http/keyvals"; @@ -525,7 +528,7 @@ like($r, qr/HTTP\/1\.1 500.*Unsupported SAML Version/s, my ($ptime, $ftime) = get_time(); $r = modify_saml_obj($xml_obj, '/samlp:Response', 'IssueInstant', $ftime); -like($r, qr/HTTP\/1\.1 500.*IssueInstant.*in the future/s, +like($r, qr/HTTP\/1\.1 500.*Assertion is not yet valid/s, 'response future issue instant'); $r = modify_saml_obj($xml_obj, '/samlp:Response', 'IssueInstant'); @@ -551,7 +554,7 @@ $r = modify_saml_obj($xml_obj, '//saml:Assertion', 'ID'); like($r, qr/HTTP\/1\.1 500.*ID.*is missing/s, 'assertion no id'); $r = modify_saml_obj($xml_obj, '//saml:Assertion', 'IssueInstant', $ftime); -like($r, qr/HTTP\/1\.1 500.*IssueInstant.*in the future/s, +like($r, qr/HTTP\/1\.1 500.*Assertion is not yet valid/s, 'assertion future issue instant'); $r = modify_saml_obj($xml_obj, '//saml:NameID'); @@ -568,7 +571,7 @@ like($r, qr/HTTP\/1\.1 500.*SubjectConfirmationData.*is missing/s, $r = modify_saml_obj($xml_obj, '//saml:SubjectConfirmationData', 'NotOnOrAfter', $ptime); -like($r, qr/HTTP\/1\.1 500.*Subject has expired/s, +like($r, qr/HTTP\/1\.1 500.*Assertion has expired/s, 'assertion subject has expired'); $r = modify_saml_obj($xml_obj, '//saml:Conditions'); @@ -596,7 +599,7 @@ like(get('/', auth_token => get_auth_token($r)), qr/Welcome user1/, $r = modify_saml_obj($xml_obj, '//saml:AuthnStatement', 'SessionNotOnOrAfter', $ptime); -like($r, qr/HTTP\/1\.1 500.*Assertion Session has expired/s, +like($r, qr/HTTP\/1\.1 500.*Assertion has expired/s, 'assertion session has expired'); $r = modify_saml_obj($xml_obj, '//saml:AuthnStatement', 'SessionIndex'); @@ -611,6 +614,26 @@ $r = modify_saml_obj($xml_obj, '//saml:AttributeStatement'); like(get('/', auth_token => get_auth_token($r)), qr/Welcome user1/, 'assertion no attribute statement'); +$r = modify_saml_obj($xml_obj, '//saml:Conditions', 'NotBefore', $ftime); +like($r, qr/HTTP\/1\.1 500.*Allowed clock skew is 120 seconds/s, + 'Allowed clock skew default 120'); + +cfg_post({saml_allowed_clock_skew => '60'}, 1); + +$r = modify_saml_obj($xml_obj, '//saml:Conditions', 'NotBefore', $ftime); +like($r, qr/HTTP\/1\.1 500.*Allowed clock skew is 60 seconds/s, + 'Allowed Clock scew 60'); + +cfg_post({saml_allowed_clock_skew => '360'}); + +$r = modify_saml_obj($xml_obj, '//saml:Conditions', 'NotBefore', $ftime); +like(get('/', auth_token => get_auth_token($r)), qr/Welcome user1/, + 'Allowed Clock scew NotBefore'); + +$r = modify_saml_obj($xml_obj, '//saml:Conditions', 'NotOnOrAfter', $ptime); +like(get('/', auth_token => get_auth_token($r)), qr/Welcome user1/, + 'Allowed Clock scew NotOnOrAfter'); + ### SP-initiated logout # Logout Request