search

brave-intl / bat-go

Pass "go", collect 200 BAT
Mozilla Public License 2.0
42 stars 31 forks source link

Add integration tests for nitro enclave attestation verification #2366

Open kdenhartog opened 7 months ago

kdenhartog commented 7 months ago

https://github.com/brave-intl/bat-go/blob/nitro-payments-dev/libs/nitro/attestation.go#L159

kdenhartog commented 2 months ago

This one is good for overall code health to make sure that the CLI tooling is performing all the expected checks automatically to give us greater assurance that we're able to detect malicious activity if it were to occur. I'm going to mark this as blocking since it's testing a crucial security check that we don't want to fail.

ibukanov commented 1 month ago

@kdenhartog As a part of localdev branch I have added an option to bypass nitro API calls to be able to run the code locally.

It principle it can allow to write a unit tests or even integration tests to verify that the code behaves correctly. This assumes that bypassed Nitro calls do what they are supposed to do and Amazon has tested the library thoroughly. Or would you rather have an option to do integration tests against the real Nitro enclave?

kdenhartog commented 1 month ago

nitrite is not maintained by AWS and also doesn't contain tests. Therefore, we'd effectively be deferring critical security logic to others without certainty that it's meeting the requirements of our usage.

IIRC, the reason I asked for this came back to our usage of this within the CLI. In some cases, when we were trying to fix things to get them working we ended up just commenting this out because the PCR value checks kept on misbehaving. This meant that we inadvertently just disabled security checks that we needed.

Looking at this again, I do think the better answer here would be to do integration tests against the real nitro enclaves using the CLI tooling. This way we can be certain that if we're disabling something we are intentional about it, but also don't conflict with the localdev option that you recently added which is more useful for quick testing and updates to the application layer which aren't really relevant to whether nitro passes. Instead, I'd expect if these checks don't pass the tool just fully errors out which is better tested as an integration test anyways rather than as a specific unit test.

Side note as I was reviewing what this issue was about. @pavelbrm also left some good comments on the old PR about changes that should be made in this library, but I'm not sure if an issue got opened for these. One such example is this: https://github.com/brave-intl/bat-go/pull/1691/files#r1590715197