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

Misc fixes for key sync #87

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Misc fixes for key sync #87

wants to merge 7 commits into from

Conversation

DJAndries
Copy link
Collaborator

  • Worker gets own hostname/IP from vsock-relay IP provider server
  • Wait for networking to be setup before starting key sync process
  • Change flow to contact GET and PUT /enclave/state to prompt key generation from app, and notify the app of new keys
  • Include port when contacting other enclaves, so key synchronization can proceed when multiple enclaves are on the same host
  • Other misc fixes

Copy link
Contributor

@rillian rillian left a comment

Choose a reason for hiding this comment

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

Seems fine, but I'm still confused about the motivation for some of these changes. Why do we need another proxy connection to get the worker's private IP address? Can't the leader just call back to the source addresses it gets requests from?

@@ -8,8 +8,8 @@ import (
)

const (
maxAttstnBodyLen = 1 << 14 // Upper limit for attestation body length.
boxKeyLen = 32 // NaCl box's private and public key length.
maxAttstnBodyLen = 256 * 1024 // Upper limit for attestation body length.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a lot of extra space. How big is the ppoprf state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

between 100 - 200 kb

claucece
claucece previously approved these changes Aug 27, 2024
Copy link
Member

@claucece claucece left a comment

Choose a reason for hiding this comment

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

This lgtm, but I agree also with the comment from ralph.

Copy link

[puLL-Merge] - brave/nitriding-daemon@87

Description

This PR introduces significant changes to the nitriding-daemon project, focusing on improving the key synchronization process, enhancing network setup, and refining the overall architecture. The changes aim to make the system more robust, secure, and efficient in handling enclave operations and key management.

Possible Issues

  1. The increase in maxAttstnBodyLen from 16KB to 256KB might lead to increased memory usage.
  2. The removal of the GET /enclave/state endpoint could potentially break existing client applications that rely on this endpoint.

Security Hotspots

  1. The change in PCR verification (attestation.go) now excludes both PCR3 and PCR4. This change should be carefully reviewed to ensure it doesn't introduce any security vulnerabilities.
  2. The introduction of a new HostIpProviderPort in the configuration might expose a new attack surface if not properly secured.
Changes

Changes

  1. attestation.go:

    • Modified PCR verification to exclude both PCR3 and PCR4 instead of just PCR4.
  2. doc/key-synchronization.md:

    • Updated the sequence diagram to reflect changes in the key synchronization process.
  3. enclave.go:

    • Added heartbeatActive and myHostname fields to the Enclave struct.
    • Modified the weAreLeader function to increase timeout and improve error handling.
    • Updated setupWorkerPostSync to send key to the app and start heartbeat.
    • Changed getLeader to use the full FQDNLeader including port.
  4. flake.nix and flake.lock:

    • Added new files for Nix package management.
  5. handlers.go:

    • Removed getStateHandler function.
  6. main.go:

    • Added hostIpProviderPort configuration option.
  7. metrics.go:

    • Added request duration metrics.
    • Modified metric labels for better consistency.
  8. proxy.go:

    • Updated setupNetworking to signal when network is ready.
  9. sync_worker.go:

    • Introduced interimSyncState to manage sync state more efficiently.
    • Modified sync process to use the new state management.
  10. util.go:

    • Replaced AWS Instance Metadata Service with a custom host IP provider.
    • Added functions to interact with the application for key management.
  11. Various test files:

    • Updated tests to accommodate the new changes in the codebase.

Overall, this PR significantly refactors the key synchronization process, improves network setup, and enhances the interaction between the enclave and the application for key management.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants