feat: add support for proof of absence in certificate lookups #878

nathanosdev · 2024-04-29T14:27:22Z

Description

Adds support for proof of absence in certificate lookups. Previously a lookup with either return a value or undefined if the value cannot be found. Now the result of a lookup will differentiate between Found, Unknown and Absent, which allows clients to tell the difference between a value that is provably absent from the tree, or one that might have been pruned from the tree (maliciously or otherwise).

Fixes # SDK-1219

How Has This Been Tested?

I've added new unit tests to cover the new lookup functionality.

Checklist:

My changes follow the guidelines in CONTRIBUTING.md.
The title of this PR complies with Conventional Commits.
I have edited the CHANGELOG accordingly.
I have made corresponding changes to the documentation.

nathanosdev · 2024-04-29T14:36:28Z

@krpeacock it would be good to get your opinion on the interface here. I've kept it so that Certificate.lookup() has the same behaviour as before. To get the new lookup behaviour, clients would need to import the lookup_path() function.

Would you prefer to keep it like this or to propagate the new behaviour to the Certificate.lookup() function too?

It's also possible now to make the agent more robust by retrying requests when requested certificate entries are Unknown since that would imply the replica is acting maliciously. Retrying these requests would hit another (hopefully honest) replica and get a certificate that has the expected values. Examples are when we do lookups for time or request_status, but I think that would be best done in a follow up task.

krpeacock · 2024-05-01T21:12:27Z

packages/agent/src/agent/http/index.ts

        throw new Error('Time was not found in the response or was not in its expected format.');
      }

