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

Remove unused userinfo and online token validation #3239

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Remove unused userinfo and online token validation #3239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
112bc60
Remove unused userinfo and online token validation
npalaska Feb 8, 2023
b9a9992
fix auth.py imports
npalaska Feb 9, 2023
43d97fd
Merge branch 'openid-connect' into pbench_1074
npalaska Feb 9, 2023
8271f92
Merge branch 'openid-connect' into pbench_1074
npalaska Feb 9, 2023
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
94 changes: 1 addition & 93 deletions lib/pbench/server/auth/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,78 +362,14 @@ def __init__(
pem_public_key += "-----END PUBLIC KEY-----\n"
self._pem_public_key = pem_public_key

well_known_endpoint = ".well-known/openid-configuration"
well_known_uri = f"realms/{self._realm_name}/{well_known_endpoint}"
endpoints_json = self._connection.get(well_known_uri).json()

try:
self._userinfo_endpoint = endpoints_json["userinfo_endpoint"]
self._tokeninfo_endpoint = endpoints_json["introspection_endpoint"]
except KeyError as e:
raise OpenIDClientError(
HTTPStatus.BAD_GATEWAY,
f"Missing endpoint {e!r} at {well_known_uri}; Endpoints found:"
f" {endpoints_json}",
)

def __repr__(self):
return (
f"OpenIDClient(server_url={self._connection.server_url}, "
f"client_id={self.client_id}, realm_name={self._realm_name}, "
f"headers={self._connection.headers})"
)

def token_introspect_online(self, token: str) -> Optional[JSON]:
"""The introspection endpoint is used to retrieve the active state of a
token.

It can only be invoked by confidential clients.

The introspected JWT token contains the claims specified in
https://tools.ietf.org/html/rfc7662.

Note: this is not supposed to be used in production, instead rely on
offline token validation because of security concerns mentioned in
https://www.rfc-editor.org/rfc/rfc7662.html#section-4.

Args:
token : token value to introspect

Returns:
Extracted token information
{
"aud": <targeted_audience_id>,
"email_verified": <boolean>,
"expires_in": <number_of_seconds>,
"access_type": "offline",
"exp": <unix_timestamp>,
"azp": <client_id>,
"scope": <scope_string>, # "openid email profile"
"email": <user_email>,
"sub": <user_id>
}
"""
if not self._tokeninfo_endpoint:
return None

payload = {
"client_id": self.client_id,
"client_secret": self._client_secret_key,
"token": token,
}
token_payload = self._connection.post(
self._tokeninfo_endpoint, data=payload
).json()

audience = token_payload.get("aud")
if not audience or self.client_id not in audience:
# If our client is not an intended audience for the token,
# we will not verify the token.
token_payload = None

return token_payload

def token_introspect_offline(self, token: str) -> JSON:
def token_introspect(self, token: str) -> JSON:
"""Utility method to decode access/Id tokens using the public key
provided by the identity provider.

Expand Down Expand Up @@ -481,31 +417,3 @@ def token_introspect_offline(self, token: str) -> JSON:
jwt.InvalidAudienceError,
) as exc:
raise OpenIDTokenInvalid() from exc

def get_userinfo(self, token: str = None) -> JSON:
"""The userinfo endpoint returns standard claims about the authenticated
user, and is protected by a bearer token.

FIXME - This method appears unused in the rest of the code.

http://openid.net/specs/openid-connect-core-1_0.html#UserInfo

Args:
token : Valid token to extract the userinfo

Returns:
Userinfo payload
{
"family_name": <surname>,
"sub": <user_id>,
"picture": <URL>,
"locale": <locale_name>,
"email_verified": <boolean>,
"given_name": <given_name>,
"email": <email_address>,
"hd": <domain_name>,
"name": <full_name>
}
"""
headers = {"Authorization": f"Bearer {token}"} if token else None
return self._connection.get(self._userinfo_endpoint, headers=headers).json()
24 changes: 3 additions & 21 deletions lib/pbench/server/auth/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@
import jwt

from pbench.server import PbenchServerConfig
from pbench.server.auth import (
InternalUser,
OpenIDClient,
OpenIDClientError,
OpenIDTokenInvalid,
)
from pbench.server.auth import InternalUser, OpenIDClient, OpenIDTokenInvalid
from pbench.server.database.models.active_tokens import ActiveTokens
from pbench.server.database.models.users import User

Expand Down Expand Up @@ -194,28 +189,15 @@ def verify_auth_oidc(auth_token: str) -> Optional[InternalUser]:
InternalUser object if the verification succeeds, None on failure.
"""
try:
token_payload = oidc_client.token_introspect_offline(token=auth_token)
token_payload = oidc_client.token_introspect(token=auth_token)
except OpenIDTokenInvalid:
token_payload = None
except Exception:
current_app.logger.exception(
"Unexpected exception occurred while verifying the auth token {}",
auth_token,
)

# Offline token verification resulted in some unexpected error,
# perform the online token verification.

# Note: Online verification should NOT be performed frequently, and it
# is only allowed for non-public clients.
try:
token_payload = oidc_client.token_introspect_online(token=auth_token)
except OpenIDClientError as exc:
current_app.logger.debug(
"Can not perform OIDC online token verification, '{}'", exc
)
token_payload = None

token_payload = None
Comment on lines 191 to +200
Copy link
Member

Choose a reason for hiding this comment

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

Arguably, this is beyond the scope of this PR, but I'll make the comment anyway, in case you're interested.

Since we (now) have the same response, other than the logger call, regardless of what exception we receive, there's not much reason to have two separate except clauses here.

Moreover, the fact that there was an unexpected exception should probably not be noted here (we're in the "middle" of the call stack).

So, it strikes me that lines 197-204 should be slimmed down to just:

    except Exception:
        token_payload = None

And the catch-all with the logger call should be moved to the end of token_introspect().

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the initial intention was that we didn't want to log for these valid jwt exceptions jwt.ExpiredSignatureError, jwt.InvalidSignatureError, jwt.InvalidAudienceError. These exceptions mean that the token decode was invalid and realistically we will get a lot of tokens that are invalid because of these 3. If we encounter any of these exceptions then we will raise OpenIDTokenInvalid error and move on.

However, if some other exception happened other than these 3 then that means something with our decode has actually failed (not invalid) so we catch that with a generic exception and log the message and move on.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed: we want to log if we or something on which we depend (like Keycloak itself) does something "wrong" or "unexpected", but we don't want to complain every time a client (or "would-be client") does something wrong, because that'll flood the logs and serve no real purpose.

And for authentication failures, we probably don't ever want to return more information to the caller than just "unauthorized".

Copy link
Member

Choose a reason for hiding this comment

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

@npalaska, I agree; however, the catch-generic-log-and-move-on should be done in token_introspect() and not here in verify_auth_oidc().

return (
None
if token_payload is None
Expand Down
166 changes: 9 additions & 157 deletions lib/pbench/test/unit/server/auth/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,35 +352,10 @@ def __init__(
def get(self, path: str, **kwargs) -> MockResponse:
if path.endswith(f"realms/{realm_name}"):
json_d = {"public_key": public_key}
elif path.endswith(f"realms/{realm_name}/.well-known/openid-configuration"):
json_d = {
"userinfo_endpoint": f"{self.server_url}/userinfo",
"introspection_endpoint": f"{self.server_url}/introspection",
}
if client_id == "us-missing-userinfo-ep":
del json_d["userinfo_endpoint"]
elif client_id == "us-missing-introspection-ep":
del json_d["introspection_endpoint"]
elif client_id == "us-empty-tokeninfo-ep":
json_d["introspection_endpoint"] = ""
elif path.endswith("userinfo"):
json_d = {"path": path, "kwargs": kwargs}
else:
raise Exception(f"MockConnection: unrecognized .get(path={path})")
return MockResponse(json_d)

def post(self, path: str, data: Dict, **kwargs) -> MockResponse:
if path.endswith("/introspection"):
if client_id == "us-raise-exc":
raise OpenIDClientError(400, "Introspection failed")
cid = "other-client" if client_id == "us-other-aud" else client_id
json_d = {"aud": [cid], "data": data}
if client_id == "us-token-payload":
json_d["sub"] = "67890"
else:
raise Exception(f"MockConnection: unrecognized .post(path={path})")
return MockResponse(json_d)

monkeypatch.setattr(pbench.server.auth, "Connection", MockConnection)
return config

Expand Down Expand Up @@ -436,61 +411,8 @@ def test_construct_oidc_client_succ(self, monkeypatch):
oidc_client._pem_public_key
== f"-----BEGIN PUBLIC KEY-----\n{public_key}\n-----END PUBLIC KEY-----\n"
)
assert oidc_client._userinfo_endpoint.endswith("/userinfo")
assert oidc_client._tokeninfo_endpoint.endswith("/introspection")

def test_construct_oidc_client_fail_ep(self, monkeypatch):
"""Verify .construct_oidc_client() failure mode where OpenIDClientError
is raised
"""
# First failure should be caused by missing "userinfo" endpoint.
client_id = "us-missing-userinfo-ep"
config = mock_connection(monkeypatch, client_id)
with pytest.raises(OpenIDClientError):
OpenIDClient.construct_oidc_client(config)

# Second failure should be caused by missing "introspection" endpoint.
client_id = "us-missing-introspection-ep"
config = mock_connection(monkeypatch, client_id)
with pytest.raises(OpenIDClientError):
OpenIDClient.construct_oidc_client(config)

def test_token_introspect_online_no_ep(self, monkeypatch):
"""Verify .token_introspect_online() with empty token information end
point
"""
client_id = "us-empty-tokeninfo-ep"
public_key = "opqrstu"
config = mock_connection(monkeypatch, client_id, public_key)
oidc_client = OpenIDClient.construct_oidc_client(config)
json_d = oidc_client.token_introspect_online("my-token")
assert json_d is None

def test_token_introspect_online_not_aud(self, monkeypatch):
"""Verify .token_introspect_online() with different audience."""
client_id = "us-other-aud"
public_key = "vwxyz01"
config = mock_connection(monkeypatch, client_id, public_key)
oidc_client = OpenIDClient.construct_oidc_client(config)
json_d = oidc_client.token_introspect_online("my-token")
assert json_d is None, f"{json_d!r}"

def test_token_introspect_online_succ(self, monkeypatch):
"""Verify .token_introspect_online() with different audience."""
client_id = "us"
public_key = "2345678"
config = mock_connection(monkeypatch, client_id, public_key)
secret = config["openid-connect"]["secret"]
oidc_client = OpenIDClient.construct_oidc_client(config)
json_d = oidc_client.token_introspect_online("my-token")
assert json_d["aud"] == [client_id]
assert json_d["data"] == {
"client_id": client_id,
"client_secret": secret,
"token": "my-token",
}, f"{json_d!r}"

def test_token_introspect_offline_succ(self, monkeypatch, rsa_keys):
def test_token_introspect_succ(self, monkeypatch, rsa_keys):
"""Verify .token_introspect_offline() success path"""
client_id = "us"
token, expected_payload = gen_rsa_token(client_id, rsa_keys["private_key"])
Expand All @@ -514,10 +436,10 @@ def test_token_introspect_offline_succ(self, monkeypatch, rsa_keys):
)

# This is the target test case.
response = oidc_client.token_introspect_offline(token)
response = oidc_client.token_introspect(token)
assert response == expected_payload

def test_token_introspect_offline_exp(self, monkeypatch, rsa_keys):
def test_token_introspect_exp(self, monkeypatch, rsa_keys):
"""Verify .token_introspect_offline() failure via expiration"""
client_id = "us"
token, expected_payload = gen_rsa_token(
Expand All @@ -531,12 +453,12 @@ def test_token_introspect_offline_exp(self, monkeypatch, rsa_keys):
oidc_client = OpenIDClient.construct_oidc_client(config)

with pytest.raises(OpenIDTokenInvalid) as exc:
oidc_client.token_introspect_offline(token)
oidc_client.token_introspect(token)
assert (
str(exc.value.__cause__) == "Signature has expired"
), f"{exc.value.__cause__}"

def test_token_introspect_offline_aud(self, monkeypatch, rsa_keys):
def test_token_introspect_aud(self, monkeypatch, rsa_keys):
"""Verify .token_introspect_offline() failure via audience error"""
client_id = "us"
token, expected_payload = gen_rsa_token(client_id, rsa_keys["private_key"])
Expand All @@ -547,10 +469,10 @@ def test_token_introspect_offline_aud(self, monkeypatch, rsa_keys):
oidc_client = OpenIDClient.construct_oidc_client(config)

with pytest.raises(OpenIDTokenInvalid) as exc:
oidc_client.token_introspect_offline(token)
oidc_client.token_introspect(token)
assert str(exc.value.__cause__) == "Invalid audience", f"{exc.value.__cause__}"

def test_token_introspect_offline_sig(self, monkeypatch, rsa_keys):
def test_token_introspect_sig(self, monkeypatch, rsa_keys):
"""Verify .token_introspect_offline() failure via signature error"""
client_id = "us"
token, expected_payload = gen_rsa_token(client_id, rsa_keys["private_key"])
Expand All @@ -564,27 +486,11 @@ def test_token_introspect_offline_sig(self, monkeypatch, rsa_keys):

with pytest.raises(OpenIDTokenInvalid) as exc:
# Make the signature invalid.
oidc_client.token_introspect_offline(token + "1")
oidc_client.token_introspect(token + "1")
assert (
str(exc.value.__cause__) == "Signature verification failed"
), f"{exc.value.__cause__}"

def test_get_userinfo(self, monkeypatch):
"""Verify .get_userinfo() properly invokes Connection.get()"""
# Mock the Connection object and generate an OpenIDClient object.
client_id = "us"
config = mock_connection(monkeypatch, client_id)
oidc_client = OpenIDClient.construct_oidc_client(config)

# Ensure .get_userinfo() invokes Connection.get() with the correct
# parameters.
token = "the-token"
json_d = oidc_client.get_userinfo(token)
assert json_d == {
"kwargs": {"headers": {"Authorization": "Bearer the-token"}},
"path": "https://example.com/userinfo",
}


@dataclass
class MockRequest:
Expand Down Expand Up @@ -776,61 +682,7 @@ def tio_exc(token: str) -> JSON:
app = Flask("test-verify-auth-oidc-offline-invalid")
app.logger = make_logger
with app.app_context():
monkeypatch.setattr(oidc_client, "token_introspect_offline", tio_exc)
user = Auth.verify_auth(token)

assert user is None

def test_verify_auth_oidc_online(self, monkeypatch, rsa_keys, make_logger):
"""Verify OIDC token online verification works via Auth.verify_auth()"""
# Use the client ID to direct the MockConnection class to return a
# token payload.
client_id = "us-token-payload"
token, expected_payload = gen_rsa_token(client_id, rsa_keys["private_key"])

# Mock the Connection object and generate an OpenIDClient object,
# installing it as Auth module's OIDC client.
config = mock_connection(
monkeypatch, client_id, public_key=rsa_keys["public_key"]
)
oidc_client = OpenIDClient.construct_oidc_client(config)
monkeypatch.setattr(Auth, "oidc_client", oidc_client)

def tio_exc(token: str) -> JSON:
raise Exception("Token introspection offline failed for some reason")

app = Flask("test-verify-auth-oidc-online")
app.logger = make_logger
with app.app_context():
monkeypatch.setattr(oidc_client, "token_introspect_offline", tio_exc)
user = Auth.verify_auth(token)

assert user.id == "67890"

def test_verify_auth_oidc_online_fail(self, monkeypatch, rsa_keys, make_logger):
"""Verify OIDC token online verification via Auth.verify_auth() fails
returning None
"""
# Use the client ID to direct the online token introspection to raise
# an OpenIDClientError.
client_id = "us-raise-exc"
token, expected_payload = gen_rsa_token(client_id, rsa_keys["private_key"])

# Mock the Connection object and generate an OpenIDClient object,
# installing it as Auth module's OIDC client.
config = mock_connection(
monkeypatch, client_id, public_key=rsa_keys["public_key"]
)
oidc_client = OpenIDClient.construct_oidc_client(config)
monkeypatch.setattr(Auth, "oidc_client", oidc_client)

def tio_exc(token: str) -> JSON:
raise Exception("Token introspection offline failed for some reason")

app = Flask("test-verify-auth-oidc-online-fail")
app.logger = make_logger
with app.app_context():
monkeypatch.setattr(oidc_client, "token_introspect_offline", tio_exc)
monkeypatch.setattr(oidc_client, "token_introspect", tio_exc)
user = Auth.verify_auth(token)

assert user is None