Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENG-1446: Verify enclave sig in export/import #31

Merged
merged 8 commits into from
Apr 3, 2024

Conversation

oliviathet
Copy link
Contributor

@oliviathet oliviathet commented Apr 2, 2024

Based on ToB findings. Pin the signer enclave's quorum public key to import and export sites and verify the signed public keys from the enclave. Once we've deployed a new release that returns the enclave quorum public key in the import and export bundles, we can implement the TODOs I've left for checking that those fields aren't null and can be compared to the pinned quorum public key -- this lets the client know when the pinned quorum public key should be rotated.

@oliviathet oliviathet requested review from r-n-o and timurnkey April 2, 2024 13:55
export/index.html Outdated Show resolved Hide resolved
import/index.html Outdated Show resolved Hide resolved
/**
* Function to verify enclave signature on import bundle received from the server.
*/
const verifyEnclaveSignature = async (enclaveQuorumPublic, publicSignature, publicKey) => {
Copy link

@timurnkey timurnkey Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know if it's possible to use JS modules within an iFrame? I'm seeing a lot of duplicate code which will become harder to maintain. It'd be awesome if we could use a module system although it might not be possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! the HpkeEncrypt and HpkeDecrypt code are actually already JS modules. at some point early on, i tried to DRY up the shared code across the 3 pages but it is a lot of refactoring. as some follow ups, i think we modularize:

  • part 1: import/index.html and import/standalone.html
  • part 2: import and export
  • part 3: import/export and auth

Copy link
Collaborator

@r-n-o r-n-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major concerns here! Small things below but otherwise: 🚀

@@ -12,7 +12,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
directory: ["auth", "export"]
directory: ["auth", "export", "import"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah. Good catch!

export/index.html Outdated Show resolved Hide resolved
export/index.html Outdated Show resolved Hide resolved
export/index.html Outdated Show resolved Hide resolved
@oliviathet oliviathet force-pushed the olivia/verify-enclave-sig branch from be10770 to 86da7fb Compare April 2, 2024 17:22
Olivia Thet added 2 commits April 2, 2024 17:49
…ns. convert arrow fns to declarative fns to avoid ordering issues.
…ns. convert arrow fns to declarative fns to avoid ordering issues.
@oliviathet oliviathet requested a review from r-n-o April 2, 2024 22:09
@oliviathet
Copy link
Contributor Author

@r-n-o Updated the padding function, now called normalizePadding. Added some unit tests for normalizePadding and fromDerSignature. And even though the arrow vs declarative functions weren't the culprit this time, it did trip me up earlier so for now, I just converted all the arrow functions to declarative. In the future when we DRY up some of the code into JS modules, there could be smaller modules of code that make it easier to go back to using arrow functions without encountering a runtime issue

@@ -160,4 +161,78 @@ describe("TKHQ", () => {
// TODO: test logMessage / sendMessageUp
expect(true).toBe(true);
})

it("normalizes padding in a byte array", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@oliviathet oliviathet merged commit d8be53b into main Apr 3, 2024
6 checks passed
@oliviathet oliviathet deleted the olivia/verify-enclave-sig branch April 3, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants