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

Missing fingerprint in PMD leads to an error #555

Closed
tschmidtb51 opened this issue Aug 6, 2024 · 9 comments · Fixed by #571
Closed

Missing fingerprint in PMD leads to an error #555

tschmidtb51 opened this issue Aug 6, 2024 · 9 comments · Fixed by #571

Comments

@tschmidtb51
Copy link
Collaborator

Today, I came across a weird report regarding OpenPGP stating:

Requirement 20: Public OpenPGP Key (failed)
    - ERROR: Fingerprint of public OpenPGP key https://example.com/.well-known/csaf/openpgp/4C071FE3584D1E7CD70FE8B2CF9436A769B3A45B.asc does not match remotely loaded.
    - INFO: No OpenPGP keys loaded.

In the same report, it says:

Requirement 19: Signatures
    - INFO: All signatures verified.

That does not make sense to me: Either the OpenPGP key couldn't be loaded (then one can't check signatures) or the OpenPGP key was loaded. Also, it is not quite clear to me what "Fingerprint of public OpenPGP key https://example.com/.well-known/csaf/openpgp/4C071FE3584D1E7CD70FE8B2CF9436A769B3A45B.asc does not match remotely loaded." mean. Where does the fingerprint come from?

(Sorry, I needed to redact some values for privacy concerns.)

@tschmidtb51 tschmidtb51 added service+dev investigation_needed This item needs investigation labels Aug 6, 2024
@bernhardreiter bernhardreiter self-assigned this Aug 7, 2024
@bernhardreiter
Copy link
Member

bernhardreiter commented Aug 7, 2024

The code compares that for a PMD section like this:

{
 "public_openpgp_keys": [
    {
      "fingerprint": "804FED63730227FF2FB6D9712EA2477380F3EDCB",
      "url": "https://intevation.de/.well-known/csaf/openpgp/804FED63730227FF2FB6D9712EA2477380F3EDCB.asc"
    }
  ]
}

the fingerprint calculated from the downloaded file from the URL matches the given string for "fingerprint":. The matching rules are "a general form of case-insensitivity" (https://pkg.go.dev/strings#EqualFold). A widely used implementation like GnuPG accepts more fingerprint formats. So this is a possible cause for the missmatch (but would not be allowed by the schema, see below). A further analysis should do the comparison manually.

@bernhardreiter
Copy link
Member

bernhardreiter commented Aug 7, 2024

The following patch adds more detail to the checker output:

diff --git a/cmd/csaf_checker/processor.go b/cmd/csaf_checker/processor.go
index 451a315..b0929a8 100644
--- a/cmd/csaf_checker/processor.go
+++ b/cmd/csaf_checker/processor.go
@@ -1519,7 +1519,7 @@ func (p *processor) checkPGPKeys(_ string) error {
                }
 
                if !strings.EqualFold(ckey.GetFingerprint(), string(key.Fingerprint)) {
-                       p.badPGPs.error("Fingerprint of public OpenPGP key %s does not match remotely loaded.", u)
+                       p.badPGPs.error("Given Fingerprint ('%s') of public OpenPGP key %s does not match remotely loaded ('%s').", string(key.Fingerprint), u, ckey.GetFingerprint())
                        continue
                }
                if p.keys == nil {

Edit: %q should be used instead of '%s' as minor improvement.

@bernhardreiter
Copy link
Member

bernhardreiter commented Aug 7, 2024

Looking at https://docs.oasis-open.org/csaf/csaf/v2.0/provider_json_schema.json

    "public_openpgp_keys": {
      "title": "List of public OpenPGP keys",
      "description": "Contains a list of OpenPGP keys used to sign CSAF documents.",
      "type": "array",
      "items": {
        "title": "PGP keys",
        "description": "Contains all information about an OpenPGP key used to sign CSAF documents.",
        "type": "object",
        "required": [
          "url"
        ],
        "properties": {
          "fingerprint": {
            "title": "Fingerprint of the key",
            "description": "Contains the fingerprint of the OpenPGP key.",
            "type": "string",
            "minLength": 40,
            "pattern": "^[0-9a-fA-F]{40,}$"
          },
          "url": {
            "title": "URL of the key",
            "description": "Contains the URL where the key can be retrieved.",
            "$ref": "#/$defs/url_t"
          }
        }
      }
    }

The schema does not seem to mandate the fingerprint, which is in contrast to Example 124 (see https://docs.oasis-open.org/csaf/csaf/v2.0/os/csaf-v2.0-os.html#717-requirement-7-provider-metadatajson) which is labled Minimal but contains a fingerprint value.

So either the Standard needs a small correction or the schema.

The checker acts more like the standard.

@bernhardreiter
Copy link
Member

The checker will check all signatures against the pubkeys provided in the PMD. So the message - INFO: All signatures verified. means that all found documents were verified fine against the pubkeys.

@tschmidtb51 are all questions answered for you?

As followup and part of this issue should we:

  • Consider adding a patch like the above one for more verbosity?
  • Report the standard contradiction from above to the TC?

@tschmidtb51
Copy link
Collaborator Author

tschmidtb51 commented Aug 7, 2024

@tschmidtb51 are all questions answered for you?

Yes. Thank you for the fast analysis.

As followup and part of this issue should we:

  • Consider adding a patch like the above one for more verbosity?

Yes. Please do so.

  • Report the standard contradiction from above to the TC?

Already done in oasis-tcs/csaf#764 (I took the liberty to copy your analysis.).

@tschmidtb51 tschmidtb51 added enhancement New feature or request and removed investigation_needed This item needs investigation labels Aug 7, 2024
@bernhardreiter bernhardreiter added csaf_checker csaf_downloader and removed enhancement New feature or request labels Aug 7, 2024
@bernhardreiter bernhardreiter changed the title Weird report regarding OpenPGP Missing fingerprint in PMD leads to an error Aug 7, 2024
@bernhardreiter
Copy link
Member

As oasis-tcs/csaf#764 says:

It must be an oversight, that the fingerprint is not required per schema.

we will turn a missing fingerprint into a warning.

So we have to change the implementation in two ways:
a) change error to warning
b) give more details (like patch above)

And checking that it works in all places where this is considered, at least checker and downloader.

koplas added a commit that referenced this issue Aug 8, 2024
Warn if no fingerprint is specified and give more details, if
fingerprint comparison fails.

Closes #555
koplas added a commit that referenced this issue Aug 8, 2024
Warn if no fingerprint is specified and give more details, if
fingerprint comparison fails.

Closes #555
@bernhardreiter
Copy link
Member

#558 is back to draft, as we need a version without API change for 3.x.
And a test is needed if an empty fingerprint is flagged as error correctly by the schema check. If the empty fingerprint is correctly flagged, then we would need no API change.

@bernhardreiter
Copy link
Member

@koplas thanks for doing the test in #558. The test shows that an empty fingerprint will already hit the schema test barrier.

Can you do a merge request that does not change the public API, but adds the additional details for a missing or unmatching fingerprint next? That would be a siginificant improvement for the analysis and we can release it with the next 3.x release.

We should then open a new issue with the wish to improve diagnostics when there is an empty fingerprint and discuss technical options there. One option is to see if we can get more details about the failed schema check to users.

@bernhardreiter bernhardreiter linked a pull request Sep 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants