auth: Add option to allow basic auth for non-TLS requests #674
Conversation
WalkthroughThe changes involve modifications to the authentication logic in the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/674/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ably/auth.go (1)
548-548: LGTM! Consider adding a security warning comment.The condition correctly implements the opt-in behavior for using basic auth over non-TLS connections. This is a good security practice as it requires explicit acknowledgment of the insecure configuration.
Consider adding a comment to warn about the security implications:
+ // InsecureAllowBasicAuthWithoutTLS explicitly allows basic authentication over non-TLS connections. + // WARNING: This is inherently insecure as credentials are sent in base64 encoding. Only use this + // option in trusted environments like local development or VPNs. if opts.NoTLS && !opts.InsecureAllowBasicAuthWithoutTLS {ably/options.go (2)
411-414: Enhance security warning in documentationThe documentation should be more explicit about the security implications of using this option.
Consider expanding the documentation to:
- // InsecureAllowBasicAuthWithoutTLS permits an API key to be used even if the connection - // will not use TLS, something which would otherwise not be permitted for security reasons. + // InsecureAllowBasicAuthWithoutTLS permits an API key to be used even if the connection + // will not use TLS, something which would otherwise not be permitted for security reasons. + // WARNING: Using this option is highly discouraged in production environments as it may + // expose your API key to attackers through man-in-the-middle attacks. Only use this + // option in secure development environments or private networks where the security + // implications are fully understood.
1323-1329: Add usage example and enhance documentationThe function would benefit from a usage example and more detailed documentation.
Consider enhancing the documentation:
-// WithInsecureAllowBasicAuthWithoutTLS permits an API key to be used even if the connection -// will not use TLS, something which would otherwise not be permitted for security reasons. +// WithInsecureAllowBasicAuthWithoutTLS permits an API key to be used even if the connection +// will not use TLS, something which would otherwise not be permitted for security reasons. +// +// WARNING: Using this option is highly discouraged in production environments as it may +// expose your API key to attackers through man-in-the-middle attacks. Only use this +// option in secure development environments or private networks where the security +// implications are fully understood. +// +// Example usage: +// +// client := ably.NewREST( +// WithKey("your-api-key"), +// WithTLS(false), +// WithInsecureAllowBasicAuthWithoutTLS(), +// )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ably/auth.go(1 hunks)ably/options.go(2 hunks)
🔇 Additional comments (1)
ably/options.go (1)
411-414: Implementation aligns well with PR objectives
The implementation successfully adds the requested functionality while:
- Following established code patterns
- Including appropriate security warnings
- Maintaining code quality and consistency
Also applies to: 1323-1329
There was a problem hiding this comment.
Not sure why so many tests got flaky suddenly : ( .
We always had green CI for every PR before.
One of the common test I can see failing is TestRealtimeConn_RTN15i_OnErrorWhenConnected
@lmars can you check what might be going wrong?
|
@lmars we are planning to create one more PR to fix present enter 250 test right? |
|
Simple test can be added to |
We internally often want to use the Go SDK in private environments (e.g. local development, or within virtual private networks), where we do not use TLS, but find it convenient to be able to use API keys rather than tokens. This commit adds an InsecureAllowBasicAuthWithoutTLS option which permits the use of an API key when the NoTLS option is also set. We only require this in the Go SDK, so there is no intention of adding this option to the feature spec. Signed-off-by: Lewis Marshall <[email protected]>
lmars
force-pushed
the
allow-no-tls-basic-auth
branch
from
0e1c8e8 to
631f76c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ably/options.go (1)
411-414: Consider enhancing security documentationWhile the implementation is solid, consider adding:
- A comment in the documentation explaining when this option should be used (e.g., specific development scenarios, private networks)
- A warning about the security implications of using this option in production environments
Also applies to: 1323-1329
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ably/auth.go(1 hunks)ably/options.go(2 hunks)ably/options_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/auth.go
🔇 Additional comments (3)
ably/options_test.go (1)
239-259: Well-structured test cases!
The test cases effectively verify both the security constraint and the override behavior. Good job on:
- Testing both positive and negative scenarios
- Verifying the specific error code
- Using a realistic API key format
ably/options.go (2)
411-414: Good field naming and documentation!
The field follows Go security-related naming conventions by using the "Insecure" prefix, similar to InsecureSkipVerify. The documentation clearly explains the security implications.
1323-1329: Clean implementation following existing patterns!
The function follows the established pattern of other option setters in the codebase and maintains consistent documentation style.
We internally often want to use the Go SDK in private environments (e.g. local development, or within virtual private networks), where we do not use TLS, but find it convenient to be able to use API keys rather than tokens.
This commit adds an
InsecureAllowBasicAuthWithoutTLSoption which permits the use of an API key when theNoTLSoption is also set.We only require this in the Go SDK, so there is no intention of adding this option to the feature spec.
Summary by CodeRabbit