-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: header auth #6825
base: main
Are you sure you want to change the base?
feat: header auth #6825
Conversation
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 2ec65a3 in 5 minutes and 2 seconds
More details
- Looked at
307
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. pkg/query-service/auth/rbac.go:64
- Draft comment:
Consider adding validation or sanitization for the user information retrieved from HTTP headers to prevent potential security vulnerabilities. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment raises a legitimate security concern - header values could contain malicious input. However, this function appears to be an internal utility, likely called after authentication has already occurred (given the JWT-based GetUserFromRequest exists). Without seeing how this function is used, we can't be certain if validation is needed here or if it's handled at the call site. The comment is speculative without more context.
I could be wrong about this being an internal utility - if this is exposed directly to external requests, validation would be critical. Also, even internal utilities should be defensive.
While defensive programming is good, we don't have enough context to know if validation is needed here or if it's handled elsewhere. The comment is speculative without seeing the full security architecture.
Delete the comment since it makes a speculative security suggestion without clear evidence that validation is missing or needed at this specific point in the code.
2. pkg/query-service/dao/sqlite/connection.go:133
- Draft comment:
Reconsider handling multiple organizations ininitializeOrgPreferences
. The current implementation has a commented-out check for multiple organizations, which could lead to issues if not addressed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is pointing out something that's already acknowledged in the code comments. The TODO and commented-out code is intentional as part of a transition to multi-tenant support. The existing comment on L122 already explains this with "TODO(Ahsan): Make it multi-tenant when we move to a system with multiple orgs." The PR comment doesn't add new information or identify a specific issue that needs fixing.
The code change could potentially introduce bugs by removing the single-org constraint without fully implementing multi-org support. Maybe this deserves discussion.
The existing code comment already acknowledges this is an interim state during transition to multi-org support. The PR comment doesn't provide actionable feedback beyond what's already documented.
Delete the comment as it's not providing actionable feedback beyond what's already documented in the code comments.
3. pkg/query-service/main.go:126
- Draft comment:
TheuseHeaderAuth
flag is set but not used inserverOptions
. Ensure it is used where necessary to affect server behavior. - Reason this comment was not posted:
Confidence changes required:50%
Inmain.go
, theuseHeaderAuth
flag is set but not used in theserverOptions
. This could lead to confusion or errors if the flag is expected to affect server behavior.
4. frontend/src/container/Login/index.tsx:190
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets or styled components instead. This is also applicable in other parts of the file. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
5. frontend/src/container/Login/index.tsx:49
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is also applicable in other parts of the project. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_vO8bDTFLaT3yEeGW
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
return nil, apiErr | ||
} | ||
|
||
hash, err := PasswordHash(utils.GeneratePassowrd()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in utils.GeneratePassowrd()
. It should be utils.GeneratePassword()
. This could lead to a runtime error if the function does not exist.
Summary
This is a first cut of supporting "trusted header authentication", as referenced in this issue, and also inspired by Kubernetes' support for authenticating proxies and impersonation. It is working in my cluster, but I expect will need more polish before merge. I wanted to open the PR to provide a concrete basis for discussion and alignment on the approach and design.
The feature is an alternative means of authenticating users, in addition to the existing email+password login and the ee-only SAML login. In this scenario:
The implementation in this PR expects that the query service is locked down at the network level to only permit access through the reverse proxy. This can be achieved via running the proxy as a container in the same pod and only listening on localhost, via network policy, or other means. Future enhancements could include:
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
The feature is behind a feature flag command-line arg that preserves the prior logic when it is not enabled. When it is enabled, the front-end will call the login API with only the email specified, and the query service will use the header injected by the proxy to authenticate the user, instead of password.
This implementation will create the specified user and org in the sqllite DB, if they do not already exist. This may merit discussion to ensure it aligns with the long-term intentions for SigNoz multi-tenancy. It doesn't appear that the current persisted user and org data are used for much, but I'm sure there are plans to build on them. Happy to discuss this on Slack or elsewhere.
Manually tested login, logout, email+password login when auth headers are absent.
Important
Add trusted header authentication to support user authentication via HTTP headers, with changes in backend and frontend components.
X-Signoz-User
,X-Signoz-Role
, andX-Signoz-Org
headers inauth.go
andrbac.go
.Login()
inauth.go
to support header-based user creation and authentication.CreateHeaderAuthUser()
inauth.go
for creating users from headers.loginUser()
inauth.go
andhttp_handler.go
to use header authentication.headerEmail
toGetVersionResponse
inresponse.go
.headerAuthEmail
state and logic inLogin/index.tsx
to handle header-based login.getVersion.ts
to includeheaderEmail
in the payload.use-header-auth
flag inmain.go
to enable header authentication..editorconfig
to use tabs for TypeScript files.This description was created by for 2ec65a3. It will automatically update as commits are pushed.