jjethwa / rundeck

GNU General Public License v3.0
123 stars 137 forks source link

Import CA certs to new keystore #79

Closed johnseekins closed 7 years ago

johnseekins commented 7 years ago

We should import CA certs to the keystore created so Java can actually verify external certs (for things like the EC2 node clasifier).

jjethwa commented 7 years ago

Hi @johnseekins

Thanks for the PR. Is DM_BASE_CFG_PATH set somewhere?

johnseekins commented 7 years ago

Dang...that's something from my testing. What a silly oversight.

jjethwa commented 7 years ago

Sorry @johnseekins

One last change, sorry 😛 I think the keystore command should be wrapped in a file check if statement to make sure we don't accidentally override one provided by a user

if [ ! -f /etc/rundeck/ssl/keystore ]; then
  sudo -u rundeck keytool -importkeystore -destkeystore /etc/rundeck/ssl/keystore -srckeystore /etc/ssl/certs/java/cacerts -deststoretype JKS -srcstoretype JKS -deststorepass ${TRUSTSTORE_PASS} -srcstorepass changeit > /dev/null
fi
johnseekins commented 7 years ago

I'm confused...that command is in an if block:

   if [ ! -f /etc/rundeck/ssl/truststore ]; then
       echo "=>Generating ssl cert"
       sudo -u rundeck mkdir -p /etc/rundeck/ssl
       sudo -u rundeck keytool -keystore /etc/rundeck/ssl/keystore -alias rundeck -genkey -keyalg RSA -keypass ${KEYSTORE_PASS} -storepass ${TRUSTSTORE_PASS} -dname "cn=localhost, o=OME, c=DE"
       sudo -u rundeck keytool -importkeystore -destkeystore /etc/rundeck/ssl/keystore -srckeystore /etc/ssl/certs/java/cacerts -deststoretype JKS -srcstoretype JKS -deststorepass ${TRUSTSTORE_PASS} -srcstorepass changeit > /dev/null
       cp /etc/rundeck/ssl/keystore /etc/rundeck/ssl/truststore
   fi
jjethwa commented 7 years ago

Hi @johnseekins

Yes, that if blocks checks for the truststore file, but we should also check for the keystore file's existence before touching it. This is just to make sure we don't override a keystore file that has been introduced by a volume.

jjethwa commented 7 years ago
if [ ! -f /etc/rundeck/ssl/truststore ]; then
       echo "=>Generating ssl cert"
       sudo -u rundeck mkdir -p /etc/rundeck/ssl
       sudo -u rundeck keytool -keystore /etc/rundeck/ssl/keystore -alias rundeck -genkey -keyalg RSA -keypass ${KEYSTORE_PASS} -storepass ${TRUSTSTORE_PASS} -dname "cn=localhost, o=OME, c=DE"
       if [ ! -f /etc/rundeck/ssl/keystore ]; then
              sudo -u rundeck keytool -importkeystore -destkeystore /etc/rundeck/ssl/keystore -srckeystore /etc/ssl/certs/java/cacerts -deststoretype JKS -srcstoretype JKS -deststorepass ${TRUSTSTORE_PASS} -srcstorepass changeit > /dev/null
       fi
       cp /etc/rundeck/ssl/keystore /etc/rundeck/ssl/truststore
   fi
johnseekins commented 7 years ago

Based on the if statement you're recommending, that second command will never happen.

What I was hoping for that additional import command was this: If the file doesn't exist -> create it then import any ca certificates in to that file so that while Rundeck is using that file it can properly verify external certs.

This import would only happen when the file didn't exist.

jjethwa commented 7 years ago

Hi @johnseekins

The if statement I recommended will stop the import command from running if /etc/rundeck/ssl/keystore exists already. This is for the corner case where a user supplies /etc/rundeck/ssl/keystore via a volume but does not provide /etc/rundeck/ssl/truststore I don't see a valid case for a user to do that, but you never know, so it's best not to touch their stuff 😛

johnseekins commented 7 years ago

I completely agree, and I just realized that the outer if statement wasn't what I thought it was truststore vs keystore...ugh. Given that I actually understand now...how about I try that again...

jjethwa commented 7 years ago

Thanks @johnseekins

I just merged the changes. It'll be in latest in a few minutes. Appreciate your PRs as always 😃