infra/ansible
23
7
Fork 53
Code Pull requests 11 Activity Actions

[postfix] Obfuscate the IP on smtp-auth server #3297

Merged
zlopez merged 1 commit from zlopez/ansible:12835 into main 2026-05-05 07:52:06 +00:00
Conversation 3 Commits 1 Files changed 3 +21
zlopez commented 2026-04-22 10:44:46 +00:00
Owner
Copy link

This will update configuration to clean Received header from e-mail
forwarded by smtp-auth-iso01 server to prevent leaking of internal IPs.
See infra/tickets#12835

Signed-off-by: Michal Konecny mkonecny@redhat.com

This will update configuration to clean Received header from e-mail forwarded by smtp-auth-iso01 server to prevent leaking of internal IPs. See https://forge.fedoraproject.org/infra/tickets/issues/12835 Signed-off-by: Michal Konecny <mkonecny@redhat.com>
[postfix] Obfuscate the IP on smtp-auth server
All checks were successful
Linter / yamllint (pull_request) Successful in 1m2s
Details
Linter / ansible-lint (pull_request) Successful in 51s
Details
AI Code Review / ai-review (pull_request_target) Successful in 16s
Details
661e8244f8 
This will update configuration to clean Received header from e-mail
forwarded by smtp-auth-iso01 server to prevent leaking of internal IPs.
See infra/tickets#12835

Signed-off-by: Michal Konecny <mkonecny@redhat.com>
forgejo-actions commented 2026-04-29 21:51:16 +00:00
Copy link

AI Code Review

📋 MR Summary

This merge request updates the Postfix configuration on smtp-auth servers to strip 'Received' headers to prevent leaking internal IP addresses.

  • Key Changes:
    • Adds a custom header checks file to ignore 'Received' headers.
    • Modifies master.cf to use a new 'ascleanup' service for submissions.
    • Adds an Ansible task to copy the header checks file to target servers.
  • Impact: roles/base/files/postfix, roles/base/tasks
  • Risk Level: 🟡 Medium - While the logic for header removal is correct, file naming mismatches will cause the Ansible playbook to fail or Postfix to fail starting due to missing files.

Detailed Code Review

The approach of creating a separate cleanup service (ascleanup) and binding it to the submission service via cleanup_service_name is the correct and secure way to strip headers in Postfix without affecting other traffic. However, there is a critical typo in the file names between the actual file on disk, the Ansible copy task, and the Postfix master.cf configuration. This will break the deployment and potentially the mail service if Postfix fails to reload.

📂 File Reviews

📄 `roles/base/tasks/postfix.yml` - Adds an Ansible task to deploy the new Postfix header checks file.
  • Critical [Bug]: There is a mismatch in the filename. The created file is header_checks_submission.smtp-auth (plural 'checks'), but the src and dest in the Ansible task use header_check_submission (singular 'check'). Furthermore, master.cf expects /etc/postfix/header_checks_submission. This will cause the copy task to fail, or Postfix to fail to start.
    • Suggestion: Update the src and dest to match the actual filename and the path defined in master.cf.
- name: Install /etc/postfix/header_checks_submission file
  ansible.builtin.copy:
    src: postfix/header_checks_submission.smtp-auth
    dest: /etc/postfix/header_checks_submission
    mode: "0644"

