Deny unauthenticated keyserver writes by default#42
Conversation
Add the local keyserver service, CLI publish/login/fetch commands, compose smoke wiring, and tests as a reviewable baseline for the security hardening work stacked above it. Constraint: Security fixes for issues #30-#34 target files that are not yet present on main. Rejected: Fold all security fixes into one PR | would make review and issue closure harder. Confidence: medium Scope-risk: broad Directive: Review this foundation before merging stacked hardening PRs into main. Tested: PYTHONPATH=. python3 -m unittest discover keyserver/tests; env -u GOROOT -u GOPATH /opt/homebrew/bin/go test ./... Not-tested: Docker compose smoke test was not run locally.
Keyserver writes now require an admin token or per-user publish token by default, and delete is admin-only. A local-development unauthenticated publish override remains available but is blocked on non-loopback binds unless explicitly marked unsafe. Constraint: Preserve the SSO-issued per-user publish-token workflow while closing the no-token default. Rejected: Remove unauthenticated local publish override entirely | local isolated demos may still need it, but it must be explicit and guarded. Confidence: high Scope-risk: moderate Directive: Treat the keyserver as public-facing; do not reintroduce implicit unauthenticated writes. Tested: PYTHONPATH=. python3 -m unittest discover keyserver/tests; env -u GOROOT -u GOPATH /opt/homebrew/bin/go test ./... Related: #30
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR implements secure-by-default authentication for the keyserver by denying unauthenticated writes when no admin token is configured. The implementation includes proper validation to prevent unsafe configurations and comprehensive test coverage.
Critical Issues Found:
- Multiple test methods use
patch.dict(os.environ, env, clear=True)which clears ALL environment variables and could cause test failures or unpredictable behavior. This needs to be fixed before merge.
Security Improvements:
- Default deny behavior prevents unauthorized writes
- Loopback validation ensures unsafe configurations can't be used in production
- Admin-only delete operations enforce proper access control
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| with patch.dict(os.environ, env, clear=True): | ||
| status, body = self.request("PUT", "/v1/peers/alice", valid_profile()) |
There was a problem hiding this comment.
🛑 Logic Error: Using clear=True in patch.dict removes all environment variables, which could cause self.request() to fail if the HTTP connection or server depends on any environment variables. Test execution may crash or behave unpredictably.
| with patch.dict(os.environ, env, clear=True): | |
| status, body = self.request("PUT", "/v1/peers/alice", valid_profile()) | |
| env = {"ENDE_KEYSERVER_ALLOW_UNAUTHENTICATED_WRITES": "1"} | |
| with patch.dict(os.environ, env): | |
| status, body = self.request("PUT", "/v1/peers/alice", valid_profile()) |
| with patch.dict(os.environ, env, clear=True): | ||
| with self.assertRaisesRegex(ValueError, "loopback bind"): | ||
| keyserver_server.validate_write_auth_config("0.0.0.0", "") |
There was a problem hiding this comment.
🛑 Logic Error: Using clear=True clears all environment variables, which could cause test failures or unpredictable behavior if the system depends on environment variables like PATH, HOME, or locale settings during HTTP operations.
| with patch.dict(os.environ, env, clear=True): | |
| with self.assertRaisesRegex(ValueError, "loopback bind"): | |
| keyserver_server.validate_write_auth_config("0.0.0.0", "") | |
| env = {"ENDE_KEYSERVER_ALLOW_UNAUTHENTICATED_WRITES": "1"} | |
| with patch.dict(os.environ, env): | |
| with self.assertRaisesRegex(ValueError, "loopback bind"): | |
| keyserver_server.validate_write_auth_config("0.0.0.0", "") |
| with patch.dict(os.environ, env, clear=True): | ||
| keyserver_server.validate_write_auth_config("127.0.0.1", "") |
There was a problem hiding this comment.
🛑 Logic Error: Using clear=True removes all environment variables, which could cause failures if system operations depend on variables like PATH or locale settings.
| with patch.dict(os.environ, env, clear=True): | |
| keyserver_server.validate_write_auth_config("127.0.0.1", "") | |
| env = {"ENDE_KEYSERVER_ALLOW_UNAUTHENTICATED_WRITES": "1"} | |
| with patch.dict(os.environ, env): | |
| keyserver_server.validate_write_auth_config("127.0.0.1", "") |
|
Closing because this was opened against the wrong repository. The security triage is being redone against companyjupiter/quarkify. |
1 participant
Summary
Closes #30.
Validation
PYTHONPATH=. python3 -m unittest discover keyserver/testsenv -u GOROOT -u GOPATH /opt/homebrew/bin/go test ./...