search

micronaut-projects / micronaut-core

Micronaut Application Framework
http://micronaut.io
Apache License 2.0
6.05k stars 1.06k forks source link

Yaml Errors loading security secrets #5388

Closed jonnii closed 3 years ago

jonnii commented 3 years ago

While trying to upgrade to 2.5.1 from 2.4.2 our test suite fails to load our application.yml from test resources which we use to override a variety of options, most notably the jwt secret settings.

We believe the part of the config causing the issue is:

    token:
      roles-name: scope
      jwt:
        bearer:
          headerName: Authorization

        signatures:
          secret:
            generator:
              secret: 2384j2938j3981j3291823jdsalkjd918
              jws-algorithm: HS256

An example of the stack trace caused is as follows:

   java.lang.ArrayIndexOutOfBoundsException: Index 163 out of bounds for length 3
        at org.yaml.snakeyaml.reader.StreamReader.update(StreamReader.java:199)
        at org.yaml.snakeyaml.reader.StreamReader.ensureEnoughData(StreamReader.java:176)
        at org.yaml.snakeyaml.reader.StreamReader.ensureEnoughData(StreamReader.java:171)
        at org.yaml.snakeyaml.reader.StreamReader.peek(StreamReader.java:126)
        at org.yaml.snakeyaml.scanner.ScannerImpl.scanToNextToken(ScannerImpl.java:1177)
        at org.yaml.snakeyaml.scanner.ScannerImpl.fetchMoreTokens(ScannerImpl.java:287)
        at org.yaml.snakeyaml.scanner.ScannerImpl.checkToken(ScannerImpl.java:227)
        at org.yaml.snakeyaml.parser.ParserImpl$ParseImplicitDocumentStart.produce(ParserImpl.java:195)
        at org.yaml.snakeyaml.parser.ParserImpl.peekEvent(ParserImpl.java:158)
        at org.yaml.snakeyaml.parser.ParserImpl.checkEvent(ParserImpl.java:148)
        at org.yaml.snakeyaml.composer.Composer.checkNode(Composer.java:82)
        at org.yaml.snakeyaml.constructor.BaseConstructor.checkData(BaseConstructor.java:123)
        at org.yaml.snakeyaml.Yaml$1.hasNext(Yaml.java:489)
        at io.micronaut.context.env.yaml.YamlPropertySourceLoader.processInput(YamlPropertySourceLoader.java:75)
        at io.micronaut.context.env.AbstractPropertySourceLoader.read(AbstractPropertySourceLoader.java:117)
        at io.micronaut.context.env.AbstractPropertySourceLoader.loadProperties(AbstractPropertySourceLoader.java:102)
        at io.micronaut.context.env.AbstractPropertySourceLoader.load(AbstractPropertySourceLoader.java:68)
        at io.micronaut.context.env.AbstractPropertySourceLoader.load(AbstractPropertySourceLoader.java:55)
        at io.micronaut.context.env.DefaultEnvironment.loadPropertySourceFromLoader(DefaultEnvironment.java:574)
        at io.micronaut.context.env.DefaultEnvironment.readPropertySourceList(DefaultEnvironment.java:504)
        at io.micronaut.context.env.DefaultEnvironment.readPropertySourceList(DefaultEnvironment.java:490)

Additionally we have noted that running a version of 2.5.0 with #5306 (Init/Bean context performance improvements) reverted does not have the same problem. As an additional note a similar change to the Yaml constructor was reverted previously https://github.com/micronaut-projects/micronaut-core/commit/30c3a1480f529734a56db4184ad77ca4a805130d#diff-443c370ab31590ea9d58cfa3682e9cd761a2554511e2b4665212a9fc88f9ff3e.

The line in question in the above diff is the change:

new Yaml(YAML_CONSTRUCTOR)

Task List

Environment Information

Please let me know if there's any additional information that would be useful to provide to help pin this down.

jonnii commented 3 years ago

Updated to note that this still persists with 2.5.1.

dstepanov commented 3 years ago

We probably need to remove that SafeConstructor

graemerocher commented 3 years ago

I'm unsure we should, there are open CVEs related to using SnakeYAML without it https://securitylab.github.com/research/swagger-yaml-parser-vulnerability/

jonnii commented 3 years ago

@graemerocher is there a work around you can suggest?

graemerocher commented 3 years ago

Can you provide an example that reproduces the issue?

jameskleeh commented 3 years ago

@jonnii The snippet you included parses OK. Need an example to pursue this further

jonnii commented 3 years ago

I've been working on creating a reproduction and in the process I've realized that my initial thoughts are/were a red herring. At this point I'm trying to narrow it down.

jonnii commented 3 years ago

Ok I've narrowed down the problem, turns out it's nothing like what I originally thought it was. We do the following as part of our tests to make them run in parallel:

tasks.withType(Test) {
    systemProperty("junit.jupiter.execution.parallel.enabled", true)
    systemProperty("junit.jupiter.execution.parallel.mode.default", "concurrent")
    systemProperty("kotest.framework.parallelism", "4")   // <-- this is what causes the issue
}

Removing the kotest framework parallelization makes everything work fine, which is a little surprising.

njimenezotto commented 3 years ago

The same happens to me with the property junit.jupiter.execution.parallel.enabled = true

graemerocher commented 3 years ago

Somebody needs to provide an example with steps to reproduce

njimenezotto commented 3 years ago

Here you will find an example https://github.com/noeliajimenezg/micronaut-issue-5388 If you comment one of the 2 test classes, the tests run fine but if you want to run both classes, it throws the error.