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

Insecure registry support #24

Merged
merged 9 commits into from
Nov 16, 2021

Conversation

tdhooks
Copy link
Contributor

@tdhooks tdhooks commented Sep 19, 2021

Adds support for insecure registries via an insecureRegistries array in sarus.json. This field functions the same as the insecure-registries field in docker's daemon.json. If the registry host for the given pull is listed in this configuration field, sarus will not use secure http by default and will not validate certificates. Security enforcement is enabled by default and will be carried out in all cases except when configured not to.

This PR remains WIP until the following are resolved:

  • Tests: Integration tests for this feature would be rather straightforward, but would also require more than just adding additional tests to the current flow (e.g. hosting an insecure registry to test against), so I want to consult with maintainers before moving forward there.

closes #23

@taliaga
Copy link
Collaborator

taliaga commented Sep 23, 2021

Hi @tdhooks , thanks for putting this MR together, that's great.
All LGTM so far. As you say, I believe it's only missing at least 3 tests:

  1. A test for serverIsInsecure
  2. An integration test checking the pulling from insecure registry fails
  3. An integration test checking the pulling from insecure registry succeeds if properly configured

For 1, a unittest would suffice. test_CLI.cpp is a good source of inspiration.

For 2 and 3:
Integration tests are run through the script sarus-itest-standalone. They take the stadanlone sarus archive and use it inside a docker container to run the python-driven tests. The simplest solution that could be tried would be to run, in the same integration test container, a localhost insecure registry with Docker's registry image using sarus. sarus pull registry:2; sarus run registry:2 &. That could be done as part of _run_cmd_in_container for the integration tests functions. A more engineered and solid way to solve this would require several refactorings of the CI scripts and would be out of scope for this task IMHO.
Two things to consider:

  • To tweak the sarus.json used in the tests to add the insecure registry exception, test_environment_variables.py serves as an example (even if not the cleanest). Alternatively, as part of the _run_cmd_in_container this insecure registry could be set for all integration tests at bash level. That would be even better, as the plan would be to use the local registry for most of the integration tests in the future.
  • The local insecure registry will need an example image which could be pulled right after registry startup.

Wdyt?

@Madeeks Madeeks assigned Madeeks and unassigned Madeeks Sep 23, 2021
@Madeeks Madeeks added the enhancement New feature or request label Sep 23, 2021
@Madeeks Madeeks linked an issue Sep 23, 2021 that may be closed by this pull request
@tdhooks
Copy link
Contributor Author

tdhooks commented Sep 27, 2021

Unit tests are in. I just added an insecure registry to the test config and followed the pattern in test_CLI.cpp.

As for integration tests, simply running sarus pull registry:2; sarus run registry:2 & before tests in _run_cmd_in_container could be tricky, because there'd be no good way to push a test image from the container itself without doing something wacky like docker in docker.

What could work is exposing the container's port 5000 and pushing from the host, but I'm not sure how we'd handle that within the current test framework, because we'd need to run an image and execute the push concurrently, resolve the container network location, and keep dependent events in sync.

It also may make sense to host the registry in another container alongside the test container more as part of the test's environment than the test itself. That would look like, within sarus-itest-standalone, starting and pushing to a registry container before running tests with _run_cmd_in_container and taking it down afterward. The only complication there, besides just adding more moving parts in general, would be container networking, but I don't think it would be a big deal to have _run_cmd_in_container run containers on a bridge network, or add another similar function that does and only use it when warranted.

Of course finding or hosting a registry image that has an image pre-loaded is ideal, but I haven't been able to find one, especially not from a trusted source. Overall I favor the two-container approach if this isn't an option.

@taliaga
Copy link
Collaborator

taliaga commented Oct 6, 2021

Amazing! I suggested the "simpler" option to avoid you the trouble of dealing with yet another moving piece and the cross-container network issue, but clearly you've already seen that coming :)

The way I was thinking before going that path was trying to keep the images on the host (inside the caches folder) and bind-mounting that in the registry (host -> docker -> sarus). Have a look at https://docs.docker.com/registry/deploying/#customize-the-storage-location. The hope is for this to be easier, but it may prove false, in which case we'd be left with the 2-container-based approach.

In any of the two cases, we'll need a preprocessing step for the integration tests that does the pull + tag + push to the local insecure registry of the images used in integration tests (typically alpine, ubuntu, etc.), from the docker container, ideally without leaving root-owned stuff on the host. Maybe pytest can help there with a session-level fixture or maybe easier with just an earlier bash function call. Your call! (no pun intended)

Thanks for your help btw

@tdhooks
Copy link
Contributor Author

tdhooks commented Oct 31, 2021

Integration tests are up. I was able to get the local (in container) registry set up using the cache directory without needing to write root owned files to the host. I think the separate container approach could probably be done without too much trouble using container network mode, but it probably isn't worth it for these few tests.

I wasn't able to get the 'from scratch' test pipeline running, but unless the test environment is appreciably different it should work the same. Worth double checking anyway, though.

@Madeeks
Copy link
Member

Madeeks commented Nov 4, 2021

Hi @tdhooks, thanks for all your work on this PR!
We are having some issues with Travis CI lately, but I have run the tests on my laptop (also the builds from source on OpenSUSE and Ubuntu) and some stages of our internal pipeline and they work fine.

From my point of view everything's there to remove the Draft status and promote this to a proper PR.

I have a few observations which I'll comment directly on the code.

CI/src/integration_tests/test_command_pull.py Outdated Show resolved Hide resolved
doc/config/configuration_reference.rst Outdated Show resolved Hide resolved
src/cli/CommandPull.hpp Outdated Show resolved Hide resolved
@tdhooks tdhooks marked this pull request as ready for review November 5, 2021 00:29
@tdhooks
Copy link
Contributor Author

tdhooks commented Nov 5, 2021

I've addressed and responded to comments and left them up to you all to resolve. All tests pass on my end.

@taliaga
Copy link
Collaborator

taliaga commented Nov 8, 2021

LGTM

@Madeeks
Copy link
Member

Madeeks commented Nov 9, 2021

Hi @tdhooks, code is looking good from my side as well.
As you might be aware from the contribution guidelines, we are required to ask you (as a first time contributor) to sign a Contributors License Agreement (CLA) with ETH Zurich.
I have sent the form to the email address I found in your GitHub profile, could I ask if you have received it?
Thanks a lot!

@tdhooks
Copy link
Contributor Author

tdhooks commented Nov 12, 2021

I have received the CLA form and will get to it soon.

@tdhooks
Copy link
Contributor Author

tdhooks commented Nov 16, 2021

CLA returned and conflicts resolved. Should be good to go.

@Madeeks Madeeks merged commit 9bd835e into eth-cscs:develop Nov 16, 2021
@Madeeks
Copy link
Member

Madeeks commented Nov 16, 2021

I confirm reception of the CLA, and everything looking good in the code.
Thanks again @tdhooks for the first external contribution! 🎉

@tdhooks tdhooks deleted the insecure-registry-support branch December 14, 2021 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for insecure registries
3 participants