smolnar82 commented on code in PR #1209:
URL: https://github.com/apache/knox/pull/1209#discussion_r3128777940
##########
gateway-docker/src/main/resources/docker/gateway-entrypoint.sh:
##########
@@ -183,14 +195,18 @@ fi
if [[ -n $CA_FILE ]] && [[ -f ${CA_FILE} ]]
then
echo "Creating truststore with provided CA certificate/s."
- importMultipleCerts "$CA_FILE"
+ if ! importMultipleCerts "$CA_FILE"; then
+ echo "WARN: Unable to import CA certs from [${CA_FILE}]. Continuing
startup..."
+ fi
fi
# Import custom certs one by one
if [[ -n $CUSTOM_CERT ]] && [[ -f ${CUSTOM_CERT} ]]
then
echo "Importing Custom certs."
- importMultipleCerts "$CUSTOM_CERT"
+ if ! importMultipleCerts "$CUSTOM_CERT"; then
+ echo "WARN: Unable to import custom certs from [${CUSTOM_CERT}].
Continuing startup..."
Review Comment:
I think this is redundant:
- the log in line 197 already tells we are importing custom certs
- the log in importMultipleCerts will list the certs that were not imported
##########
gateway-docker/src/main/resources/docker/gateway-entrypoint.sh:
##########
@@ -53,15 +54,26 @@ importMultipleCerts() {
# step 2), increment counter when last line of cert is found
for N in $(/usr/bin/seq 0 $((CERTS - 1))); do
ALIAS="${FILE%.*}-$N"
- /bin/cat "$FILE" |
+ # Make import idempotent across restarts when truststore is persisted.
+ keytool -delete \
Review Comment:
The `import` command below adds the `-trustcacerts` flag, but this new
`delete` doesn't. Why?
##########
gateway-docker/src/main/resources/docker/gateway-entrypoint.sh:
##########
@@ -201,8 +217,9 @@ then
amazon-ca-1:/home/knox/cacrts/AmazonRootCA1.cer
amazon-ca-2:/home/knox/cacrts/AmazonRootCA2.cer
amazon-ca-3:/home/knox/cacrts/AmazonRootCA3.cer
- amazon-ca-4:/home/knox/cacrts/AmazonRootCA4.cer
- letsencrypt-stg-root:/home/knox/cacrts/letsencrypt-stg-root-x1.pem"
Review Comment:
What happened to this cert? I see that you added two new certs:
- isrgrootx1:/home/knox/cacrts/isrgrootx1.pem
- isrgrootx2:/home/knox/cacrts/isrg-root-x2.pem
Based on the `curl` command in `Dockerfile`, this seems to be ok. Can you
please give a 1-2 sentence explanation on this?
##########
gateway-docker/src/main/resources/docker/gateway-entrypoint.sh:
##########
@@ -201,8 +217,9 @@ then
amazon-ca-1:/home/knox/cacrts/AmazonRootCA1.cer
amazon-ca-2:/home/knox/cacrts/AmazonRootCA2.cer
amazon-ca-3:/home/knox/cacrts/AmazonRootCA3.cer
- amazon-ca-4:/home/knox/cacrts/AmazonRootCA4.cer
- letsencrypt-stg-root:/home/knox/cacrts/letsencrypt-stg-root-x1.pem"
+ amazon-ca-4:/home/knox/cacrts/AmazonRootCA4.cer
Review Comment:
nit: extra space at the beginning of this line
##########
gateway-docker/src/main/resources/docker/gateway-entrypoint.sh:
##########
@@ -86,21 +98,21 @@ export GATEWAY_SERVER_RUN_IN_FOREGROUND=true
# Create Master secret
if [[ -n ${KNOX_MASTER_SECRET} ]]
then
- MASTER_SECRET=$(/bin/cat "${KNOX_MASTER_SECRET}" 2>/dev/null)
+ MASTER_SECRET=$(/bin/cat "${KNOX_MASTER_SECRET}" 2> /dev/null)
fi
if [[ -n ${MASTER_SECRET} ]]
then
echo "Using provided knox master secret [env:MASTER_SECRET]"
else
echo "Using default knox master secret"
- MASTER_SECRET="knox"
+ MASTER_SECRET="!apacheknox!"
Review Comment:
Why is this change? Neither this new value, nor the old one is something a
password cracker wouldn't break in 5 secs (assuming the attacker wouldn't check
this file and know already the default password).
##########
gateway-docker/src/main/resources/docker/gateway-entrypoint.sh:
##########
@@ -183,14 +195,18 @@ fi
if [[ -n $CA_FILE ]] && [[ -f ${CA_FILE} ]]
then
echo "Creating truststore with provided CA certificate/s."
- importMultipleCerts "$CA_FILE"
+ if ! importMultipleCerts "$CA_FILE"; then
+ echo "WARN: Unable to import CA certs from [${CA_FILE}]. Continuing
startup..."
Review Comment:
I think this is redundant:
- the log in line 197 already tells we are importing CA certs
- the log in importMultipleCerts will list the certs that were not imported
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
