Skip to content
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

Feature/certutil pin #14

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

sboyd-m
Copy link

@sboyd-m sboyd-m commented Jul 18, 2024

We had some patches applied to a fork of the spacepants module, and we need to update to this new module. However, it seems some of the old bugs were never fixed, so here are updated versions of the patches. We really, really don't want to drag around another fork.

To quote other Puppet contributors:

This Misconception lies in the Code since the most early Days.

When adding one's own SSL bundle, the cert db is generated with a
password, but the password was never used by the 'exec' that changes the
trust settings. This results in 'certutil' trying to open a terminal to
prompt for the password, which fails, which crashes the Puppet run.

The short version is that the temporary password file must always be
generated and used, not just when generating self-signed certs.
Currently, there is a bug in that supplying 'cert_name' into the ssl
hash will cause the Exec that changes the trust to operate on that name,
however the pk12 creation step uses the server fqdn for the name. The
result is the trust-modify Exec failing, crashing the Puppet run.
@fraenki fraenki self-assigned this Oct 9, 2024
@fraenki
Copy link
Member

fraenki commented Oct 9, 2024

@sboyd-m, would you please help me to understand the changes you've made... Which bugs were adressed/fixed? Which config triggers these bugs? Is anything backwards-incompatible in this PR?

@fraenki fraenki added the bug Something isn't working label Oct 9, 2024
@sboyd-m
Copy link
Author

sboyd-m commented Oct 9, 2024

Is there not enough information in the commit messages? I described the bugs there, let me know if something is unclear so I can reword them.

Regarding backwards compatibility, I believe it should be fine, since the only changes are to exec resources which are refreshonly, but I can't say I explicitly tested this on running-infra, only to create new infra.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants