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.

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Improve SHA* marking #554

wants to merge 23 commits into from

Conversation

koplas
Copy link
Contributor

@koplas koplas commented Jul 25, 2024

@JanHoefelmeyer Can you check if all requirements of #289 (comment) are met?

Closes #289

Copy link
Contributor

@s-l-teichmann s-l-teichmann left a 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.

@tschmidtb51
Copy link
Collaborator

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?

@koplas
Copy link
Contributor Author

koplas commented Aug 10, 2024

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.

Copy link
Contributor

@JanHoefelmeyer JanHoefelmeyer left a 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

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
@koplas koplas mentioned this pull request Nov 27, 2024
@tschmidtb51
Copy link
Collaborator

I wonder whether the comment of just evaluating hashes if they look like hashes is implemented here?

@koplas
Copy link
Contributor Author

koplas commented Dec 19, 2024

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:

return util.HashFromReader(res.Body)

Copy link
Contributor

@JanHoefelmeyer JanHoefelmeyer left a 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")
Copy link
Contributor

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).

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve SHA* marking
6 participants