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

Refs #28924: Drop amqp key and truststore + generate Artemis client certs #275

Merged
merged 1 commit into from Apr 20, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Mar 6, 2020

No description provided.

manifests/candlepin.pp Outdated Show resolved Hide resolved
@@ -121,31 +118,15 @@
key_file => $client_key,
cert_file => $client_cert,
} ~>
file { $amqp_store_dir:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should ensure it's absent to clean up after ourselves, but I don't recall if we can ensure directories are absent in Puppet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered about that. Should the puppet module clean this up or installer post install task clean this up?

Copy link
Member

Choose a reason for hiding this comment

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

Normally I'd say Puppet but if Puppet can't clean up directories with the file type then I'm ok with a post install task rather than an exec here.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my googling you can do a regular file set to absent with recursive set to true. This just doesn't feel as clean. That weird line where sometimes we cleanup in the puppet module and sometimes we use the puppet module to simply represent the state and the installer does cleanup. I thought we were universally doing the latter. Its hard to know which way to go.

spec/acceptance/candlepin_spec.rb Outdated Show resolved Hide resolved
@jturel
Copy link
Contributor

jturel commented Mar 9, 2020

Seeing this:

[ WARN 2020-03-09T18:11:46 verbose]  Unknown variable: 'artemis_keystore'. (file: /usr/share/foreman-installer/modules/certs/manifests/candlepin.
pp, line: 128, column: 200)

So the subsequent keytool command fails since -destkeystore is missing

@jturel
Copy link
Contributor

jturel commented Mar 17, 2020

@ehelms just pinging to ensure you saw my last update 🏓

@ehelms ehelms force-pushed the refs-28924 branch 2 times, most recently from fdf0018 to 64867f1 Compare March 26, 2020 14:31
manifests/candlepin.pp Outdated Show resolved Hide resolved
@ehelms ehelms force-pushed the refs-28924 branch 2 times, most recently from 12f26a8 to b598214 Compare March 31, 2020 21:39
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

ACK, leaving it open so it can be merged together with the candlepin change.

@jturel
Copy link
Contributor

jturel commented Apr 13, 2020

I did some testing and was able to connect with SSL client auth from Katello to Artemis.

I basically followed the steps here, extracted the client cert+key from the stores, and used them to connect. Seems like we should be able to approximate that here, and let me know if I can provide any more detail.

@ehelms
Copy link
Member Author

ehelms commented Apr 13, 2020

@jturel Apologies, I'm not following what the action item is regarding server and client keystores.

@jturel
Copy link
Contributor

jturel commented Apr 13, 2020

@jturel Apologies, I'm not following what the action item is regarding server and client keystores.

I need my own clarification then :)

The artemis_client entry you've added to the keystore: what is the purpose of that? Is it for Katello to use? If so, I suppose we'll need puppet-katello to extract the cert and key in PEM format in order to connect to the broker.

Or is it for Candlepin to use? If so, Candlepin is connecting directly to the embedded artemis without SSL since it's running within the same JVM which means it's not necessary, unless you're thinking forward to candlepin connecting to an external Artemis

@ehelms
Copy link
Member Author

ehelms commented Apr 13, 2020

Ahh yes. This client certificate is being added to the keystore/truststore so that it can be used by Katello. The client certificate is already deployed to the system in a place that Katello can consume it. This change is just ensuring its in the truststore. We abuse keystore/truststore by creating one and using it for both.

/etc/pki/katello/certs/java-client.crt
/etc/pki/katello/private/java-client.key

@@ -31,6 +28,8 @@
}

$java_client_cert_name = 'java-client'
$artemis_alias = 'artemis-client'
$artemis_client_dn = "C=${city}, ST=${state}, O=candlepin, OU=${org_unit}, CN=${cname}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the CN should be $hostname rather than $cname

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, well here's the contents of the file:

katelloUser=C=Raleigh, ST=North Carolina, O=candlepin, OU=SomeOrgUnit, CN=[]

Hard to discern what you wanted me to see at that URL..

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh, yea, I see now how this gets handled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to hostname, I forget cname is additional cnames to add.

Copy link
Member

Choose a reason for hiding this comment

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

In SSL you have Common Name (cn) and subject Alternative Name (subjectAltName). cname is DNS terminology.

@ehelms ehelms force-pushed the refs-28924 branch 2 times, most recently from 7d3e549 to 2ef13b0 Compare April 15, 2020 19:04
@@ -31,6 +28,8 @@
}

$java_client_cert_name = 'java-client'
$artemis_alias = 'artemis-client'
$artemis_client_dn = "C=${city}, ST=${state}, O=candlepin, OU=${org_unit}, CN=${hostname}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It actually seems like the order of these matters.... (most specific -> least specific?)

Should be like this: CN=localhost, OU=SomeOrgUnit, O=candlepin, ST=North Carolina, C=US

Note also that C is country, not city

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh yea, sorry you are right on the country.

I modeled this off of "C=US, ST=North Carolina, O=candlepin, OU=SomeOrgUnit, CN=dhcp-8-29-228.lab.eng.rdu2.redhat.com"

openssl x509 -noout -text -in /etc/pki/katello/certs/java-client.crt

Copy link
Contributor

Choose a reason for hiding this comment

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

All good. When I re-order them in login.config it prevents Katello from connecting with an auth failure 🤷

Copy link
Contributor

@jturel jturel left a comment

Choose a reason for hiding this comment

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

ACK. works in prod and dev.

@ehelms ehelms merged commit cbc1fac into theforeman:master Apr 20, 2020
@ekohl ekohl changed the title Refs #28924: Drop amqp key and truststore Refs #28924: Drop amqp key and truststore + generate Artemis client certs Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants