Skip to content

Commit

Permalink
Add support for clock skew tolerance between SP and IdP.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@route443
route443 committed Nov 3, 2023
1 parent d8b75d1 commit 36b4b75
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 37 deletions.
6 changes: 5 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
88 changes: 57 additions & 31 deletions saml_sp.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
}
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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];
Expand Down Expand Up @@ -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') {
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down
33 changes: 28 additions & 5 deletions t/js_saml.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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');
Expand All @@ -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
Expand Down

0 comments on commit 36b4b75

Please sign in to comment.