feat: add support for proof of absence in certificate lookups

[nathanosdev]

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]

@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]

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

[nathanosdev]

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]

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

[krpeacock]

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

[nathanosdev]

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]

Can use compare instead.

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

[nathanosdev]

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]

@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]

@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]

@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]

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]

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