Skip to content

fix(etcd): set correct permissions on certs generated on first etcd node - #13447

Open
mehrdadbn9 wants to merge 1 commit into
kubernetes-sigs:masterfrom
mehrdadbn9:fix/etcd-cert-perms-13250
Open

fix(etcd): set correct permissions on certs generated on first etcd node#13447
mehrdadbn9 wants to merge 1 commit into
kubernetes-sigs:masterfrom
mehrdadbn9:fix/etcd-cert-perms-13250

Conversation

@mehrdadbn9

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it

On the first etcd node, make-ssl-etcd.sh generates the etcd certs and moves them into /etc/ssl/etcd as root:root with umask-based permissions (0600 for keys, 0644 for certs). Every other etcd node receives the certs through the slurp/copy path with the correct etcd:${etcd_cert_group} ownership and 0640 mode, so only the generating node is inconsistent.

This broke an etcd backup job running on the first control-plane node with a read-permission error (see issue for repro).

This PR adds one normalization task in roles/etcd/tasks/gen_certs_script.yml right after cert generation on the first etcd node. It sets the same owner/group and the same mode split the copy path already uses:

  • 0644 for public certs (ca.pem, admin-.pem, member-.pem, node-*.pem)
  • 0600 for private keys (*-key.pem)

The task reuses etcd_owner / etcd_cert_group so behavior stays consistent with the copy tasks, and it only runs when gen_certs is true (generation happened).

Which issue(s) this PR fixes

Fixes #13250

Special notes for your reviewer

  • Delegation matches the generation task: delegate_to: "{{ groups['etcd'][0] }}" + run_once: true, so it runs exactly where the certs are generated.
  • File names use {{ inventory_hostname }} because that is how the generator names certs (same pattern as the slurp/copy tasks above).
  • Modes deliberately keep 0600 on keys (stricter than the copy path's 0640) to match upstream etcd PKI conventions; happy to align to 0640 if reviewers prefer exact parity with the copy path.

Does this PR introduce a user-facing change?

Fix etcd certificate permissions on the first etcd node: generated certs are now normalized to the same ownership (etcd:${etcd_cert_group}) and mode split (0644 certs / 0600 keys) as the certs copied to other etcd nodes.

@kubernetes-prow kubernetes-prow Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 1, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

Hi @mehrdadbn9. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kubernetes-prow kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 1, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mehrdadbn9
Once this PR has been reviewed and has the lgtm label, please assign mzaian for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mehrdadbn9
mehrdadbn9 force-pushed the fix/etcd-cert-perms-13250 branch from 9149ffd to 5fd73d1 Compare September 1, 2026 18:32
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 1, 2026
@yankay

yankay commented Sep 2, 2026

Copy link
Copy Markdown
Member

/ok-to-test

@kubernetes-prow kubernetes-prow Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 2, 2026
@yankay
yankay requested a lite review from Copilot September 2, 2026 01:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new file: loop can create empty cert/key files when a listed path doesn’t exist (e.g., node certs on a dedicated etcd host), and the proposed modes don’t currently match the existing copy-path behavior, so the “normalization” may still be inconsistent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses inconsistent ownership/permissions of etcd certificates on the first etcd node (the node where make-ssl-etcd.sh generates certs), so that jobs reading the certs (e.g., backups) behave consistently across all etcd nodes.

Changes:

  • Adds a post-generation normalization task in roles/etcd/tasks/gen_certs_script.yml to set owner/group/mode for certs generated on the first etcd node.
File summaries
File Description
roles/etcd/tasks/gen_certs_script.yml Adds a new task intended to normalize ownership/mode of generated etcd PKI artifacts on the first etcd node.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread roles/etcd/tasks/gen_certs_script.yml Outdated
Comment on lines +49 to +69
- name: Gen_certs | Normalize permissions on generated etcd certs on first etcd node
file:
path: "{{ item.path }}"
owner: "{{ item.owner | default(etcd_owner) }}"
group: "{{ item.group | default(etcd_cert_group) }}"
mode: "{{ item.mode | default('0640') }}"
loop:
- { path: "{{ etcd_cert_dir }}/ca.pem", mode: "0644" }
- { path: "{{ etcd_cert_dir }}/ca-key.pem", mode: "0600" }
- { path: "{{ etcd_cert_dir }}/admin-{{ inventory_hostname }}.pem", mode: "0644" }
- { path: "{{ etcd_cert_dir }}/admin-{{ inventory_hostname }}-key.pem", mode: "0600" }
- { path: "{{ etcd_cert_dir }}/member-{{ inventory_hostname }}.pem", mode: "0644" }
- { path: "{{ etcd_cert_dir }}/member-{{ inventory_hostname }}-key.pem", mode: "0600" }
- { path: "{{ etcd_cert_dir }}/node-{{ inventory_hostname }}.pem", mode: "0644" }
- { path: "{{ etcd_cert_dir }}/node-{{ inventory_hostname }}-key.pem", mode: "0600" }
delegate_to: "{{ groups['etcd'][0] }}"
run_once: true
when:
- gen_certs
- inventory_hostname == groups['etcd'][0]

On the first etcd node, make-ssl-etcd.sh generates certs and moves
them to the etcd cert dir with root:root ownership and umask-based
permissions. Other nodes receive certs via the slurp/copy path with
correct ownership and modes, so only the generating node ends up
with wrong perms.

Normalize the generated certs on the first etcd node:
- public certs (ca, member-*, admin-*, node-* for control-plane
  hosts that generate certs): owner etcd, mode 0644
- private keys: same ownership, mode 0600

The stems are derived from groups['etcd'] and the intersection of
gen_node_certs_True with kube_control_plane, matching exactly the
hosts for which make-ssl-etcd.sh generates member/admin/node certs
in this task file. Referencing node-<etcd-host> certs directly fails
on topologies with separate etcd and control-plane nodes because
those files are named after control-plane hosts.

Fixes the CI failures on ubuntu24-ha-separate-etcd and
ubuntu24-cilium-sep seen in the previous revision of this change.

Signed-off-by: Mehrdad Biukian Naeini <mehrdadbiukian@gmail.com>
@mehrdadbn9
mehrdadbn9 force-pushed the fix/etcd-cert-perms-13250 branch from 5fd73d1 to e180f63 Compare September 2, 2026 03:51
@kubernetes-prow kubernetes-prow Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 2, 2026
@mehrdadbn9

Copy link
Copy Markdown
Contributor Author

Root-caused the 2 CI failures (ubuntu24-ha-separate-etcd, ubuntu24-cilium-sep): the previous revision referenced node-{{ inventory_hostname }} certs on the first etcd node, but node-* certs are named after control-plane hosts (make-ssl-etcd.sh generates them from the HOSTS variable = gen_node_certs_True ∩ kube_control_plane). With etcd and control-plane on separate machines those files don't exist on the etcd node and the file task fails.

The pushed revision derives the file list from the same group expressions the generator uses: member/admin for every etcd host, node-* for gen_node_certs_True ∩ kube_control_plane hosts, plus ca.

Verified locally with ansible 2.16 against a separate-etcd topology (etcd1/etcd2 + cp1/cp2): certs land 0644, keys 0600, both tasks ok, and the absent-file case that broke CI is gone because the file list now matches exactly what the generator produces on that node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent etcd certificate permissions on first control-plane / etcd node

3 participants