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

Empty rolie #357

Merged
merged 24 commits into from
Jun 30, 2023
Merged

Empty rolie #357

merged 24 commits into from
Jun 30, 2023

Conversation

JanHoefelmeyer
Copy link
Contributor

Solves #336

Csaf checker explicitely filters out if the schema error was no entries and will give a warning for this error only.

Csaf aggregator creates "empty" ROLIE feeds if no entries are present.

Csaf Provider already has an option to create ROLIE feeds during setup. This is currently not mandatory, however, but would be possible to make mandatory if needed.

Csaf Downloader will find feeds with emptry entry, but will not download anything from them.

@JanHoefelmeyer JanHoefelmeyer added this to the Release 2.2.0 milestone Apr 21, 2023
@JanHoefelmeyer JanHoefelmeyer linked an issue Apr 21, 2023 that may be closed by this pull request
cmd/csaf_checker/processor.go Outdated Show resolved Hide resolved
cmd/csaf_checker/processor.go Outdated Show resolved Hide resolved
cmd/csaf_aggregator/indices.go Outdated Show resolved Hide resolved
cmd/csaf_aggregator/indices.go Outdated Show resolved Hide resolved
@bernhardreiter bernhardreiter removed this from the Release 2.2.0 milestone Apr 21, 2023
Copy link
Collaborator

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments.

BTW: Shouldn't the change in the csaf_provider to generate the appropriated ROLIE feeds be part of this PR or do you want to have that separate?

csaf/schema/ROLIE_feed_json_schema.json Outdated Show resolved Hide resolved
cmd/csaf_aggregator/indices.go Outdated Show resolved Hide resolved
@JanHoefelmeyer JanHoefelmeyer marked this pull request as draft May 5, 2023 12:16
@JanHoefelmeyer JanHoefelmeyer marked this pull request as ready for review May 10, 2023 09:46
s-l-teichmann
s-l-teichmann previously approved these changes May 11, 2023
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.

LGTM

Copy link
Collaborator

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The creation of empty ROLIE feeds when setting up a provider (using csaf_uploader -a create) is not implemented (but it would make the test much easier).
  2. Using the aggregator with an empty ROLIE feed (e.g. create a provider, upload one document, delete all entries from the ROLIE feed) results in an error.

Happy to discuss the issues - maybe I misunderstood something

@JanHoefelmeyer JanHoefelmeyer marked this pull request as draft May 23, 2023 06:14
@mfd2007
Copy link
Collaborator

mfd2007 commented May 23, 2023

I can confirm that there is some problem with the aggregator. If the feed is empty, something doesn't work as expected:

csaf-feed-tlp-white.json.txt

2023/05/23 09:35:19 provider-metadata: https://acme.test/.well-known/csaf/provider-metadata.json
2023/05/23 09:35:19 AdvisoryFileProcessor.Process: Found 1 ROLIE feed(s).
....
2023/05/23 09:35:19 Feed URL: https://acme.test/.well-known/csaf/white/csaf-feed-tlp-white.json
....
2023/05/23 09:35:20 white: 0
....
2023/05/23 09:35:20 error: 'ACME' failed: open /data1/archive/ACME-2023-05-23-093519/white/changes.csv: no such file or directory

In that case the provider won't be listed in the aggregator.json. On the other hand, is this a real issue? Does an aggregator want to list an "empty" CSAF provider? Why should the aggregator do this?

According to the provider, I can't find the right option to create the ROLIE feed. For the different TLP-Level there won't be create any feeds.

@tschmidtb51
Copy link
Collaborator

Intended behavior:

  • csaf_provider: create empty ROLIE feeds as configured during setup (csaf_uploader -a create)
  • csaf_aggregator: create empty ROLIE feed where appropriate
  • csaf_downloader: "ignore" empty ROLIE feeds
  • csaf_checker: info (or warn?) on empty ROLIE feeds

@tschmidtb51
Copy link
Collaborator

I haven't located the bug in the aggregator why it is not creating a ROLIE feed with an empty feed array, but I'm happy to document that as it is consistent (doesn't show up in the aggregator.json either) - and leave that for now.

Documented in #377

s-l-teichmann
s-l-teichmann previously approved these changes Jun 22, 2023
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.

LGTM

mfd2007
mfd2007 previously approved these changes Jun 23, 2023
Copy link
Collaborator

@mfd2007 mfd2007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments. Additional remarks:

  1. I couldn't test the csaf_aggreagtorchange because List provider with empty feeds  #377. It fails as soon as you have a ROLIE feed with an empty entry array. See example error message: 'Example_Company_02' failed: open /var/www/aggregator-data/Example_Company_02-2023-06-23-222130/white/csaf-feed-tlp-white.json: no such file or directory.
  2. There is still some confusion regarding the error message and its meaning in the csaf_checker.
  3. The change in c495006 didn't fix the issue in the csaf_provider. The entry array is still empty when delivered.

cmd/csaf_provider/create.go Show resolved Hide resolved
cmd/csaf_checker/processor.go Outdated Show resolved Hide resolved
cmd/csaf_checker/processor.go Outdated Show resolved Hide resolved
@JanHoefelmeyer JanHoefelmeyer dismissed stale reviews from mfd2007 and s-l-teichmann via 7a63832 June 29, 2023 08:05
@tschmidtb51
Copy link
Collaborator

@JanHoefelmeyer #357 (comment) still exists - the other two have been resolved with the last 3 commits.

There are also merge conflicts...

Copy link
Collaborator

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge conflict resolution seems to be gone wrong - now any domain requested fails. I haven't found the line number yet where the issue is. But basically, it reports

2023/06/30 20:43:10 Failed to find valid provider-metadata.json for domain domain.tld. Continuing with next domain.
{
  "version": "2.1.1-123-g30c2218",
  "date": "2023-06-30T18:43:02.567470825Z"
}

for any valid domain

@s-l-teichmann
Copy link
Contributor

The merge conflict resolution seems to be gone wrong - now any domain requested fails. I haven't found the line number yet where the issue is. But basically, it reports

2023/06/30 20:43:10 Failed to find valid provider-metadata.json for domain domain.tld. Continuing with next domain.
{
  "version": "2.1.1-123-g30c2218",
  "date": "2023-06-30T18:43:02.567470825Z"
}

for any valid domain

@tschmidtb51 That was indeed a problem introduced by a faulty merge.
(Forgot to remove an else which should not be there)

I cleaned up the error handling in commit 89e69a7 .

@s-l-teichmann
Copy link
Contributor

s-l-teichmann commented Jun 30, 2023

I was able to run ./csaf_checker -v -o bsi.json bsi.de successfully

Copy link
Collaborator

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and works

@tschmidtb51
Copy link
Collaborator

@s-l-teichmann Thank you for the fast fix!

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.

LGTM

@s-l-teichmann s-l-teichmann merged commit b619124 into main Jun 30, 2023
@JanHoefelmeyer JanHoefelmeyer deleted the empty_ROLIE branch July 19, 2023 08:56
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.

Implement ROLIE feed allowing empty entries
5 participants