From 8ce5c6ead78e80eb8d1d529d62114e0cd91dc3b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Bompard?= Date: Fri, 13 Jan 2023 16:38:15 +0100 Subject: [PATCH] Store the chosen IPA server in the session for both the user and admin client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents admin commands from running on a server and user commands running on another. Fixes: #1079 Signed-off-by: Aurélien Bompard --- news/1079.bug | 3 ++ noggin/controller/authentication.py | 2 +- noggin/controller/password.py | 4 +-- noggin/controller/registration.py | 2 +- noggin/security/ipa.py | 26 +++++++++++++---- noggin/security/ipa_admin.py | 7 ++--- tests/unit/conftest.py | 3 +- tests/unit/controller/test_forgot_password.py | 6 ++-- tests/unit/security/test_ipa.py | 28 +++++++++++++++++-- 9 files changed, 61 insertions(+), 20 deletions(-) create mode 100644 news/1079.bug diff --git a/news/1079.bug b/news/1079.bug new file mode 100644 index 000000000..248da995b --- /dev/null +++ b/news/1079.bug @@ -0,0 +1,3 @@ +Store the chosen IPA server in the session for both the user client and the +admin client. This prevents admin commands from running on a server and user +commands running on another. diff --git a/noggin/controller/authentication.py b/noggin/controller/authentication.py index db4e2b720..dbdfacb0a 100644 --- a/noggin/controller/authentication.py +++ b/noggin/controller/authentication.py @@ -64,7 +64,7 @@ def otp_sync(): if form.validate_on_submit(): with handle_form_errors(form): try: - ipa = untouched_ipa_client(current_app) + ipa = untouched_ipa_client(current_app, session) ipa.otptoken_sync( user=form.username.data, password=form.password.data, diff --git a/noggin/controller/password.py b/noggin/controller/password.py index 4e636bb81..463cd1f01 100644 --- a/noggin/controller/password.py +++ b/noggin/controller/password.py @@ -38,7 +38,7 @@ def _validate_change_pw_form(form, username, ipa=None): if ipa is None: - ipa = untouched_ipa_client(current_app) + ipa = untouched_ipa_client(current_app, session) current_password = form.current_password.data password = form.password.data @@ -236,7 +236,7 @@ def forgot_password_change(): # example) ipa_admin.user_mod(username, userpassword=temp_password) # Change the password as the user, so it's not expired. - ipa = untouched_ipa_client(current_app) + ipa = untouched_ipa_client(current_app, session) ipa.change_password( username, new_password=password, diff --git a/noggin/controller/registration.py b/noggin/controller/registration.py index 6edfd1a9b..e459c2d40 100644 --- a/noggin/controller/registration.py +++ b/noggin/controller/registration.py @@ -264,7 +264,7 @@ def activate_account(): # First, set it as an admin. This will mark it as expired. ipa_admin.user_mod(user.username, userpassword=password) # And now we set it again as the user, so it is not expired any more. - ipa = untouched_ipa_client(current_app) + ipa = untouched_ipa_client(current_app, session) ipa.change_password( user.username, new_password=password, old_password=password ) diff --git a/noggin/security/ipa.py b/noggin/security/ipa.py index c9f8c9838..8fbc35385 100644 --- a/noggin/security/ipa.py +++ b/noggin/security/ipa.py @@ -123,12 +123,26 @@ def raise_on_failed(result): raise ValidationError(failed) +def choose_server(app, session=None): + """ + Choose a server among the configured IPA server and store the result in the session. + """ + server = None + if session is not None: + server = session.get('noggin_ipa_server_hostname', None) + if server is None: + server = random.choice(app.config['FREEIPA_SERVERS']) + if session is not None: + session['noggin_ipa_server_hostname'] = server + return server + + # Construct an IPA client from app config, but don't attempt to log in with it # or to form a session of any kind with it. This is useful for one-off cases # like password resets where a session isn't actually required. -def untouched_ipa_client(app): +def untouched_ipa_client(app, session): return Client( - random.choice(app.config['FREEIPA_SERVERS']), + choose_server(app, session), verify_ssl=app.config['FREEIPA_CACERT'], ) @@ -142,7 +156,7 @@ def untouched_ipa_client(app): # It will be None if no session was provided or was provided but invalid. def maybe_ipa_session(app, session): encrypted_session = session.get('noggin_session', None) - server_hostname = session.get('noggin_ipa_server_hostname', None) + server_hostname = choose_server(app, session) if encrypted_session and server_hostname: fernet = Fernet(app.config['FERNET_SECRET']) ipa_session = fernet.decrypt(encrypted_session) @@ -174,8 +188,9 @@ def maybe_ipa_login(app, session, username, userpassword): # in the session and just always use that. Flask sessions are signed, so we # are safe in later assuming that the server hostname cookie has not been # altered. - chosen_server = random.choice(app.config['FREEIPA_SERVERS']) - client = Client(chosen_server, verify_ssl=app.config['FREEIPA_CACERT']) + client = Client( + choose_server(app, session), verify_ssl=app.config['FREEIPA_CACERT'] + ) auth = client.login(username, userpassword) @@ -185,7 +200,6 @@ def maybe_ipa_login(app, session, username, userpassword): bytes(client._session.cookies['ipa_session'], 'utf8') ) session['noggin_session'] = encrypted_session - session['noggin_ipa_server_hostname'] = chosen_server session['noggin_username'] = username return client diff --git a/noggin/security/ipa_admin.py b/noggin/security/ipa_admin.py index 6868b9191..21cb57423 100644 --- a/noggin/security/ipa_admin.py +++ b/noggin/security/ipa_admin.py @@ -1,9 +1,8 @@ -import random from functools import wraps -from flask import current_app +from flask import current_app, session -from .ipa import Client +from .ipa import choose_server, Client class IPAAdmin: @@ -66,7 +65,7 @@ def __maybe_ipa_admin_session(self): username = current_app.extensions["ipa-admin"]["username"] password = current_app.extensions["ipa-admin"]["password"] client = Client( - random.choice(current_app.config['FREEIPA_SERVERS']), + choose_server(current_app, session), verify_ssl=current_app.config['FREEIPA_CACERT'], ) client.login(username, password) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index a5cff3943..93169e475 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -4,6 +4,7 @@ import pytest import python_freeipa +from flask import session from vcr import VCR from noggin.app import create_app, ipa_admin @@ -158,7 +159,7 @@ def _make_user(name): o_loginshell='/bin/bash', fascreationtime=f"{now.isoformat()}Z", ) - ipa = untouched_ipa_client(app) + ipa = untouched_ipa_client(app, session) ipa.change_password(name, password, password) created_users.append(name) diff --git a/tests/unit/controller/test_forgot_password.py b/tests/unit/controller/test_forgot_password.py index 7607011bf..5cc2b9c0e 100644 --- a/tests/unit/controller/test_forgot_password.py +++ b/tests/unit/controller/test_forgot_password.py @@ -6,7 +6,7 @@ import python_freeipa from bs4 import BeautifulSoup from fedora_messaging import testing as fml_testing -from flask import current_app +from flask import current_app, session from noggin.app import ipa_admin, mailer from noggin.representation.user import User @@ -75,7 +75,7 @@ def dummy_user_no_password(ipa_testing_config, app): o_loginshell='/bin/bash', fascreationtime=f"{now.isoformat()}Z", ) - # ipa = untouched_ipa_client(app) + # ipa = untouched_ipa_client(app, session) # ipa.change_password(name, password, password) yield @@ -278,7 +278,7 @@ def test_change_too_old(client, token_for_dummy_user, patched_lock): def test_change_recent_password_change( client, dummy_user, dummy_group, token_for_dummy_user, patched_lock_active ): - ipa = untouched_ipa_client(current_app) + ipa = untouched_ipa_client(current_app, session) ipa.change_password("dummy", "dummy_password", "dummy_password") result = client.get(f'/forgot-password/change?token={token_for_dummy_user}') patched_lock_active["delete"].assert_called_once() diff --git a/tests/unit/security/test_ipa.py b/tests/unit/security/test_ipa.py index ddbb96774..ebbd51cd0 100644 --- a/tests/unit/security/test_ipa.py +++ b/tests/unit/security/test_ipa.py @@ -8,6 +8,7 @@ from noggin.app import ipa_admin from noggin.security.ipa import ( + choose_server, Client, maybe_ipa_login, maybe_ipa_session, @@ -23,6 +24,30 @@ def ipa_call_error(): } +def test_choose_server(client, mocker): + random = mocker.patch("noggin.security.ipa.random") + random.choice.side_effect = ["a.example.test", "b.example.test", "c.example.test"] + with client.session_transaction() as sess: + server = choose_server(current_app, sess) + assert server == "a.example.test" + with client.session_transaction() as sess: + server = choose_server(current_app, sess) + # After a second call it is still the first result + assert server == "a.example.test" + random.choice.assert_called_once() + + +def test_choose_server_no_session(client, mocker): + random = mocker.patch("noggin.security.ipa.random") + random.choice.side_effect = ["a.example.test", "b.example.test", "c.example.test"] + server = choose_server(current_app) + assert server == "a.example.test" + server = choose_server(current_app) + # If we can't store the value in the session, we'll call random.choice again. + assert server == "b.example.test" + assert random.choice.call_count == 2 + + @pytest.mark.vcr def test_ipa_session_authed(client, logged_in_dummy_user): """Check maybe_ipa_session() when a user is logged in""" @@ -74,10 +99,9 @@ def test_ipa_login(client, dummy_user): def test_ipa_untouched_client(client): with client.session_transaction() as sess: - ipa = untouched_ipa_client(current_app) + ipa = untouched_ipa_client(current_app, sess) assert ipa is not None assert 'noggin_session' not in sess - assert 'noggin_ipa_server_hostname' not in sess assert 'noggin_username' not in sess