Fix websocket authentication #6632
Conversation
esterlus
commented
Nov 14, 2024
- websocket authentication needed to decode the URI string to compare against a valid token
- allow local cluster with api authentication as documented in SETUP_LOCAL_CLUSTER.md
📝 WalkthroughWalkthroughThe pull request introduces changes to enhance API token authentication in the HOPR cluster setup. It updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
SETUP_LOCAL_CLUSTER.md (2)
37-41: LGTM! Consider adding more context about API authentication.The new instructions for enabling API authentication are clear and concise. However, consider adding:
- A brief explanation of what API token authentication provides
- Where to find the generated API token after setup
- Any security considerations users should be aware of
🧰 Tools
🪛 Markdownlint
39-39: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
39-41: Specify language for the code block.Add shell/bash language specification to the fenced code block for proper syntax highlighting.
-``` +```bash HOPRD_DISABLE_API_AUTHENTICATION=0 ./scripts/setup-local-cluster.sh -i topologies/full_interconnected_cluster.sh<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 39-39: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>scripts/setup-local-cluster.sh (2)</summary><blockquote> `150-154`: **Enhance API token parameter handling** The current implementation has a few potential improvements: 1. Consider removing the "--api" flag when authentication is disabled 2. Add validation for the api_token value when authentication is enabled Consider this improved implementation: ```diff - local api_token_params="--api" - if [[ "${HOPRD_DISABLE_API_AUTHENTICATION:-1}" == 0 ]]; then - api_token_params="${api_token_params} --apiToken ${api_token}" - fi + local api_token_params="" + if [[ "${HOPRD_DISABLE_API_AUTHENTICATION:-1}" == 0 ]]; then + if [[ -z "${api_token}" ]]; then + echo "Error: API token is required when authentication is enabled" >&2 + exit 1 + fi + api_token_params="--api --apiToken ${api_token}" + fi
159-159: Document the HOPRD_DISABLE_API_AUTHENTICATION environment variableThis environment variable controls a critical security feature. Consider adding a comment above this line explaining its purpose and implications.
Add documentation above the environment variable:
+ # HOPRD_DISABLE_API_AUTHENTICATION: Set to 0 to enable API authentication, 1 to disable (default: 1) HOPRD_DISABLE_API_AUTHENTICATION="${HOPRD_DISABLE_API_AUTHENTICATION:-1}" \
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
SETUP_LOCAL_CLUSTER.md(1 hunks)hoprd/rest-api/src/preconditions.rs(2 hunks)scripts/setup-local-cluster.sh(1 hunks)
🧰 Additional context used
🪛 Markdownlint
SETUP_LOCAL_CLUSTER.md
39-39: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
hoprd/rest-api/src/preconditions.rs (3)
12-12: LGTM! Good choice of URL decoding library.
The urlencoding crate is a well-maintained choice for handling URL-encoded strings.
Line range hint 15-71: Verify authentication flow test coverage.
Since this is security-critical code handling WebSocket authentication, ensure:
- Test coverage for all authentication paths
- Error cases are properly tested
- Integration tests with actual WebSocket clients
40-41: Verify WebSocket route coverage and compatibility.
The addition of /api/v3/ prefix is good for API versioning. However, we should verify:
- All WebSocket endpoints are covered
- No breaking changes for existing clients
#!/bin/bash
# Description: Check for any other WebSocket endpoints that might need authentication
# Look for WebSocket route definitions
rg -A 3 "websocket|ws://"
# Look for route handlers with WebSocket upgrades
ast-grep --pattern 'upgrade_ws($$$)' scripts/setup-local-cluster.sh (1)
150-159: Verify authentication behavior for both API and WebSocket endpoints
The changes affect both REST API and WebSocket authentication. Let's verify the authentication behavior works as expected.
b7a294d to
306b4b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
SETUP_LOCAL_CLUSTER.md (2)
37-37: Minor grammar improvement neededConsider revising to include "the" article:
-If you want to enable authentication via API token on local cluster, run: +If you want to enable authentication via API token on the local cluster, run:🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ... enable authentication via API token on local cluster, run: ``` HOPRD_DISABLE_API_AU...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
39-41: Add language specification to the code blockSpecify the shell language for the code block:
-``` +```shell HOPRD_DISABLE_API_AUTHENTICATION=0 ./scripts/setup-local-cluster.sh -i topologies/full_interconnected_cluster.sh<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 39-39: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ffc0c36ec539efd36c3f91a34a88fe0392e78084 and 306b4b9c9c534a329d184a69b5a31133a5ad454d. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `SETUP_LOCAL_CLUSTER.md` (1 hunks) * `hoprd/rest-api/src/preconditions.rs` (2 hunks) * `scripts/setup-local-cluster.sh` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * hoprd/rest-api/src/preconditions.rs * scripts/setup-local-cluster.sh </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>SETUP_LOCAL_CLUSTER.md</summary> [uncategorized] ~37-~37: You might be missing the article “the” here. Context: ... enable authentication via API token on local cluster, run: ``` HOPRD_DISABLE_API_AU... (AI_EN_LECTOR_MISSING_DETERMINER_THE) </details> </details> <details> <summary>🪛 Markdownlint</summary> <details> <summary>SETUP_LOCAL_CLUSTER.md</summary> 39-39: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary> <details> <summary>SETUP_LOCAL_CLUSTER.md (1)</summary> `39-41`: **Consider adding more context about API authentication** The command enables API authentication but doesn't explain: - The default behavior (authentication disabled by default) - Where to find or configure the API token - The impact on security Consider adding these details to help users understand the security implications and configuration requirements. Let's check the related script to understand the default behavior: <details> <summary>✅ Verification successful</summary> Let me gather more information about the API token handling and authentication: --- **Add API authentication documentation** Based on the findings: - API authentication is enabled by default (HOPRD_DISABLE_API_AUTHENTICATION=1 is the default) - The default API token is "^^LOCAL-testing-123^^" for local development - The API token can be customized using the --api-token parameter - Authentication can be disabled using --disableApiAuthentication flag Please add the following details to the documentation: - API authentication is enabled by default for security - The default API token for local development is "^^LOCAL-testing-123^^" - How to set a custom API token: `--api-token <your-token>` - Warning about security implications of disabling authentication - Links to access the API: * REST API: http://localhost:<port> * WebSocket: ws://localhost:<port>/api/v3/messages/websocket?apiToken=<token> </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Check the default value and token handling in the setup script rg "HOPRD_DISABLE_API_AUTHENTICATION" -A 5Length of output: 1815
Script:
#!/bin/bash # Check for API token generation and usage rg "api_token" scripts/setup-local-cluster.sh -A 5 # Check for any authentication related documentation rg -i "api.*auth|auth.*api" README.md docs/ -A 3Length of output: 5461
🧰 Tools
🪛 Markdownlint
39-39: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
|
@esterlus pls run |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
SETUP_LOCAL_CLUSTER.md (3)
37-38: Minor grammar improvement neededConsider revising the sentence to include "the":
-If you want to enable authentication via API token on local cluster, run: +If you want to enable authentication via API token on the local cluster, run:🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ... enable authentication via API token on local cluster, run: ``` HOPRD_DISABLE_API_AU...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
39-41: Add language specifier to the code blockAdd the
bashlanguage specifier to the code block for better syntax highlighting:-``` +```bash HOPRD_DISABLE_API_AUTHENTICATION=0 ./scripts/setup-local-cluster.sh -i topologies/full_interconnected_cluster.sh<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 39-39: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `37-41`: **Enhance documentation with API token details** The new authentication instructions would benefit from additional context about: 1. What the API token is used for 2. How to obtain or configure the token 3. The security implications of enabling/disabling authentication Consider adding a brief explanation, for example: ```markdown API token authentication adds an additional layer of security to your local cluster by requiring valid tokens for API and WebSocket connections. When enabled: - Each node will be configured with a unique API token - All API requests must include this token - WebSocket connections must provide the token for authentication🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ... enable authentication via API token on local cluster, run: ``` HOPRD_DISABLE_API_AU...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint
39-39: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
SETUP_LOCAL_CLUSTER.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
SETUP_LOCAL_CLUSTER.md
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ... enable authentication via API token on local cluster, run: ``` HOPRD_DISABLE_API_AU...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint
SETUP_LOCAL_CLUSTER.md
39-39: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)