✅ Summary

  • Overall Assessment: A critical filename mismatch was identified that will prevent successful deployment and potentially break the Postfix service.
  • Priority Issues:
    • Fix the spelling of header_checks_submission in the Ansible task to match the committed file name and the path expected by master.cf.
  • Minor Suggestions:
    • Ensure that the postfix-pcre package is installed on the target servers, as the configuration specifically requests pcre:/etc/postfix/....

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This merge request updates the Postfix configuration on smtp-auth servers to strip 'Received' headers to prevent leaking internal IP addresses. - **Key Changes:** - Adds a custom header checks file to ignore 'Received' headers. - Modifies master.cf to use a new 'ascleanup' service for submissions. - Adds an Ansible task to copy the header checks file to target servers. - **Impact:** roles/base/files/postfix, roles/base/tasks - **Risk Level:** 🟡 Medium - While the logic for header removal is correct, file naming mismatches will cause the Ansible playbook to fail or Postfix to fail starting due to missing files. ### Detailed Code Review The approach of creating a separate cleanup service (`ascleanup`) and binding it to the submission service via `cleanup_service_name` is the correct and secure way to strip headers in Postfix without affecting other traffic. However, there is a critical typo in the file names between the actual file on disk, the Ansible copy task, and the Postfix `master.cf` configuration. This will break the deployment and potentially the mail service if Postfix fails to reload. #### 📂 File Reviews <details> <summary><strong>📄 `roles/base/tasks/postfix.yml`</strong> - Adds an Ansible task to deploy the new Postfix header checks file.</summary> - **Critical** [Bug]: There is a mismatch in the filename. The created file is `header_checks_submission.smtp-auth` (plural 'checks'), but the `src` and `dest` in the Ansible task use `header_check_submission` (singular 'check'). Furthermore, `master.cf` expects `/etc/postfix/header_checks_submission`. This will cause the copy task to fail, or Postfix to fail to start. - *Suggestion:* Update the `src` and `dest` to match the actual filename and the path defined in `master.cf`. ```` - name: Install /etc/postfix/header_checks_submission file ansible.builtin.copy: src: postfix/header_checks_submission.smtp-auth dest: /etc/postfix/header_checks_submission mode: "0644" ```` </details> ### ✅ Summary - **Overall Assessment:** A critical filename mismatch was identified that will prevent successful deployment and potentially break the Postfix service. - **Priority Issues:** - Fix the spelling of `header_checks_submission` in the Ansible task to match the committed file name and the path expected by `master.cf`. - **Minor Suggestions:** - Ensure that the `postfix-pcre` package is installed on the target servers, as the configuration specifically requests `pcre:/etc/postfix/...`. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
zlopez force-pushed 12835 from 661e8244f8
All checks were successful
Linter / yamllint (pull_request) Successful in 1m2s
Details
Linter / ansible-lint (pull_request) Successful in 51s
Details
AI Code Review / ai-review (pull_request_target) Successful in 16s
Details
to 67953a5eaa
Some checks failed
Linter / yamllint (pull_request) Successful in 27s
Details
Linter / ansible-lint (pull_request) Failing after 45s
Details
2026-05-04 12:55:32 +00:00 Compare
zlopez force-pushed 12835 from 67953a5eaa
Some checks failed
Linter / yamllint (pull_request) Successful in 27s
Details
Linter / ansible-lint (pull_request) Failing after 45s
Details
to 3f1e07dd89
All checks were successful
Linter / yamllint (pull_request) Successful in 25s
Details
Linter / ansible-lint (pull_request) Successful in 56s
Details
AI Code Review / ai-review (pull_request_target) Successful in 18s
Details
2026-05-04 13:26:23 +00:00 Compare
forgejo-actions commented 2026-05-04 13:29:32 +00:00
Copy link

AI Code Review

📋 MR Summary

This MR configures the smtp-auth server to obfuscate internal IP addresses by stripping Received: headers from forwarded emails.

  • Key Changes:
    • Added a new Postfix PCRE header_checks file to ignore Received: headers.
    • Updated master.cf to implement a dedicated ascleanup service for the submission port using the new header checks.
    • Added postfix-pcre package installation and deployment logic for the new header checks file to the Ansible base role.
  • Impact: roles/base/files/postfix, roles/base/tasks/postfix.yml
  • Risk Level: 🟢 Low - The changes use standard Postfix configurations (cleanup service and PCRE header checks) to enforce privacy without introducing complex logic or modifying core mail routing.

Detailed Code Review

The implementation correctly applies standard Postfix patterns to strip sensitive submission headers. Creating a dedicated cleanup service (ascleanup) tied to the submission port ensures that only authenticated submitted messages have their Received: headers stripped, rather than interfering with all mail flowing through the standard cleanup service. The Ansible tasks properly gate the file deployment to smtp-auth hosts.

📂 File Reviews

📄 `roles/base/tasks/postfix.yml` - Adds the postfix-pcre package and handles the deployment of the header checks file.
  • Minor [Suggestion]: The postfix-pcre package is added to the base package installation list, which will install it on all hosts, whereas the header checks file is only deployed on smtp-auth* hosts. If you only want this package on the smtp-auth servers, consider moving the package installation to a conditionally executed task.
    • Suggestion: Move postfix-pcre to a separate task with a when: inventory_hostname.startswith('smtp-auth') condition, or leave as is if having it on all base nodes is acceptable.

✅ Summary

  • Overall Assessment: The changes correctly implement the requested IP obfuscation with no critical issues identified. Ready for merge.
  • Minor Suggestions:
    • Consider restricting the installation of the postfix-pcre package only to smtp-auth hosts if it is not required system-wide.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This MR configures the smtp-auth server to obfuscate internal IP addresses by stripping `Received:` headers from forwarded emails. - **Key Changes:** - Added a new Postfix PCRE header_checks file to ignore `Received:` headers. - Updated `master.cf` to implement a dedicated `ascleanup` service for the submission port using the new header checks. - Added `postfix-pcre` package installation and deployment logic for the new header checks file to the Ansible base role. - **Impact:** roles/base/files/postfix, roles/base/tasks/postfix.yml - **Risk Level:** 🟢 Low - The changes use standard Postfix configurations (cleanup service and PCRE header checks) to enforce privacy without introducing complex logic or modifying core mail routing. ### Detailed Code Review The implementation correctly applies standard Postfix patterns to strip sensitive submission headers. Creating a dedicated cleanup service (`ascleanup`) tied to the submission port ensures that only authenticated submitted messages have their `Received:` headers stripped, rather than interfering with all mail flowing through the standard cleanup service. The Ansible tasks properly gate the file deployment to `smtp-auth` hosts. #### 📂 File Reviews <details> <summary><strong>📄 `roles/base/tasks/postfix.yml`</strong> - Adds the postfix-pcre package and handles the deployment of the header checks file.</summary> - **Minor** [Suggestion]: The `postfix-pcre` package is added to the base package installation list, which will install it on all hosts, whereas the header checks file is only deployed on `smtp-auth*` hosts. If you only want this package on the smtp-auth servers, consider moving the package installation to a conditionally executed task. - *Suggestion:* Move `postfix-pcre` to a separate task with a `when: inventory_hostname.startswith('smtp-auth')` condition, or leave as is if having it on all base nodes is acceptable. </details> ### ✅ Summary - **Overall Assessment:** The changes correctly implement the requested IP obfuscation with no critical issues identified. Ready for merge. - **Minor Suggestions:** - Consider restricting the installation of the `postfix-pcre` package only to `smtp-auth` hosts if it is not required system-wide. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
