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

Use IPA CA cert in Custodia secrets client #506

Closed
wants to merge 1 commit into from

Conversation

tscherf
Copy link
Contributor

@tscherf tscherf commented Feb 24, 2017

No description provided.

@tiran
Copy link
Member

tiran commented Feb 24, 2017

Why do you propose to change the settings? By default python-requests enforces certificate validation. Without additional settings, it uses the system trust store. The IPA root CA is injected into the system trust store.

@HonzaCholasta
Copy link
Contributor

We don't want to trust certificates issued by random internet CAs, this is how it should have been from the beginning. A commit message would be nice though.

@tscherf, please add this ticket URL to the commit message: https://fedorahosted.org/freeipa/ticket/6686

@tiran
Copy link
Member

tiran commented Feb 24, 2017

Please change the title of the commit, too. It's implies that we did not verify certs in the past.

In the future please don't call the system trust store a random collection of CAs. It's diminishing and vilifying the hard work of the security team to provide a secure selection of CA certs. This change is purely an attempt to harden IPA and use the same selection of CAs everywhere.

@tscherf
Copy link
Contributor Author

tscherf commented Feb 24, 2017

When the system wide trust store is supposed to be used here, then something else must be broken somewhere in the verification code. Without explicitly using the IPA trust anchor stored in IPA_CA_CRT, the installer failed with an "[SSL: CERTIFICATE_VERIFY_FAILED]" error. We have seen this in CA-less and chained CA setups.

@tscherf
Copy link
Contributor Author

tscherf commented Feb 24, 2017

Sorry, closed this by mistake.

@tscherf tscherf reopened this Feb 24, 2017
@tiran
Copy link
Member

tiran commented Feb 24, 2017

LGTM, but I want @simo5 to give the final ACK.

Since Custodia is only used during replica installation on an enrolled system, ipa-client-install has already provided the certificate. I don't see any issue in the proposed fix.

ipaserver.secrets.client does not yet use Custodia's own client library. I'll keep the problem in mind once we have updated to recent Custodia version.

@tiran tiran requested review from simo5 and tiran February 24, 2017 11:43
@tiran tiran changed the title added ssl verification Use IPA CA cert in Custodia secrets client Feb 24, 2017
@simo5
Copy link
Contributor

simo5 commented Feb 24, 2017

Works for me.

@frasertweedale
Copy link
Contributor

frasertweedale commented Feb 27, 2017

@tiran FYI custodia is also used for Lightweight CA key replication, at any time a new LWCA gets created, to propagate its signing key among replicas.

@tiran tiran added the ack Pull Request approved, can be merged label Feb 27, 2017
@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Feb 27, 2017
@HonzaCholasta
Copy link
Contributor

@tscherf tscherf deleted the bz1419735 branch February 27, 2017 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
5 participants