gocsaf / csaf

Tools to download or provide CSAF (Common Security Advisory Framework) documents.
https://csaf.io
40 stars 23 forks source link

CSAF checker: mixing domains, failing validations #522

Closed ctron closed 10 months ago

ctron commented 10 months ago

During the CSAF workshop one task was to use csaf_checker to download and validate CSAF documents. As we had multiple (more than 10) "domains", I was using the command like csaf_checker dns1 dns2 dns3 ….

That resulted in successful downloads, but failing validations for some domains, due to DNS in mismatches. Validating all domains with one domain each worked though.

It somehow feels like this is suffering from: https://go.dev/doc/faq#closures_and_goroutines … but maybe it is something different.

ctron commented 10 months ago

Unfortunately I lost the log files from the workshop :shrug: sorry for that.

s-l-teichmann commented 10 months ago

During the CSAF workshop one task was to use csaf_checker to download and validate CSAF documents. As we had multiple (more than 10) "domains", I was using the command like csaf_checker dns1 dns2 dns3 ….

That resulted in successful downloads, but failing validations for some domains, due to DNS in mismatches. Validating all domains with one domain each worked though.

It somehow feels like this is suffering from: https://go.dev/doc/faq#closures_and_goroutines … but maybe it is something different.

It is intended that the checker should reset its state after working on one domain. Maybe there is something that is not reset properly. It would be nice to have a actual list of domains we could check to reproduce the issue.

ctron commented 10 months ago

It somehow feels like this is suffering from: https://go.dev/doc/faq#closures_and_goroutines … but maybe it is something different.

And while Go puts that into the "closures and goroutines" area, this can also be triggered in more basic scenarios, like:

package main

func main() {
    a := []int{1, 2, 3}
    var b []*int

    for _, v := range a {
        b = append(b, &v)
    }

    for _, v := range a {
        print(v, " ")
    }
    println()
    for _, v := range b {
        print(*v, " ")
    }
    println()
}

And the code here feels a bit like that: https://github.com/csaf-poc/csaf_distribution/blob/6c8b3757aacef4e45d6fccf818a4218add03eed6/cmd/csaf_checker/processor.go#L249-L291

ctron commented 10 months ago

It is intended that the checker should reset its state after working on one domain. Maybe there is something that is not reset properly. It would be nice to have a actual list of domains we could check to reproduce the issue.

Sorry, I don't have the setup of the workshop anymore. That was as a set of .test domains with not much content.

s-l-teichmann commented 10 months ago

It somehow feels like this is suffering from: https://go.dev/doc/faq#closures_and_goroutines … but maybe it is something different.

And while Go puts that into the "closures and goroutines" area, this can also be triggered in more basic scenarios, like:

package main

func main() {
  a := []int{1, 2, 3}
  var b []*int

  for _, v := range a {
      b = append(b, &v)
  }

  for _, v := range a {
      print(v, " ")
  }
  println()
  for _, v := range b {
      print(*v, " ")
  }
  println()
}

And the code here feels a bit like that:

https://github.com/csaf-poc/csaf_distribution/blob/6c8b3757aacef4e45d6fccf818a4218add03eed6/cmd/csaf_checker/processor.go#L249-L291

I'm am pretty aware of this as this a long discussed flaw. I really would like to reproduce this issue before guessing about the reasons.

s-l-teichmann commented 10 months ago

Looking at the code https://github.com/csaf-poc/csaf_distribution/blob/6c8b3757aacef4e45d6fccf818a4218add03eed6/cmd/csaf_checker/processor.go#L290 does not get called if one of the continue cases above occurs. In these cases the processor does not get its reset. The fix would be trivial by moving the clean to the begin of the loop. But as I said: First I need a way to easily reproduce this.

ctron commented 10 months ago

But as I said: First I need a way to easily reproduce this.

I am sorry, I don't have one. But I think it should be easy to set one up with some wildcard DNS, cert and bash.

s-l-teichmann commented 10 months ago

I have created PR #523 to do the reset in any case. But this could only one reason for the issue. I assign this to @JanHoefelmeyer to check this when he will come back from his vacation.

@ctron: If you find the time to reproduce it you may check the PR if it helps.

s-l-teichmann commented 10 months ago

Re-opening it for further examination by @JanHoefelmeyer . Maybe gets closed again soon.

JanHoefelmeyer commented 10 months ago

I need a short clarification @ctron : Do you remember the exact error you got for failing domains?

Or whether the report was successful and a domain that should've passed via the checker was reported as faulty or whether the report couldn't be done? (Resulting in "Could not parse the Provider-Metadata.json of: 'insert domain name here'".)

ctron commented 10 months ago

Resulting in "Could not parse the Provider-Metadata.json of: 'insert domain name here'"

This sounds familiar. But I am not 100% sure, sorry.

JanHoefelmeyer commented 10 months ago

Okay, then since the only bugs I could reproduce are indeed those where the processor reset was skipped (fixed via https://github.com/csaf-poc/csaf_distribution/pull/523), I'll consider this solved for now as well.