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

Fix missing error messages from loading provider-metadata.json #531

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

mgoetzegb
Copy link
Contributor

What

Fix: don't drop error messages from loading provider-metadata.json

Additionally removed the duplicate check of provider metadata candidates retrieved from security.txt.

Why

Previously in case of trying last resort dns, all other error messages were dropped.

@tschmidtb51
Copy link
Collaborator

@mgoetzegb Thank you for your contribution. Unfortunately, I'm currently failing to understand the issue. Could you please open an issue describing the steps to reproduce as well as current results and what the changed code would generate as outcome? Please also link the issue to your PR.
Many thanks in advance!

@mgoetzegb
Copy link
Contributor Author

mgoetzegb commented Apr 29, 2024

Sorry for the delay, did not find time last week. I now added an issue with steps to reproduce and the change with this PR.

@koplas koplas self-requested a review June 28, 2024 12:57
Copy link
Contributor

@koplas koplas left a comment

Choose a reason for hiding this comment

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

I prefer a distinction between error and debug messages. For example, TLS errors should be logged as errors, not debug messages. The user should be able to see the relevant errors without setting the log level to debug.

An example output should look like:

{"time":"2024-04-26T15:26:29Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://sick.com/.well-known/csaf/provider-metadata.json"}
{"time":"2024-04-26T15:26:30Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://sick.com/.well-known/security.txt"}
{"time":"2024-04-26T15:26:30Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://sick.com/security.txt"}
{"time":"2024-04-26T15:26:30Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://csaf.data.security.sick.com"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"fetching \"https://sick.com/.well-known/csaf/provider-metadata.json\" failed: Get \"https://sick.com/.well-known/csaf/provider-metadata.json\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"Fetching \"https://sick.com/.well-known/security.txt\" failed: Get \"https://sick.com/.well-known/security.txt\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"Fetching \"https://sick.com/security.txt\" failed: Get \"https://sick.com/security.txt\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"fetching \"https://csaf.data.security.sick.com\" failed: Get \"https://csaf.data.security.sick.com\": dial tcp 203.0.113.1:443: connect: connection timed out"}

If a valid provider-metadata.json was found, the fetching errors should be displayed as debug messages.

@koplas koplas requested a review from JanHoefelmeyer June 28, 2024 13:38
mgoetzegb added 3 commits July 1, 2024 10:28
previously in case case of trying last resort dns, all other error messages were dropped
@mgoetzegb mgoetzegb force-pushed the fix-pmd-error-messages branch from 7d4ce5d to f004c47 Compare July 15, 2024 08:54
@mgoetzegb
Copy link
Contributor Author

mgoetzegb commented Jul 15, 2024

I prefer a distinction between error and debug messages. For example, TLS errors should be logged as errors, not debug messages. The user should be able to see the relevant errors without setting the log level to debug.

An example output should look like:

{"time":"2024-04-26T15:26:29Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://sick.com/.well-known/csaf/provider-metadata.json"}
{"time":"2024-04-26T15:26:30Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://sick.com/.well-known/security.txt"}
{"time":"2024-04-26T15:26:30Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://sick.com/security.txt"}
{"time":"2024-04-26T15:26:30Z","level":"DEBUG","msg":"http","who":"downloader","method":"GET","url":"https://csaf.data.security.sick.com"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"fetching \"https://sick.com/.well-known/csaf/provider-metadata.json\" failed: Get \"https://sick.com/.well-known/csaf/provider-metadata.json\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"Fetching \"https://sick.com/.well-known/security.txt\" failed: Get \"https://sick.com/.well-known/security.txt\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"Fetching \"https://sick.com/security.txt\" failed: Get \"https://sick.com/security.txt\": tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"time":"2024-04-26T15:31:00Z","level":"ERROR","msg":"Loading provider-metadata.json","domain":"sick.com","message":"fetching \"https://csaf.data.security.sick.com\" failed: Get \"https://csaf.data.security.sick.com\": dial tcp 203.0.113.1:443: connect: connection timed out"}

If a valid provider-metadata.json was found, the fetching errors should be displayed as debug messages.

This requires an adjustment in the calling code, i.e. in the different components.

I rebased the branch and added this in commit f004c47.

Adjustments for csaf_aggregator and csaf_downloader:

If the provider metadata json is invalid, then error messages from loading the provider metadata json are printed on log level error. Otherwise they are printed on level debug if the verbose option is set.

I'm not sure why those separate verbose options exist in the first place, given a leveled logger is used, but that is another topic I think.

The ProviderMetadataLoader is also used in the csaf_cecker, but there the error messages are already unconditionally printed on log level warn, not sure if that is desired. But given I'm not familiar with this component at all, I left it untouched for now.

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

@koplas koplas merged commit 1e531de into gocsaf:main Jul 15, 2024
3 checks passed
@mgoetzegb mgoetzegb deleted the fix-pmd-error-messages branch July 15, 2024 12:49
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.

4 participants