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

Patched Scripts for New LE CAs #49

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

matthewprobasco
Copy link

@matthewprobasco matthewprobasco commented Jun 7, 2024

Added new LE CA's to setup-le.sh

Added new LE CA's to setup-le.sh and also added logic to copy full CA path to cert in renew-le.sh
Corrected Cert Order and removed the Root CA from the chain.
renew-le.sh Outdated Show resolved Hide resolved
@rcritten
Copy link

Can you add a link to the ticket you filed, #48

And add signed-off-by to the commit? (git commit -s)

reverted since the setup script will add the full chain as expected.

Closes freeipa#48
reverted since the setup script will add the full chain as expected.

Closes freeipa#48

Signed-off-by: Matthew Probasco <[email protected]>
@matthewprobasco matthewprobasco requested a review from rcritten June 10, 2024 15:39
@vinnyvinny1989
Copy link

Thank for your work guys, I successfully fixed problem with Freeipa and new LE CA's

@osirisinferi
Copy link

Please don't pin intermediate certificates. That's a terrible idea. If there's a need for pinning, only pin roots and check the chain using the signatures. Thank you.

@aarongable
Copy link

Hi, Let's Encrypt tech lead here -- @osirisinferi is correct, please do not hardcode the four current intermediates. They can (and will!) change with no warning, requiring another emergency change like this.

Additionally, the place this script is downloading the intermediates from is the Let's Encrypt website, not API, so those paths are not guaranteed to remain static in the long term, and this script my break even without an intermediate change.

It should not be necessary to hard-code these intermediates at all. The issuing intermediate is provided alongside the newly issued certificate at the end of the issuance process.

@rcritten
Copy link

I'm having a hard time wording this without sounding like a complete jerk so please bear with me.

The freeipa project has a demo installation that uses LE certificates. This script was written to handle its initial setup and renewal. It is as basic as things come. As long as it works we focus on other things. We're better open source stewards elsewhere but when a ball can be dropped it's always this one.

Others have found the script useful and we've patched it here and there but honestly we tend to only react to things. Things like user-submitted patches which often takes us months to get around to merging. We're not proud of it, but it's the truth.

The way that non-IPA certs are loaded into IPA is via a tool that was written in response to so many people getting PKI wrong.. The tool verifies that the cert has basic X.509v3 compliance, has the right extensions, has extensions that the IPA crypto libraries can understand and does chain validation using NSS. NSS requires the full chain. Hence retrieving the chain bit by bit and adding it to the IPA CA store.

Granted in the context of LE much of this is unnecessary but the tool is generic and lots of people use their own self-signed CAs to sign the IPA CA and/or its certificates. So we need to check because troubleshooting TLS issues is harder than verifying that the certs are sane.

Whether and how we can leverage API to retrieve these intermediate certs, or somehow pull apart the renewed cert and load in the chain, I don't know. This project has a pretty low bar. Basically as long as it works to renew the freeipa demo certs we let it do its thing. I don't know the scope of work and don't want @matthewprobasco to feel responsible for what could turn out to be a lot more work.

@aarongable
Copy link

Honestly, I get it -- the change here is clearly the minimal change necessary to keep things working as they previously have, and sometimes the minimal change is the best change. I truly have no objection (not that you'd have to listen to an objection of mine anyway!) to the project merging this PR as-is, especially since it's from an outside contributor.

I guess I'd just ask that the project consider filing a bug to track this behavior as something to fix. The unfortunate truth is that when things like this break, the Let's Encrypt team gets support requests that we and our wonderful volunteer community then have to try to understand. Even if it doesn't get addressed in the immediate future, having a well-documented bug on file will help other folks who run into similar issues in the future solve the problem that time, and help future contributors know what kind of PR to file -- whether it's another band-aid or a larger change.

@kimdre
Copy link

kimdre commented Aug 24, 2024

What should we do instead to fix the current SSL issue then? My UI login and ipa tools are all broken because of SSL errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants