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

Logging and misc improvements #133

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Logging and misc improvements #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9962bc5
feat: (check for access_token and id_token in oauth assertions, more …
Nov 3, 2021
edd246b
fix: (add fallback value to avoid x-forwarded-uri being undefined if …
Nov 3, 2021
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: 71 additions & 54 deletions src/plugin/oauth/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2075,61 +2075,62 @@ class BaseOauthPlugin extends BasePlugin {
*/
if (
pluginStrategy == PLUGIN_STRATEGY_OIDC &&
plugin.config.assertions.aud &&
idToken.aud != plugin.config.client.client_id
plugin.config.assertions.aud
) {
return false;
if (idToken.aud != plugin.config.client.client_id) {
plugin.server.logger.verbose("aud does not match client_id");
return false;
}
} else {
plugin.server.logger.verbose("Assertions: SKIPPED aud")
}

/**
* access token is expired and refresh tokens are disabled
*/
if (
plugin.config.assertions.exp &&
tokenset_is_expired(tokenSet) &&
!(
plugin.config.features.refresh_access_token &&
tokenset_can_refresh(tokenSet)
)
) {
plugin.server.logger.verbose(
"tokenSet is expired and refresh tokens disabled"
);
return false;
}
if (plugin.config.assertions.exp) {
const is_expired = tokenset_is_expired(tokenSet)

/**
* both access and refresh tokens are expired and refresh is enabled
*/
if (
plugin.config.assertions.exp &&
tokenset_is_expired(tokenSet) &&
plugin.config.features.refresh_access_token &&
!tokenset_can_refresh(tokenSet)
) {
plugin.server.logger.verbose(
"tokenSet expired and refresh no longer available"
);
return false;
if (is_expired) {
plugin.server.logger.verbose("tokenSet is expired");

if (!plugin.config.features.refresh_access_token) {
plugin.server.logger.verbose("refresh tokens disabled");
return false
}

if (!tokenset_can_refresh(tokenSet)) {
plugin.server.logger.verbose("refresh no longer available");
return false
}

plugin.server.logger.verbose("refresh token available");
}
plugin.server.logger.verbose("Assertions: PASSED exp");
} else {
plugin.server.logger.verbose("Assertions: SKIPPED exp")
}

if (
pluginStrategy == PLUGIN_STRATEGY_OIDC &&
Copy link
Owner

Choose a reason for hiding this comment

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

This check needs to remain.

Copy link
Author

Choose a reason for hiding this comment

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

Can you elaborate on this? We use OAuth2 and need it to validate nbf and iss.

plugin.config.assertions.nbf &&
tokenset_is_premature(tokenSet)
) {
plugin.server.logger.verbose("tokenSet is premature");
return false;

if (plugin.config.assertions.nbf) {
if (tokenset_is_premature(tokenSet)) {
plugin.server.logger.verbose("tokenSet is premature");
return false
}
plugin.server.logger.verbose("Assertions: PASSED nbf")
} else {
plugin.server.logger.verbose("Assertions: SKIPPED nbf")
}

if (
pluginStrategy == PLUGIN_STRATEGY_OIDC &&
Copy link
Owner

Choose a reason for hiding this comment

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

This check needs to remain.

plugin.config.assertions.iss
) {

if (plugin.config.assertions.iss) {
if (!tokenset_issuer_match(tokenSet, issuer.issuer)) {
plugin.server.logger.verbose("tokenSet has a mismatch issuer");
return false;
}
plugin.server.logger.verbose("Assertions: PASSED iss")
} else {
plugin.server.logger.verbose("Assertions: SKIPPED iss")
}

if (
Expand Down Expand Up @@ -2166,32 +2167,48 @@ class BaseOauthPlugin extends BasePlugin {
) {
await plugin.set_introspection_cache(session_id, response);
}
plugin.server.logger.verbose("Assertions: PASSED introspect_access_token")
} else {
plugin.server.logger.verbose("Assertions: SKIPPED introspect_access_token")
}

if (
pluginStrategy == PLUGIN_STRATEGY_OIDC &&
Copy link
Owner

Choose a reason for hiding this comment

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

This check needs to remain.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand this, is id_token assertions not available in OAuth2?

plugin.config.assertions.id_token
) {
let idToken;
idToken = jwt.decode(tokenSet.id_token);
let idTokenValid = await plugin.id_token_assertions(idToken);

if (plugin.config.assertions.id_token) {
if (!tokenSet.id_token) {
plugin.server.logger.verbose("Missing id_token")
return false
}

const idToken = jwt.decode(tokenSet.id_token);
const idTokenValid = await plugin.id_token_assertions(idToken);
if (!idTokenValid) {
plugin.server.logger.verbose("Invalid id_token")
return false;
}
plugin.server.logger.verbose("Assertions: PASSED id_token")
} else {
plugin.server.logger.verbose("Assertions: SKIPPED id_token")
}

if (
pluginStrategy == PLUGIN_STRATEGY_OIDC &&
Copy link
Owner

Choose a reason for hiding this comment

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

This check needs to remain.

plugin.config.assertions.access_token
) {
let accessToken;
accessToken = jwt.decode(tokenSet.access_token);
Copy link
Owner

Choose a reason for hiding this comment

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

Was this the line that was blowing up previously? Or was it something else?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I believe the decode function doesn't handle undefined inputs

let accessTokenValid = await plugin.access_token_assertions(accessToken);

if (plugin.config.assertions.access_token) {
if (!tokenSet.access_token) {
plugin.server.logger.verbose("Missing access_token")
return false
}

const accessToken = jwt.decode(tokenSet.access_token);
const accessTokenValid = await plugin.access_token_assertions(accessToken);
if (!accessTokenValid) {
plugin.server.logger.verbose("Invalid access_token")
return false;
}
plugin.server.logger.verbose("Assertions: PASSED access_token")
} else {
plugin.server.logger.verbose("Assertions: SKIPPED access_token")
}

plugin.server.logger.verbose("Assertions: PASSED")
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function get_parent_request_uri(req) {
originalRequestURI = originalRequestURI.replace(/^(.+?)\/*?$/, "$1"); // remove all trailing slashes
originalRequestURI += req.headers["x-forwarded-uri"];
} else {
originalRequestURI += req.headers["x-forwarded-uri"];
originalRequestURI += req.headers["x-forwarded-uri"] || '';
Copy link
Owner

Choose a reason for hiding this comment

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

I'll think about this change. It should be handled differently but not sure how exactly :( it probably needs to throw an error if missing as the data is critical to function properly.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, throwing a hard error is much better than it being undefined as you'll be able to identify the problem quickly.

}

//x-forwarded-port
Expand Down