Socket map for check virtual users and virtual domains#281
Socket map for check virtual users and virtual domains#281Aravinda-HWK wants to merge 3 commits intoLSFLK:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, dedicated socket map service to handle virtual user and domain existence checks for the Postfix SMTP server. This change significantly enhances the flexibility and dynamism of user validation by decoupling it from the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new socketmap-server Go service to handle virtual user and domain lookups for Postfix, replacing the previous SQLite-based approach. The changes include the Go service itself, Docker integration via docker-compose.yaml, a Dockerfile for building the service, and extensive documentation and test scripts. While this is a significant and valuable feature, the review has identified several critical and high-severity issues. There is a critical race condition in the in-memory cache implementation in main.go that needs to be addressed with a mutex. Additionally, there are widespread inconsistencies between the implemented netstring protocol in the Go service and the plain-text protocol used in most of the test scripts and documentation, rendering them incorrect and non-functional. Other suggestions include improving the Dockerfile for security and reproducibility, and adding a healthcheck to the service in docker-compose.yaml.
services/socketmap/main.go
Outdated
| _, err = reader.Read(data) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read data: %w", err) | ||
| } |
There was a problem hiding this comment.
The reader.Read(data) call does not guarantee that it will read len(data) bytes; it may read fewer bytes without returning an error. This can lead to incorrect parsing of the netstring data. To ensure you read the exact number of bytes specified by the netstring length, you should use io.ReadFull.
You will need to add import "io".
if _, err := io.ReadFull(reader, data); err != nil {
return "", fmt.Errorf("failed to read data: %w", err)
}| # Run the service | ||
| CMD ["./socketmap"] |
There was a problem hiding this comment.
For better security, the container should not run as the root user. You should create a dedicated non-root user, change file ownership, and switch to that user before running the application.
# Create a non-root user and group
RUN addgroup -S appgroup && adduser -S appuser -G appgroup
# Set ownership for the app directory and binary
RUN chown -R appuser:appgroup /app
# Switch to the non-root user
USER appuser
# Run the service
CMD ["./socketmap"]
| socketmap-server: | ||
| build: | ||
| context: ./socketmap | ||
| dockerfile: Dockerfile | ||
| container_name: socketmap-server | ||
| expose: | ||
| - "9100" | ||
| networks: | ||
| - mail-network | ||
| restart: unless-stopped | ||
| environment: | ||
| - SOCKETMAP_HOST=0.0.0.0 | ||
| - SOCKETMAP_PORT=9100 | ||
| deploy: | ||
| resources: | ||
| limits: | ||
| memory: 128M | ||
| reservations: | ||
| memory: 64M | ||
|
|
There was a problem hiding this comment.
The socketmap-server service is missing a healthcheck. Adding one would improve the reliability of your stack, especially since smtp-server depends on it. Without a healthcheck, depends_on only waits for the container to start, not for the service to be ready. You could then change the dependency to condition: service_healthy.
Note that the healthcheck command needs to send a valid netstring request, which can be tricky with sh -c. You may want to consider adding a simpler healthcheck mechanism to the Go service itself.
services/socketmap/main.go
Outdated
| for { | ||
| conn, err := listener.Accept() | ||
| if err != nil { | ||
| log.Printf("⚠ Error accepting connection: %v", err) | ||
| continue | ||
| } | ||
|
|
||
| connectionCount++ | ||
| log.Printf("") | ||
| log.Printf("═══════════════════════════════════════") | ||
| log.Printf("Connection #%d from %s", connectionCount, conn.RemoteAddr()) | ||
| log.Printf("═══════════════════════════════════════") | ||
| go handleConnection(conn) | ||
| } |
There was a problem hiding this comment.
The server does not handle graceful shutdown. If it receives a SIGINT or SIGTERM, it will exit immediately, potentially dropping active connections. You should implement signal handling to allow for a graceful shutdown. This typically involves:
- Listening for
os.Interruptandsyscall.SIGTERMsignals. - On signal, call
listener.Close()to stop accepting new connections. - Use a
sync.WaitGroupto track and wait for active connections to finish before exiting.
Aravinda-HWK
force-pushed
the
socket-map-virtual-users
branch
4 times, most recently
from
e3f0b48 to
f24fa1b
Compare
278 bug secure the rspamd web UI dashboard (LSFLK#279) * fix: update Rspamd worker configuration to remove Prometheus metrics access * fix: update rspamd-server configuration to expose port 11332 without publishing feat: create socketmap for vitual-users, virtual-domains and virtual-aliases - Implemented a new Socketmap service in Go for dynamic user validation in Postfix. - Created Dockerfile for building and running the Socketmap service. - Added Docker Compose configuration for integrating Socketmap into the Silver mail server stack. - Developed comprehensive documentation including deployment guide, integration summary, and quick start reference. - Implemented caching mechanism for user lookups to improve performance. - Added automated and interactive testing scripts for validating service functionality. - Updated Postfix configuration scripts to utilize Socketmap for mailbox validation. - Established logging for monitoring service activity and troubleshooting. feat: enhance logging and troubleshooting for socketmap service feat: implement netstring protocol support for Postfix socketmap service fix: correct request format for Postfix socketmap protocol in documentation and tests chore: remove redundant documentations chore: remove outdated Socketmap architecture documentation Refactor socketmap service: Remove outdated documentation and scripts - Deleted LOGGING_UPDATE.md, NETSTRING_FIX.md, QUICKSTART.md, TROUBLESHOOTING.md, diagnostic.sh, interactive-test.sh, test-netstring.sh, and test.sh as they contained obsolete information and functionality. - Enhanced logging and netstring protocol support have been implemented in the main service code, making these documents redundant. - Updated service to ensure proper netstring encoding and decoding for socketmap requests. - Improved error handling and response logging for better debugging. remove Socketmap integration quick reference documentation feat: enhance socketmap service to support virtual domains and aliases
Aravinda-HWK
force-pushed
the
socket-map-virtual-users
branch
from
f24fa1b to
e7859dd
Compare
1 participant
📌 Description
This PR is to add a socket map for checking the existence of virtual users and virtual domains to remove the users and domains tables from the shared.db database.
🔍 Changes Made
✅ Checklist (Email System)
🧪 Testing Instructions
📷 Screenshots / Logs (if applicable)