-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
Improve SHA* marking #554
base: main
Are you sure you want to change the base?
Improve SHA* marking #554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I like the approach with the extra HEAD requests.
@s-l-teichmann I didn't fully check the code. Where was the extra request? |
I removed it after the feedback of @s-l-teichmann. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell the checker and downloader still need to give out INFO if a SHA is missing in both ROLIE-feed and at all
This option was in conflict with the help display.
commit 990c74a Merge: 86d7ce1 7824f3b Author: koplas <pschwabauer@intevation.de> Date: Fri Nov 22 16:58:46 2024 +0100 Merge branch 'sha-handling' into unittest commit 86d7ce1 Merge: a6807d2 79b8900 Author: koplas <pschwabauer@intevation.de> Date: Fri Nov 22 16:54:45 2024 +0100 Merge branch 'sha-handling' into unittest commit 79b8900 Author: koplas <pschwabauer@intevation.de> Date: Fri Nov 22 16:31:56 2024 +0100 Improve hash fetching and logging commit a6807d2 Merge: ddb5518 d18d2c3 Author: koplas <pschwabauer@intevation.de> Date: Fri Nov 22 16:51:55 2024 +0100 Merge branch 'sha-handling' into unittest commit d18d2c3 Author: koplas <pschwabauer@intevation.de> Date: Fri Nov 22 16:31:56 2024 +0100 Improve hash fetching and logging commit ddb5518 Author: koplas <54645365+koplas@users.noreply.github.com> Date: Tue Sep 17 10:45:25 2024 +0200 Extend SHA marking tests commit 13c94f4 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 20:46:31 2024 +0200 Use temp directory for downloads commit 1819b48 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 20:37:55 2024 +0200 Fix rolie feed commit 989e366 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 20:23:22 2024 +0200 Fix provider-metadata.json commit 714735d Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 20:08:21 2024 +0200 Implement provider handler commit d488e39 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 16:26:37 2024 +0200 Add info about gpg key commit a9bf9da Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 16:12:49 2024 +0200 Rename directory testdata commit 6ca6dfe Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 16:01:41 2024 +0200 Add initial downloader tests commit 20bee79 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 15:58:31 2024 +0200 Fix: Remove unecessary error print commit 8e4e508 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 14:50:48 2024 +0200 Extend links test commit 3ba29f9 Author: koplas <pschwabauer@intevation.de> Date: Mon Sep 16 14:11:14 2024 +0200 Add initial directory feed testdata commit dee55aa Author: koplas <54645365+koplas@users.noreply.github.com> Date: Mon Sep 16 10:47:32 2024 +0200 Add initial testdata commit cd9338a Author: koplas <54645365+koplas@users.noreply.github.com> Date: Thu Sep 12 15:54:42 2024 +0200 Add initial download unittests
Changing the ROLIE category fetching warning to info can be addressed later.
Allow to forbid individual hashes from downloading. This allows to for testing the behavior, if one of the hashes could not be downloaded.
I wonder whether the comment of just evaluating hashes if they look like hashes is implemented here? |
This part was untouched in this pull request: csaf/cmd/csaf_checker/processor.go Line 785 in d8e9035
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM & works
} else { | ||
s256 = sha256.New() | ||
writers = append(writers, s256) | ||
slog.Info("SHA512 not present") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing this change :).
One feedback from user side:
If I read it correctly the csaf downloader will still emit a log message per file.
If a provider decides to only provide checksums in sha256, this would massively spam the info channel.
From CSAF Speicifiaction: https://docs.oasis-open.org/csaf/csaf/v2.0/os/csaf-v2.0-os.html#7118-requirement-18-integrity
All CSAF documents SHALL have at least one hash file computed with a secure cryptographic hash algorithm (e.g. SHA-512 or SHA-3) to ensure their integrity.
I understand that it is perfectly fine to provide only one of the sha256
and sha512
checksums. (Hence this is no longer a warning). Would it be maybe better to log this on the debug channel?
Currently for me the logs of the csaf downloader are quite hard to read, if a provider does not give both types of checksums. This would be still the same then, except the log level is set to warn
. But this would then not print the very useful summary of download statistics in the end (which is done on log channel info
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this output to the debug level sounds reasonable. Pinging @tschmidtb51 for opinions.
Regarding the message collapsing: In several cases, messages can be merged to avoid spam output. As this pull request is already large enough, and other areas would also benefit from the message merging, I would prefer to handle this in a separate pull request.
@JanHoefelmeyer Can you check if all requirements of #289 (comment) are met?
Closes #289