-      if (!(timeLookup instanceof ArrayBuffer) && !ArrayBuffer.isView(timeLookup)) {
+      if (!(timeLookup.value instanceof ArrayBuffer) && !ArrayBuffer.isView(timeLookup)) {


Not related to your changes, but would be worthwhile simplifying this check / making it more robust using the bufFromBufLike util up here instead of relying on instanceof

timelookup.value is either an ArrayBuffer or a HashTree. So if we pass timelookup.value into bufFromBufLike when timelookup.value is a HashTree, it will pass the Array.isArray test in bufFromBuflike and be returned as an ArrayBuffer. This ArrayBuffer will then be passed to decodeTime which may actually succeed in decoding a timestamp from it, depending on the format of the HashTree. This results in returning an incorrect timestamp when we shouldn't.

It's more robust to exist early if we receive a malformed response in my opinion.

krpeacock · 2024-05-01T21:14:23Z

packages/agent/src/certificate.test.ts

@@ -20,6 +19,24 @@ function pruned(str: string): ArrayBuffer {
  return fromHex(str);
 }

+function bufferEqualityTester(a: unknown, b: unknown): boolean | undefined {


We already have compare and bufEquals utils in packages/agent/src/utils/buffer.ts

krpeacock · 2024-05-01T21:18:42Z

packages/agent/src/certificate.test.ts

@@ -20,6 +19,24 @@ function pruned(str: string): ArrayBuffer {
  return fromHex(str);
 }

+function bufferEqualityTester(a: unknown, b: unknown): boolean | undefined {


Again, is duplicative of other buffer utils that we already have

huh... I had understood returning undefined was important to tell Jest to use a different equality tester, but this works:

(expect as any).addEqualityTesters([bufEquals]);

Thanks!

krpeacock · 2024-05-01T21:20:54Z

packages/agent/src/certificate.ts

@@ -122,6 +120,17 @@ function isBufferEqual(a: ArrayBuffer, b: ArrayBuffer): boolean {
  return true;
 }

+function isBufferGreaterThan(a: ArrayBuffer, b: ArrayBuffer): boolean {


Can use compare instead.

isBufferGreaterThan is equivalent to compare(a, b) > 0

They're not the same because compare checks for the length of the buffers, which is important for equality, but when we're navigating the tree, we need to know if the current label is greater or less than the label we're searching for in an alphabetical sense. So b should be considered "greater" than aa, but compare will return -1 in that case.

Altering the implementation of compare to work for the tree navigation too would break the hashOfMap and some other hashing utilities.

I can remove the isBufferEqual function in this file in favor of the common utils though.

krpeacock · 2024-05-01T21:34:34Z

@nathanosdev I think that extending the changes to the lookup and also exporting lookup_path is a fine path forward.

I'd rather not also introduce the retry logic for unknown at this stage. It makes me want to think about a different design for retry logic, perhaps offering a more fine-grained set of options giving devs more control over what circumstances the agent will retry under

nathanosdev · 2024-05-07T10:18:45Z

@nathanosdev I think that extending the changes to the lookup and also exporting lookup_path is a fine path forward.

I'd rather not also introduce the retry logic for unknown at this stage. It makes me want to think about a different design for retry logic, perhaps offering a more fine-grained set of options giving devs more control over what circumstances the agent will retry under

Sounds good, I'll extend the changes to lookup.

nathanosdev · 2024-05-07T18:08:14Z

@krpeacock I've extended the changes to the lookup method now and used the lookupResultToBuffer everywhere that references the lookup method to keep the same behaviour as before for internal calls to lookup.

There seems to be something up with the Check PR title job. There was a new breaking version of conventional-changelog-angular a few days ago so I assumed that was the problem, but after locking it down to the previous version that was published several months ago it's still broken. Do you have any idea what might be going on?

krpeacock · 2024-05-07T18:10:34Z

There seems to be something up with the Check PR title job.

Oh, you just have to remove the exclamation mark from the pr title, even though it's proper semantics for conventional commits. Leave it in the change log though

nathanosdev · 2024-05-07T19:20:19Z

It doesn't seem happy with it still, the error message is Error: Must use import to load ES Module: /action/node_modules/conventional-changelog-angular/src/index.js

krpeacock reviewed May 1, 2024

View reviewed changes

nathanosdev added 2 commits May 7, 2024 19:52

feat: add support for proof of absence in certificate lookups

f988a2c

feat: propogate lookup result to public interface

2ef1692

nathanosdev force-pushed the nathan/add-proof-of-absence-support branch from 03cda41 to 2ef1692 Compare May 7, 2024 17:54

nathanosdev changed the title ~~feat: add support for proof of absence in certificate lookups~~ feat!: add support for proof of absence in certificate lookups May 7, 2024

nathanosdev changed the title ~~feat!: add support for proof of absence in certificate lookups~~ feat: add support for proof of absence in certificate lookups May 7, 2024

nathanosdev force-pushed the nathan/add-proof-of-absence-support branch from bce01f6 to 2ef1692 Compare May 7, 2024 19:06

nathanosdev marked this pull request as ready for review May 7, 2024 19:20

nathanosdev requested a review from a team as a code owner May 7, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for proof of absence in certificate lookups #878

feat: add support for proof of absence in certificate lookups #878

nathanosdev commented Apr 29, 2024 •

edited

nathanosdev commented Apr 29, 2024

krpeacock May 1, 2024

nathanosdev May 7, 2024 •

edited

krpeacock May 1, 2024

krpeacock May 1, 2024

nathanosdev May 7, 2024

krpeacock May 1, 2024

nathanosdev May 7, 2024 •

edited

krpeacock commented May 1, 2024

nathanosdev commented May 7, 2024

nathanosdev commented May 7, 2024

krpeacock commented May 7, 2024

nathanosdev commented May 7, 2024

feat: add support for proof of absence in certificate lookups #878

Are you sure you want to change the base?

feat: add support for proof of absence in certificate lookups #878

Conversation

nathanosdev commented Apr 29, 2024 • edited

Description

How Has This Been Tested?

Checklist:

nathanosdev commented Apr 29, 2024

krpeacock May 1, 2024

Choose a reason for hiding this comment

nathanosdev May 7, 2024 • edited

Choose a reason for hiding this comment

krpeacock May 1, 2024

Choose a reason for hiding this comment

krpeacock May 1, 2024

Choose a reason for hiding this comment

nathanosdev May 7, 2024

Choose a reason for hiding this comment

krpeacock May 1, 2024

Choose a reason for hiding this comment

nathanosdev May 7, 2024 • edited

Choose a reason for hiding this comment

krpeacock commented May 1, 2024

nathanosdev commented May 7, 2024

nathanosdev commented May 7, 2024

krpeacock commented May 7, 2024

nathanosdev commented May 7, 2024

nathanosdev commented Apr 29, 2024 •

edited

nathanosdev May 7, 2024 •

edited

nathanosdev May 7, 2024 •

edited