zlopez commented 2026-05-04 13:37:56 +00:00
Author
Owner
Copy link

I think I addressed the major issue on this PR now.

I think I addressed the major issue on this PR now.
zlopez merged commit 0506d3a645 into main 2026-05-05 07:52:06 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-review-please
label for requesting AI code review

  
freeze-break-request

   
post-freeze
  
Backlog Status
Needs Review
 Work items that have been created or updated but need review, refinement, or approval before they can be considered ready for sprint planning.

  
Backlog Status
Ready
 Work items that have been reviewed, refined, and are ready to be committed to a sprint. These items have clear requirements and can be worked on immediately when capacity is available.

  
chore
Work that needs to be done but doesn't add new functionality or fix bugs. Maintenance tasks that keep the system running smoothly and the codebase clean.

  
documentation
Work focused on creating, updating, or improving documentation for users, developers, or system administrators.

  
points
01
 A task that is simple, small, and has very little uncertainty. It should be easy enough for any team member to pick up and complete quickly, without a lot of discussion or research.

  
points
02
 This is a task that is slightly more complex or larger than a "1." It might involve two or three simple steps, but the outcome is still very clear and there's little uncertainty.

  
points
03
 Medium-sized task with some noticeable complexity. It might require a bit more thought or collaboration. The steps are well-defined, but it's not a trivial change.

  
points
05
 A significant piece of work. It has more complexity and a higher degree of uncertainty than smaller tasks. It might require some research, a design discussion, or collaboration across multiple teams.

  
points
08
 Complex task with a lot of uncertainty. It's often the largest size that a team will consider for a single sprint. It likely requires a detailed breakdown and multiple team members to complete.

  
points
13
 Any task that feels like it's a "13" or more is considered an Epic. These are too large and uncertain to be worked on in a single sprint and should be broken down.

  
Priority
High
 This is for work that is critical and needs to be addressed in the next sprint. It's often for urgent bugs, important features, or a major piece of technical debt that is blocking other work.

  
Priority
Low
 This is for work that is good to do but not urgent. It's a candidate for being worked on when there's extra time or when it rises in importance. It can stay in the backlog for a longer period.

  
Priority
Medium
 This is for important work that should be done soon, but isn't as urgent as a high-priority item. It will likely be considered for the next 1-2 sprints.

  
Sprint Status
Blocked
 Work items that cannot progress due to external dependencies, blockers, or issues that need resolution.

  
Sprint Status
Done
 Work items that have been completed, reviewed, and integrated. No further work is needed within the current sprint.

  
Sprint Status
In Progress
 Work items currently being worked on by a team member. Active development, research, or implementation is happening.

  
Sprint Status
Review
 Work items that are complete but need review, testing, or approval before being marked as done.

  
Sprint Status
To Do
 Work items that are committed to the sprint but haven't been started yet. These items are ready to be picked up by team members.

  
Technical Debt
Work that addresses shortcuts, workarounds, or suboptimal implementations that were done previously to meet deadlines. These items improve code quality, maintainability, or performance without adding new features.

  
Work Item
Bug
 Issues where the system is not working as intended or designed. Bugs represent deviations from expected behavior.

  
Work Item
Epic
 Large initiatives that are broken down into multiple smaller work items (user stories, tasks, bugs). Epics represent significant features or projects that span multiple sprints.

  
Work Item
Spike
 Time-boxed research, investigation, or proof-of-concept work to reduce uncertainty or explore solutions. Spikes help inform future development decisions.

  
Work Item
Task
 Used to break down larger user stories into smaller, manageable implementation pieces. They focus on the specific technical steps needed to deliver the user story's value.

  
Work Item
User Story
 User-facing features or functionality that can be framed from a user perspective, describing what someone wants to accomplish and why.

No labels
ai-review-please
freeze-break-request
post-freeze
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
infra/ansible!3297
No description provided.