Skip to content

Conversation

@denisiuriet
Copy link
Contributor

Fixes # .

Changes proposed in this PR:

@alexcos20
Copy link
Member

shouldn't we have tls on wss , and normal websocket on ws ?

@alexcos20
Copy link
Member

/run-security-scan

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request introduces automatic TLS (Transport Layer Security) functionality for the P2P component of Ocean Node. It includes adding new libp2p services (autoTLS, keychain), updating network port configurations, and changing the default for network statistics.

Comments:
• [WARNING][other] There are two different auto-tls packages listed in package.json: @ipshipyard/libp2p-auto-tls and @libp2p/auto-tls. However, only @ipshipyard/libp2p-auto-tls is imported and used in src/components/P2P/index.ts. Please clarify if @libp2p/auto-tls is intentionally included or if it can be removed to avoid confusion and unnecessary package bulk.
• [WARNING][style] The console.log statement for self:peer:update event is present in the P2P component. For production readiness, this should ideally be replaced with P2P_LOGGER.info or removed if it's purely for debugging purposes, to maintain consistent logging practices.
• [INFO][other] The default value for enableNetworkStats has been changed from false to true. Please confirm if this change in default behavior is intentional for all deployments and if there are any significant implications (e.g., performance overhead, increased resource usage) that users should be aware of by having network statistics enabled by default.
• [INFO][other] The ipV4BindWssPort has been changed from 9005 to 9006 across Dockerfile, config.json, and schemas.ts. While consistent, could you provide a brief reason for this specific port change? Was there a conflict with port 9005 or is this part of a new port allocation strategy?
• [INFO][other] The package-lock.json shows a significant number of dependency updates and additions, which is expected with new libp2p components. Please ensure that these new and updated dependencies (especially those with major version bumps, like undici in sub-dependencies) have been reviewed for any known security vulnerabilities or breaking changes.
• [INFO][other] The logic for pushing /wss bind interfaces has been moved outside the ipV4BindWsPort conditional block. This seems to correctly ensure WSS binding is independent of WS binding. Good refactor.
• [INFO][other] The autoTLS service is initialized with autoConfirmAddress: true. This is a reasonable default for many scenarios. Confirm if there are any specific cases where this should be configurable or if it's expected to always be true for Ocean Node deployments.

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