feat: show alert about ioc scanner to all customers on upgrade #41137

IljaN · 2023-12-08T13:53:48Z

Description

Display ioc scanner instructions to all customers before upgrade (console as well as web updater) and in admin settings.
Community users will be take care of in future versions ... 😿

How Has This Been Tested?

Display message in upgrade

add license key (even an outdated is fine)
force upgrade by manually incrementing the version in version.php
run occ up
run upgrade in browser

Display alert in admin section

add license key (even an outdated is fine)
login as admin
go to admin settings

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

update-docs · 2023-12-08T13:53:52Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

jnweiger

CI build is failing in php-style step:

....................                                          2399 / 2399 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error
   1) core/templates/update.admin.php (braces, method_argument_space)
      ---------- begin diff ----------
--- /drone/src/core/templates/update.admin.php
+++ /drone/src/core/templates/update.admin.php
@@ -1,7 +1,7 @@
 
 <?php $license = \OC::$server->query(\OC\License\LicenseFetcher::class)->getOwncloudLicense();
 if ($license !== null) {
-?>
+	?>
 <div class="update">
 <?php include('ioc.warning.php'); ?>
 </div>
      ----------- end diff -----------

Found 1 of 2399 files that can be fixed in 58.306 seconds, 50.000 MB memory used
Detected deprecations in use:
- Rule "braces" is deprecated. Use "single_space_around_construct", "control_structure_braces", "control_structure_continuation_position", "declare_parentheses", "no_multiple_statements_per_line", "curly_braces_position", "statement_indentation" and "no_extra_blank_lines" instead.
make: *** [Makefile:222: test-php-style] Error 8

phil-davis · 2023-12-12T13:27:29Z

Works for me on my local dev setup with license key. The link takes me to the ioc-scanner script and PDF with instructions. It runs and produces "good" results (I have core master 10.13.4-prealpha).

When I go back to Settings-Admin-General on the webUI, it still shows me all the same "Critical Alert" in red stuff. Would there be some benefit for that screen to know when the scanner was last run, and display that date? Or even display the results summary as well?

phil-davis

Text looks OK.
Following the links from the webUI Settings-Admin-General section works.
Script works as per the PDF instructions for my local case.

DeepDiver1975 · 2023-12-12T13:43:04Z

Would there be some benefit for that screen to know when the scanner was last run, and display that date? Or even display the results summary as well?

There is no automated mechanism planned for this - especially because the tool needs to be executed on all servers. We have no understanding when this will be.
A specific config parameter can be added a scanned system to suppress the alert ......

jvillafanez · 2023-12-12T14:05:28Z

core/Command/Upgrade.php

+			$license = \OC::$server->query(LicenseFetcher::class)->getOwncloudLicense();
+			if ($license !== null) {
+				$output->setFormatter($defaultFormat);
+				$this->printAdvisory($output);
+			}


Using the ILicenseManager seems better. It can also be injected.

if ($licenseManager->getLicenseStateFor('core') !== ILicenseManager::LICENSE_STATE_VALID) { .... }

not sure if we want to check for additional states though.

We explicitly don't care about the license state. License set -> highly probably a customer -> alert them

jvillafanez · 2023-12-12T14:23:33Z

A specific config parameter can be added a scanned system to suppress the alert ......

I'd vote for this, even if it's just an acknowledgement. Something like "I've already applied the patch" and / or "I've read the message" might be good enough.
As long as it's clear that suppressing the message won't perform any validation, it's up to the admin to decide what to do with the message even if he hasn't done anything yet.

DeepDiver1975 · 2023-12-12T14:45:22Z

I'd vote for this, even if it's just an acknowledgement. Something like "I've already applied the patch" and / or "I've read the message" might be good enough.
As long as it's clear that suppressing the message won't perform any validation, it's up to the admin to decide what to do with the message even if he hasn't done anything yet.

https://github.com/owncloud/core/pull/41137/files#diff-94ffa1b0139a420b5ab07b770b526cfd9d2ba152610350bdfcba5d33971bbbe6R13-R14

jnweiger

We have a config.php variable that suppresses the messages.
We don't have anything automated that sets this variable after the ioc-scanner was run.
Thus I'd suggest to add a button "I have done this" in the admin view.

phil-davis · 2023-12-13T07:53:44Z

Thus I'd suggest to add a button "I have done this" in the admin view.

And for more feature bloat, maybe the config variable should be automatically reset on/after an upgrade. And maybe it should reset after some time period (maybe every 3 months the alert would "reappear"?) And maybe we don't want/need to design in all this sort of stuff.

More importantly, when there is a new/changed ios-scanner script available (that checks for more things, new things), how is that communicated? I suppose that the read-only public link will always have the latest ioc-scanner. But how will an admin know that they should download a newer version and run it?

DeepDiver1975 · 2023-12-13T07:57:31Z

We want no easy way to click this away.
It must be hard. Best case they reach out to support and support tells them what to do AFTER we analysed the reports

For the time being this will stay this way for 10.13.4. any change will come with future releases.

Time is key here..... THX

pako81 · 2023-12-13T08:41:59Z

We need to document the new config.php option indicators-of-compromise-scanner.confirmation (@mmattel) and we would also need changelog for this for the Release Notes.

mmattel · 2023-12-13T08:57:35Z

We need to document the new config.php option indicators-of-compromise-scanner.confirmation:

I guess that this is true/false. Defaults to false. With true making the message go away.

We should add this config.php change to this PR!

phil-davis · 2023-12-13T09:12:25Z

I guess that this is true/false. Defaults to false. With true making the message go away.

According to the code:

$ioc_exec_confirmation = \OC::$server->getConfig()->getSystemValue('indicators-of-compromise-scanner.confirmation', false);
if ($license !== null && $ioc_exec_confirmation !== 'EXECUTED_ON_ALL_NODES') {

To suppress the message, the config item has to be set to exactly the literal string "EXECUTED_ON_ALL_NODES".
Any other value (false, true, some other string, some number...) will not suppress it.

feat: display alert on web updater page feat: add alert to admin settings feat: config to disable alert in admin settings + admin settings styling

sonarqubecloud · 2023-12-13T10:39:53Z

Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 53%)

See analysis details on SonarCloud

Sorted out ....

) feat: display alert on web updater page feat: add alert to admin settings feat: config to disable alert in admin settings + admin settings styling

