Conversation
e8d3b69 to
ecd73a6
Compare
|
The failed phpcs tests are not due to my changes. |
I'm afraid I have to convert this PR to draft in this case.
Well, the current master is all-✅. |
| CREATE TABLE `icingaweb_totp`( | ||
| `username` varchar(254) COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| `secret` varchar(255) NOT NULL, | ||
| `ctime` timestamp NULL DEFAULT NULL, |
There was a problem hiding this comment.
Why NULL here? From a high-level PoV, 2FA entries always have a creation time.
There was a problem hiding this comment.
I would simply stick to how all other ctime columns are created in the schema.
There was a problem hiding this comment.
"all other ctime columns" are timestamp, yours a bigint.
There was a problem hiding this comment.
Like we discussed offline I left them bigint and set to NOT NULL.
faa2b5f to
62f1087
Compare
Al2Klimov
left a comment
There was a problem hiding this comment.
Tbh, I as a reviewer don't wanna even think about whose (branch) fault the red GHA is.
Just (someone) get it green, please. Thanks.
(Notes for the future myself:
Test system (I use NixOS btw)
{ pkgs, ... }: {
imports = [
./hardware-configuration.nix
];
boot.tmp.cleanOnBoot = true;
networking.hostName = "aklimov-2fa";
networking.domain = "";
services.openssh.enable = true;
users.users.root.openssh.authorizedKeys.keys = [''ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBIroHYGSRaRNFxlK90SS0aHwWjEME30pK5J1N/V1w6a aklimov@ws-aklimov7777777.local'' ];
system.stateVersion = "23.11";
networking.firewall.allowedTCPPorts = [ 80 ];
virtualisation.oci-containers = {
backend = "podman";
containers.postgresql = {
autoStart = true;
image = "postgres:9.1";
environment = {
POSTGRES_PASSWORD = "123456";
};
ports = [ "127.0.0.1:5432:5432" ];
volumes = [ "postgres:/var/lib/postgresql/data" ];
};
};
services.icingaweb2 = {
enable = true;
#virtualHost = "iw2.aklimov.net-dump.de";
generalConfig.global.config_resource = "iw2";
modules.monitoring.enable = false;
authentications = {
pg = {
backend = "db";
resource = "iw2";
};
};
resources.iw2 = {
type = "db";
db = "pgsql";
host = "127.0.0.1";
port = "5432";
dbname = "postgres";
username = "postgres";
password = "123456";
charset = "utf8";
};
roles = {
adm = {
users = "icingaadmin,Alexander A. Klimov";
permissions = "*";
};
};
modulePackages = {
};
};
nixpkgs.overlays = [
(_: prev: {
icingaweb2-ipl = prev.icingaweb2-ipl.overrideAttrs (old: {
src = prev.fetchFromGitHub {
owner = "Icinga";
repo = "icinga-php-library";
rev = "v0.18.1";
hash = "sha256-V2PkRtVzMnzBpNLUMsSl5NUSnrycgEHGXLGilbhcGfw=";
};
}); })
(_: prev: {
icingaweb2-thirdparty = prev.icingaweb2-thirdparty.overrideAttrs (old: {
src = prev.fetchFromGitHub {
owner = "Icinga";
repo = "icinga-php-thirdparty";
#rev = "274212c7e02223687eacdfbc78c48181878ac910"; # feature/2fa
#hash = "sha256-K+NRUDnQjGG0Lmi5AOtftfueZM+VE6qkOWXF9qLzi/Y=";
rev = "1983c72cd3bbc0f4f0df5f464db4985417b4e73d"; # aklimov/feature/2fa
hash = "sha256-3CIAfhSr9GGhytOPnSfnnt/H6xWJOlQzyrZpux0VZbI=";
};
}); })
(_: prev: { icingaweb2 = prev.icingaweb2.overrideAttrs (old: {
src = prev.fetchFromGitHub {
owner = "Icinga";
repo = "icingaweb2";
#rev = "62f10871e9e270789def9a3776656ff15d094013"; # 2fa-new
#hash = "sha256-iCoCCEGLO3vf9VdbDWmecBMaRuUAKJc7f9AeNKLQv0A=";
rev = "e0e2a22fbeb90ae721b5d301af235467aa22d3ef"; # aklimov/2fa-new
hash = "sha256-0SaOkROWkc20j7MxGWsx5q4aSQoRTDaQazd2SJjCPbU=";
#rev = "70f29827f921f24b2afc0556c4800dfa55a2d0c1"; # main
#hash = "sha256-fCd5kyJk7LXdKkZS7kppPO1wfeNTOD4esliqvSAtyV4=";
};
patches = [
./opcache_reset.patch
# https://github.com/Icinga/icingaweb2/issues/5427
./migrations-db-no-pw.patch
];
# https://github.com/NixOS/nixpkgs/pull/380065
installPhase = old.installPhase + "\ncp -ra schema $out";
}); })
];
}| use Icinga\Forms\PreferenceForm; | ||
| use Icinga\User\Preferences\PreferencesStore; | ||
| use Icinga\Web\Controller; | ||
| use ipl\Html\Contract\Form; |
There was a problem hiding this comment.
Heads up! ipl\Html\Contract\Form was added recently to ipl, so our icingaweb2 2.13.0(?) packages MUST raise their dependencies. I didn't check all classes, though.
There was a problem hiding this comment.
| @@ -0,0 +1,6 @@ | |||
| CREATE TABLE "icingaweb_2fa" ( | |||
There was a problem hiding this comment.
While I can login in order to apply this migration, /account says SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "icingaweb_2fa" does not exist until applied.
Can't speak for others, but I personally could imagine just doing nothing here – i.e let the users fix it by applying the migration.
There was a problem hiding this comment.
Yes, I also would let the user fix it by applying the migration.
| * | ||
| * This form is used to manage the 2FA settings of a user account. | ||
| */ | ||
| class TwoFactorConfigForm extends CompatForm |
There was a problem hiding this comment.
I was barely able to integrate Icinga/icinga-php-thirdparty#53 for testing this.
composer updateon the PR saidThe process "patch '-p1' --no-backup-if-mismatch -d '/Users/aklimov/NET/WS/icingaweb2/icinga-php-thirdparty/vendor/shardj/zf1-future' < '/Users/aklimov/NET/WS/icingaweb2/icinga-php-thirdparty/patches/shardj-zf1-future.patch'" exceeded the timeout of 300 seconds.composer require spomky-labs/otphp chillerlan/php-qrcodeon v0.14.0 missed the class for QR codes, so I had to comment those out
Please:
- Get Add dependencies for two-factor authentication in Icinga Web icinga-php-thirdparty#53 ready
- Make me a commit based on it which contains vendor/, so I can check it out just like a release tag
There was a problem hiding this comment.
Done. Try to check out to 2686bdc3a0761519a4ffe432086a10cb19be921d, or just pull Icinga/icinga-php-thirdparty#53.
| * | ||
| * This form is used to manage the 2FA settings of a user account. | ||
| */ | ||
| class TwoFactorConfigForm extends CompatForm |
| * Form for user authentication | ||
| */ | ||
| class LoginForm extends Form | ||
| class LoginForm extends CompatForm |
There was a problem hiding this comment.
Heads up, security hole! On Postgres, I can bypass 2FA for e.g icingaadmin by logging in as ICINGAADMIN.
There was a problem hiding this comment.
@nilmerg FYI same applies to user preferences. (Not a security hole, though.) I.e ICINGAadmin and icingaADMIN have distinct user preferences.😅
Al2Klimov
left a comment
There was a problem hiding this comment.
Almost forgot! The commit history got out of hand. Consider making one or a few large commits with shared authorship where applicable:
Al2Klimov
left a comment
There was a problem hiding this comment.
@lippserd Please write your requirements on 2FA + API requests.
- They just shall not work? (What a pity.)
- They just shall work with user:pass? (Security hole, our forms already allow API requests implicitly!)
- OTP header required?
- OTP is to be appended to the password? (
username:password;otp) - ...
|
... or personal access token? (idea (c) @TheSyscall) |
699b6bf to
0b5ed2c
Compare
99bcba9 to
1d8d851
Compare
Replace `assembleVerificationForm()` on the `TwoFactor` interface with a generic `assembleTwoFactorElements()` method on `LoginForm` itself. The token input, verify button, and cancel button are now owned by the login form rather than each hook implementation. Remove the `TOKEN_INPUT`, `SUBMIT_VERIFY_2FA`, and `SUBMIT_CANCEL_2FA` constants from the `TwoFactor` interface and define them directly on `LoginForm`. Update `AuthenticationController` to reference the new location.
Extract the login page structure (logo, footer, social links, decorative orbs) from `login.phtml` into a reusable ipl-html widget. Extending `HtmlDocument` rather than `BaseHtmlElement` lets it emit `#login` and the seven `.orb` sibling divs without a wrapping root tag. Both the login action and the upcoming 2FA challenge action can now share the same visual scaffolding without duplicating markup and without using view scripts anymore.
Split the two-factor challenge out of `LoginForm` into its own standalone form which handles token verification, remember-me cookie persistence, and the cancel flow that purges the temporary session and returns to login.
Convert `AuthenticationController` from the legacy Zend Controller to `CompatController`, dropping `login.phtml` in favour of the new `LoginPage` widget and `addContent()`. The view-variable assignments are replaced by `setTitle()` and `addContent(new LoginPage(...))`. In `CompatController`, `$this->controls` is the tab bar area rendered above the page content. When no tabs are added it still emits an empty `<div class="controls">` wrapper. Replacing it with a bare `HtmlDocument` which renders as nothing when empty suppresses that wrapper entirely, keeping the login page markup clean. Two structural fixes required by the changed DOM nesting: - `login.less`: height `100%` -> `100vh` (`#login` is now inside `.content` which has no explicit height, so percentage inheritance breaks) - `history.js`: `#layout > #login` -> `#layout #login` (direct-child selector breaks because `#login` is now a grandchild of `#layout` through `.content`)
`LoginForm` is now responsible only for the first authentication factor. After a successful credential check, users with 2FA enrolled are redirected to `authentication/twofactor` where `TwoFactorChallengeForm` takes over. Remove `assembleTwoFactorElements()`, the `SUBMIT_VERIFY_2FA` / `SUBMIT_CANCEL_2FA` / `TOKEN_INPUT` constants, the `getResponse()` helper, and the `2fa_must_challenge` session branch from `assemble()`. The `onSuccess()` switch-on-button is replaced with a simple login-only handler. Also removes the `ON_SENT` handler from `AuthenticationController` that handled the cancel button. It referenced `SUBMIT_CANCEL_2FA` which is removed here, and the responsibility moves to `TwoFactorChallengeForm` in the next commit.
Add a dedicated action for the 2FA verification step. The action guards against direct access without a pending authentication, renders `TwoFactorChallengeForm` inside the same `LoginPage` scaffold, and redirects to the originally requested URL after successful verification. `loginAction()` gains a matching guard: if a 2FA session is already in progress (e.g. the user navigates back), it redirects straight to the challenge instead of showing the login form again.
corresponding CSS to use the correct spelling `#orb-notifications`.
Introduce `TwoFactorState` as a dedicated wrapper around the session namespace used during the two-factor authentication flow, replacing the scattered raw session key accesses in `LoginForm`, `TwoFactorChallengeForm`, and `AuthenticationController`. `challenge()` opens the flow by storing the temporary user, `completeChallenge()` closes it by cleaning up both the user and the pending remember-me cookie, and `isChallenged()` provides a single readable guard for the controller actions.
Replace the flat `ValidHtml|array $formContent` parameter with typed `CompatForm $form` and `LoginButtonForm[] $loginButtons` parameters. This gives the widget semantic knowledge of what it renders and removes the `array_filter(array_merge(...))` workaround from the `AuthenticationController`.
Emit an info line when a user is challenged and when they pass verification, and a warning when the submitted token is rejected. All three log lines include the method name so the 2FA backend in use is always visible in the logs.
Basic Auth cannot complete a 2FA challenge, so users with 2FA enrolled would silently bypass it via the API. Detect enrollment in `authHttp()` and respond with **403** and a JSON error message.
Remove the duplicate constant and reference the one defined in LoginForm.
Rename constants and HTML element names to a consistent `twofactor_*/submit_*` scheme, drop the now unused `SUBMIT_LOGIN` constant from `LoginForm`, and update all references in the controllers.
Replace the repeated pattern of building a URL and conditionally forwarding the redirect param with a single protected method.
Replaces the cancel submit button on the 2FA challenge form with a minimal link-styled button showing an arrow-left icon and "Back to login" label. The new `.btn-back-to-login-link` style strips all button chrome (background, padding, fixed height) and only underlines the label text on hover, making it visually unobtrusive next to the primary verify button.
This PR is based on #5397 and will therefore close it. Introduces basic two-factor authentication (2FA) using Time-based One-Time Passwords (TOTP) support for Icinga Web 2.
TOTP (Time-based One-Time Password) is a type of two-factor authentication (2FA) method. It generates short-lived numeric codes (usually 6-8 digits) based on:
Since the code changes frequently and is only valid for a short window, it makes accounts harder to hack, even if someone steals your password.
After enabling 2FA, users authenticate with their usual Icinga Web credentials and are then prompted to provide a valid TOTP code from their authenticator app (e.g., Google Authenticator, Authy).
We will use the defaults of 6 digit tokens, 30 seconds interval and sha-1 algorithm for token generation. The generated token is valid for 10 seconds before and after the current time to allow some clock drift.
Current Authentication Process
Currently, Icinga Web authentication follows this workflow:
redirect.2FA Implementation
This implementation is split in two main parts.
Configuration
The configuration of TOTP-based 2FA is fully integrated into the user’s account settings. A new form (
TwoFactorConfigForm) allows users to enable 2FA, verify, or remove a TOTP secret directly from their account settings.When a user with the required permission (
user/two-factor-authentication) accesses their account, theAccountControllerloads the current TOTP secret from the database (TwoFactorTotp::loadFromDb()). If none exists, a new secret is generated (TwoFactorTotp::generate()) and passed into the form.The form (
TwoFactorConfigForm) then guides the user through the following workflow:TwoFactorTotp::createQRCode()) and a manual provisioning URI. The user can scan this code with an authenticator app or copy the URI via a copy-to-clipboard element.TwoFactorTotpclass. Only if the verification succeeds will the secret be persisted to the database (saveToDb()).removeFromDb()). (Should be extended in the future to ask for confirmation!)Authentication
Once a user has a TOTP secret stored, the login flow gains a second step. After the username and password are verified, the session sets a
2fa_must_challenge_tokenflag, stores a temporary user object, and if “Stay logged in” was checked, a temporary remember-me cookie in the session. The remember-me cookie is not persisted yet. This prevents bypassing 2FA by simply reusing the cookie.Auth::isAuthenticated()also enforces that a user is only considered fully logged in once the TOTP challenge has succeeded, unless explicitly skipped for trusted cookies.The
LoginFormis rewritten now extendingipl\Web\Compat\CompatFormand decides which elements it should render. If the2fa_must_challenge_tokenflag is set in the session, the elements to verify the TOTP token are displayed, otherwise the normal login form elements. A valid remember-me cookie can bypass both, the password login and the token challenge: if present and verified, the cookie is renewed, persisted, and the user is logged in.When the
LoginFormis submitted via thebtn_submit_verify_2fasubmit button, the secret is loaded from the database and the entered code is verified. A successful check sets$twoFactorSuccessfultotrueon the user object that was temporarily stored in the session after password login, clears the2fa_must_challenge_tokensession flag, persists any deferred remember-me cookie, and callsAuthenticationHook::triggerLogin(). Submitting the form via thebtn_submit_cancel_2fabutton lets users abort, which purges the session and resets the login.Future
Possible extensions for the future could be:
Requires GD php extension for QR code generation
Requires Icinga/icinga-php-thirdparty#53
Closes #5397