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

Remove demo certificates when DISABLE_INSTALL_DEMO_CONFIG is set to true #240

Closed
wants to merge 1 commit into from

Conversation

danpawlik
Copy link

@danpawlik danpawlik commented Aug 17, 2021

Description

The demo certificates exists in official Opensearch container images.
Sometimes it may raise an issue to start properly the opensearch
service. In that case, if DISABLE_INSTALL_DEMO_CONFIG variable is set to
true, demo certificates will be removed before opensearch will start.

Issues Resolved

#254

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

The demo certificates exists in official Opensearch container images.
Sometimes it may raise an issue to start properly the opensearch
service. In that case, if DISABLE_INSTALL_DEMO_CONFIG variable is set to
true, demo certificates will be removed before opensearch will start.

Signed-off-by: Daniel Pawlik <[email protected]>
@dblock
Copy link
Member

dblock commented Aug 18, 2021

I was trying to find where DISABLE_INSTALL_DEMO_CONFIG is used/declared and only found the docs, https://github.com/opensearch-project/documentation-website/blob/119b9e79ec8d757a097ac164b00aaf7b3ca235cd/_opensearch/install/docker-security.md. Is this something we removed/broke in 1.0?

If yes,

  1. Should we be copying https://github.com/opendistro-for-elasticsearch/opendistro-build/blob/aabb00fb349e1eb1b916ed68c22fb5ef8be8abc7/elasticsearch/docker/build/elasticsearch/bin/docker-entrypoint.sh#L85 instead of merging this PR?
  2. Should we remove it from the documentation and enable this feature in some other way?
  3. Should we fix the root cause? I'm thinking the demo config should always work and there should be no reason to remove it.

@danpawlik
Copy link
Author

@dblock thanks for reply.
In my case, I can not run Opensearch without removing the demo certificates, so IMHO the container images are broken.
From proposed solution, number 1 seems to be the best.
Should I port the functionality and replace my commit ?

@nebulon42
Copy link

nebulon42 commented Aug 29, 2021

@dblock not sure that your suggested 1. will work because the demo files are in the image from the beginning and not created by the specified script. At least I tried to replace the script by an empty file like suggested in opendistro-for-elasticsearch/opendistro-build#10 and it didn't change anything. So I gather that the script is not executed/the files are already there. Also the creation timestamps for the demo certificates suggest that they have been created together with the image.

ad 3. demo config will be insecure for production as the certificates are not generated anew but are always the same on all systems, at least that is what I think by looking at the script install_demo_configuration.sh. And currently the same certificates are anyway in all downloaded images which is even worse.

@danpawlik
Copy link
Author

any news @dblock ?

@dblock
Copy link
Member

dblock commented Sep 7, 2021

To me an environment variable should not be permanently disabling (deleting) configuration, especially anything security-related, demo or not, at runtime. If this script runs in a production environment, DISABLE_INSTALL_DEMO_CONFIG would disable that, too, btw.

I don't know much about demo images, or how they are created, but I'll do my best to help. @peterzhuamazon can you point us to the right direction?

It looks like the official images ship with a demo security certificate configuration? I like the idea that they would be generated when the container starts the first time. Either way, need something more robust than an environment variable to replace them.

@peterzhuamazon
Copy link
Member

To me an environment variable should not be permanently disabling (deleting) configuration, especially anything security-related, demo or not, at runtime. If this script runs in a production environment, DISABLE_INSTALL_DEMO_CONFIG would disable that, too, btw.

I don't know much about demo images, or how they are created, but I'll do my best to help. @peterzhuamazon can you point us to the right direction?

It looks like the official images ship with a demo security certificate configuration? I like the idea that they would be generated when the container starts the first time. Either way, need something more robust than an environment variable to replace them.

We are doing the same practice by running install demo configuration script which comes from security repo. This script will be run either use tarball or use docker images.
I have yet to be able to reproduce the errors.
https://github.com/opensearch-project/security/blob/main/tools/install_demo_configuration.sh

Also, we change the way this script is run just like @dblock pointed out.
We will always run install demo configuration with a one time setup script here:
https://github.com/opensearch-project/opensearch-build/blob/main/scripts/opensearch-onetime-setup.sh#L16

If needed we can change this behavior back to this:
https://github.com/opendistro-for-elasticsearch/opendistro-build/blob/aabb00fb349e1eb1b916ed68c22fb5ef8be8abc7/elasticsearch/docker/build/elasticsearch/bin/docker-entrypoint.sh#L85

Let me know what you guys think about it.

Thanks.

@nebulon42
Copy link

@peterzhuamazon I think reverting to the previous behaviour will be the only way without larger rework on the container images side. Only downside of this approach is that the changes of this script will go away on container termination unless the whole /usr/share/opensearch directory being persisted somewhere. But it ensures that the demo certificates are ephemeral which IMO is an important point. You can always delete the certs from disk in a tarball install but you cannot get them out of a container image without extending the image (or with this PR but I think this is only a workaround).

@peterzhuamazon
Copy link
Member

Got it. Let me work on it tomorrow. Thanks for feedbacks @nebulon42.

@peterzhuamazon
Copy link
Member

@nebulon42 you mind close this PR so I can open a new one with the original behavior?
Thanks.

@nebulon42
Copy link

I think @peterzhuamazon meant to address @danpawlik in the last comment.

@danpawlik
Copy link
Author

Thanks @peterzhuamazon and @nebulon42.
Closing PR.

@danpawlik danpawlik closed this Sep 9, 2021
@peterzhuamazon
Copy link
Member

I think @peterzhuamazon meant to address @danpawlik in the last comment.

Yes, I apologize. And thanks both @danpawlik and @nebulon42 will send the new PR later.

@peterzhuamazon
Copy link
Member

Fixed now: #436

sf-project-io pushed a commit to softwarefactory-project/ansible-role-elastic-recheck that referenced this pull request Dec 16, 2021
The new Opensearch image should contain patch [1], so
the customized version is not needed.

[1] opensearch-project/opensearch-build#240

Change-Id: I7b5092df2b2dc84671b442a73f413c8a44de280f
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.

4 participants