IljaN requested a review from pako81 December 8, 2023 13:59

pako81 requested review from jnweiger and phil-davis December 8, 2023 14:18

This comment was marked as off-topic.

Sign in to view

DeepDiver1975 requested a review from jnweiger December 11, 2023 11:02

jnweiger previously requested changes Dec 11, 2023

View reviewed changes

DeepDiver1975 force-pushed the feat/security-advisory branch from 5721849 to 9bd1ff2 Compare December 12, 2023 12:00

DeepDiver1975 changed the title ~~Display security advisory for latest CVEs~~ feat: show alert about ioc scanner to all customers on upgrade Dec 12, 2023

DeepDiver1975 marked this pull request as ready for review December 12, 2023 12:08

IljaN force-pushed the feat/security-advisory branch 2 times, most recently from 9bd1ff2 to 08f3d15 Compare December 12, 2023 12:56

DeepDiver1975 added the 3 - To Review label Dec 12, 2023

phil-davis requested a review from jnweiger December 12, 2023 13:31

phil-davis approved these changes Dec 12, 2023

View reviewed changes

jvillafanez reviewed Dec 12, 2023

View reviewed changes

DeepDiver1975 force-pushed the feat/security-advisory branch from 08f3d15 to 5f10dcb Compare December 12, 2023 16:11

jnweiger reviewed Dec 13, 2023

View reviewed changes

IljaN force-pushed the feat/security-advisory branch from 181b2e2 to 220c746 Compare December 13, 2023 09:59

feat(security): print security-advisory after successful upgrade

6818e47

feat: display alert on web updater page feat: add alert to admin settings feat: config to disable alert in admin settings + admin settings styling

IljaN force-pushed the feat/security-advisory branch from 220c746 to 6818e47 Compare December 13, 2023 10:14

DeepDiver1975 approved these changes Dec 13, 2023

View reviewed changes

DeepDiver1975 merged commit 36c9c00 into master Dec 13, 2023

delete-merged-branch bot deleted the feat/security-advisory branch December 13, 2023 11:00

DeepDiver1975 mentioned this pull request Jun 4, 2024

Revert "feat: show alert about ioc scanner to all customers on upgrade" #41260

Merged

feat: show alert about ioc scanner to all customers on upgrade #41137

feat: show alert about ioc scanner to all customers on upgrade #41137

Uh oh!

Conversation

IljaN commented Dec 8, 2023 • edited by DeepDiver1975 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Display message in upgrade

Display alert in admin section

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

update-docs bot commented Dec 8, 2023

Uh oh!

This comment was marked as off-topic.

Uh oh!

jnweiger left a comment

Choose a reason for hiding this comment

Uh oh!

phil-davis commented Dec 12, 2023

Uh oh!

phil-davis left a comment

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Dec 12, 2023

Uh oh!

jvillafanez Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

jvillafanez commented Dec 12, 2023

Uh oh!

DeepDiver1975 commented Dec 12, 2023

Uh oh!

jnweiger left a comment

Choose a reason for hiding this comment

Uh oh!

phil-davis commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented Dec 13, 2023

Uh oh!

pako81 commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmattel commented Dec 13, 2023

Uh oh!

phil-davis commented Dec 13, 2023

Uh oh!

sonarqubecloud bot commented Dec 13, 2023

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

IljaN commented Dec 8, 2023 •

edited by DeepDiver1975

Loading

phil-davis commented Dec 13, 2023 •

edited

Loading

pako81 commented Dec 13, 2023 •

edited

Loading