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

Create custom image that does not have ssh keys stored #343

Merged
merged 26 commits into from
Mar 3, 2023

Conversation

roypaulin
Copy link
Collaborator

All keys that were stored in the image have been removed.
For dbadmin ssh keys in particular, a new variable has been added (NO_SSH_KEYS) that will allow to create an image that do not have those static keys stored inside.
e2e-leg-1 as well as github actions have been modified to use that custom image.

@roypaulin roypaulin requested a review from spilchen March 1, 2023 18:16
@roypaulin
Copy link
Collaborator Author

I am curious about why on build-images->build-server-minimal->Login to Docker Hub there is inputs.full_vertica_image instead of inputs.minimal_vertica_image?

@roypaulin roypaulin self-assigned this Mar 1, 2023
Copy link
Collaborator

@spilchen spilchen left a comment

Choose a reason for hiding this comment

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

This is good. I had a few comments/suggestions.

uses: spilchen/switch-case-action@v2
id: nosshkeys_vertica_image
with:
default: ghcr.io/${{ github.repository_owner }}/vertica-k8s:${{ github.sha }}-nosshkeys
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should rebase to pick up Duane's latest workflow change about needing the name to be lowercase. We will need to apply that change here too.


- name: Login to Docker Hub
uses: docker/login-action@v2
if: ${{ inputs.full_vertica_image != '' && startsWith(inputs.full_vertica_image, 'docker.io') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be checking inputs.nosshkeys_vertica_image

Suggested change
if: ${{ inputs.full_vertica_image != '' && startsWith(inputs.full_vertica_image, 'docker.io') }}
if: ${{ inputs.nosshkeys_vertica_image != '' && startsWith(inputs.nosshkeys_vertica_image, 'docker.io') }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I will. This is related to my question above.

@@ -68,15 +70,15 @@ RUN set -x \
# versions at once.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update this comment to explain what we are doing with NO_SSH_KEYS.

Suggested change
# versions at once.
# versions at once. This step is required if not including SSH keys because
# conditional copy isn't a thing in Docker. If no SSH keys are to be included,
# then the next RUN will remove the key we just added.

docker-vertica/Dockerfile Show resolved Hide resolved
@@ -23,6 +23,7 @@
#
# wander around in the image looking for things you can remove
rm -r -f \
/opt/vertica/config/* \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be problematic for vertica-demo. They use the vertica-k8s image, and probably rely on the config directory to be set up. Can we change this to remove specific things in /config? The agent keys, the keys for the https_certs, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I will delete specific keys files.

@spilchen
Copy link
Collaborator

spilchen commented Mar 2, 2023

I am curious about why on build-images->build-server-minimal->Login to Docker Hub there is inputs.full_vertica_image instead of inputs.minimal_vertica_image?

I think that is a bug. Can you fix it?

@roypaulin
Copy link
Collaborator Author

I am curious about why on build-images->build-server-minimal->Login to Docker Hub there is inputs.full_vertica_image instead of inputs.minimal_vertica_image?

I think that is a bug. Can you fix it?

I will.

@@ -31,7 +32,9 @@ rm -r -f \
/opt/vertica/oss/python*/lib/python*/site-packages/pip \
/opt/vertica/oss/python*/lib/python*/config-[0-9]* \
/opt/vertica/oss/python*/lib/python*/tkinter \
/opt/vertica/oss/python*/lib/python*/idlelib
/opt/vertica/oss/python*/lib/python*/idlelib \
/opt/vertica/oss/python*/lib/python*/site-packages/Cryptodome/SelfTest/PublicKey/test_vectors/ECC \
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a general fix, could we remove all test directories for packages under site-package? We can match test and *Test.

&& chmod 700 /home/dbadmin/.ssh \
&& chmod 600 /home/dbadmin/.ssh/* \
&& chown -R dbadmin:verticadba /home/dbadmin/ \
&& chmod go-w /etc/ssh/sshd_config.d/* /etc/ssh/ssh_config.d/*
&& chmod go-w /etc/ssh/sshd_config.d/* /etc/ssh/ssh_config.d/* \
&& if [[ ($NO_SSH_KEYS == "YES" || $NO_SSH_KEYS == "yes") ]] ; then \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we simplify this to do a case-insensitive compare?

Suggested change
&& if [[ ($NO_SSH_KEYS == "YES" || $NO_SSH_KEYS == "yes") ]] ; then \
&& if [[ ${NO_SSH_KEYS^^} == "YES" ]] ; then \

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I can also do the same with MINIMAL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that would be good thanks.

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.

2 participants