-
Notifications
You must be signed in to change notification settings - Fork 145
Add support for docker-credential-stores through environment #1022
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
base: main
Are you sure you want to change the base?
Changes from all commits
e2cd051
b259a76
95a5a65
37231fe
ee7c337
e8705fb
6b94344
cc11f28
44e76f1
08d8cc5
a8be41f
46de7a1
6631cd4
1078be8
53fc395
5f238b5
1a5dbed
1f7c44e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,54 @@ | ||
| import Foundation | ||
|
|
||
| class DockerConfigCredentialsProvider: CredentialsProvider { | ||
|
|
||
| func retrieve(host: String) throws -> (String, String)? { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like this is too much indirection for such a simple function. We now have four new functions containing only 5 lines of code on average, and they're only being called from a single place, thus offering no code re-use that functions/methods normally provide. I think we could get away with just a single new function: private func dockerConfig() throws -> Data? { ... }That will either read from You can then call it from the |
||
| let dockerConfigURL = FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent(".docker").appendingPathComponent("config.json") | ||
| if !FileManager.default.fileExists(atPath: dockerConfigURL.path) { | ||
| guard let configFileURL = try configFileURL else { | ||
| return nil | ||
| } | ||
| let config = try JSONDecoder().decode(DockerConfig.self, from: Data(contentsOf: dockerConfigURL)) | ||
|
|
||
| if let credentialsFromAuth = config.auths?[host]?.decodeCredentials() { | ||
| return credentialsFromAuth | ||
| let config = try JSONDecoder().decode(DockerConfig.self, from: Data(contentsOf: configFileURL)) | ||
| return try retrieveCredentials(for: host, from: config) | ||
| } | ||
|
|
||
| // MARK: - Private | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nicer if we could avoid these marks, we don't use them anywhere else in the codebase. |
||
| private var configFileURLFromEnvironment: URL? { | ||
| get throws { | ||
| guard let configPathFromEnvironment = ProcessInfo.processInfo.environment["TART_DOCKER_CONFIG"] else { | ||
| return nil | ||
| } | ||
|
|
||
| let url = URL(filePath: configPathFromEnvironment) | ||
|
|
||
| guard FileManager.default.fileExists(atPath: configPathFromEnvironment) else { | ||
| throw NSError.fileNotFoundError(url: url, message: "Registry authentication failed. Could not find docker configuration at '\(configPathFromEnvironment)'.") | ||
| } | ||
|
|
||
| return url | ||
| } | ||
| } | ||
|
|
||
| private var dockerConfigFileURL: URL? { | ||
| let url = FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent(".docker").appendingPathComponent("config.json") | ||
|
|
||
| guard FileManager.default.fileExists(atPath: url.path) else { | ||
| return nil | ||
| } | ||
|
|
||
| return url | ||
| } | ||
|
|
||
| private var configFileURL: URL? { | ||
| get throws { | ||
| return try configFileURLFromEnvironment ?? dockerConfigFileURL | ||
| } | ||
| } | ||
|
|
||
| private func retrieveCredentials(for host: String, from config: DockerConfig) throws -> (String, String)? { | ||
| if let credentials = config.auths?[host]?.decodeCredentials() { | ||
| return credentials | ||
| } | ||
|
|
||
| if let helperProgram = try config.findCredHelper(host: host) { | ||
| return try executeHelper(binaryName: "docker-credential-\(helperProgram)", host: host) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,16 @@ extension Collection { | |
| } | ||
| } | ||
|
|
||
| extension NSError { | ||
| static func fileNotFoundError(url: URL, message: String = "") -> NSError { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just throw a |
||
| return NSError( | ||
| domain: NSCocoaErrorDomain, | ||
| code: NSFileReadNoSuchFileError, | ||
| userInfo: [NSURLErrorKey: url, NSFilePathErrorKey: url.path, NSLocalizedFailureErrorKey: message] | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| func resolveBinaryPath(_ name: String) -> URL? { | ||
| guard let path = ProcessInfo.processInfo.environment["PATH"] else { | ||
| return nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,3 +14,18 @@ def tart(): | |
| def docker_registry(): | ||
| with DockerRegistry() as docker_registry: | ||
| yield docker_registry | ||
|
|
||
| @pytest.fixture(scope="function") | ||
| def docker_registry_authenticated(request): | ||
| """ | ||
| Provides an authenticated Docker registry where username/password | ||
| can be passed dynamically via the test case. | ||
|
|
||
| Usage: | ||
| - Add `docker_registry_authenticated` as a test argument. | ||
| - Pass `request.param = (username, password)` from the test. | ||
| """ | ||
| credentials = request.param if hasattr(request, "param") else ("testuser", "testpassword") | ||
|
|
||
| with DockerRegistry(credentials=credentials) as docker_registry: | ||
| yield docker_registry | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing newline at the end of the file. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import base64 | ||
| import json | ||
| import os | ||
| import tempfile | ||
| import timeit | ||
|
|
@@ -28,6 +30,72 @@ def test_pull_speed(self, tart, vm_with_random_disk, docker_registry): | |
|
|
||
| actual_speed_per_second = self._calculate_speed_per_second(amount_to_transfer, stop - start) | ||
| assert actual_speed_per_second > minimal_speed_per_second | ||
|
|
||
| @pytest.mark.dependency() | ||
| @pytest.mark.parametrize("docker_registry_authenticated", [("user1", "pass1")], indirect=True) | ||
| def test_authenticated_push_from_env_config(self, tart, vm_with_random_disk, docker_registry_authenticated): | ||
| with tempfile.NamedTemporaryFile(delete=False) as tf: | ||
| tf.write(_docker_credentials_store(docker_registry_authenticated.remote_host(), "user1", "pass1")) | ||
| tf.close() | ||
| tart.run(["push", "--insecure", vm_with_random_disk, docker_registry_authenticated.remote_name(vm_with_random_disk)], env = { "TART_DOCKER_CONFIG": tf.name }) | ||
|
|
||
| @pytest.mark.dependency() | ||
| @pytest.mark.parametrize("docker_registry_authenticated", [("user1", "pass1")], indirect=True) | ||
| def test_authenticated_push_from_docker_config(self, tart, vm_with_random_disk, docker_registry_authenticated): | ||
| with tempfile.NamedTemporaryFile(delete=False) as tf: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use a temporary file when you can write to the destination directly? with open(os.path.expanduser("~/.docker/config.json"), "w") as f:
... |
||
| tf.write(_docker_credentials_store(docker_registry_authenticated.remote_host(), "user1", "pass1")) | ||
| tf.close() | ||
| if not os.path.exists(os.path.expanduser("~/.docker")): | ||
| os.mkdir(os.path.expanduser("~/.docker")) | ||
| os.rename(tf.name, os.path.expanduser("~/.docker/config.json")) | ||
|
|
||
| tart.run(["push", "--insecure", vm_with_random_disk, docker_registry_authenticated.remote_name(vm_with_random_disk)]) | ||
|
|
||
| @pytest.mark.dependency() | ||
| @pytest.mark.parametrize("docker_registry_authenticated", [("user1", "pass1")], indirect=True) | ||
| def test_authenticated_push_env_path_precedence(self, tart, vm_with_random_disk, docker_registry_authenticated): | ||
| with tempfile.NamedTemporaryFile(delete=False) as tf: | ||
| tf.write(_docker_credentials_store(docker_registry_authenticated.remote_host(), "user1", "pass1")) | ||
| tf.close() | ||
|
|
||
| with tempfile.NamedTemporaryFile(delete=False) as tf2: | ||
| tf2.write(_docker_credentials_store(docker_registry_authenticated.remote_host(), "notuser", "notpassword")) | ||
| tf2.close() | ||
| if not os.path.exists(os.path.expanduser("~/.docker")): | ||
| os.mkdir(os.path.expanduser("~/.docker")) | ||
| os.rename(tf2.name, os.path.expanduser("~/.docker/config.json")) | ||
|
|
||
| tart.run(["push", "--insecure", vm_with_random_disk, docker_registry_authenticated.remote_name(vm_with_random_disk)], env = { "TART_DOCKER_CONFIG": tf.name }) | ||
|
|
||
| @pytest.mark.dependency() | ||
| @pytest.mark.parametrize("docker_registry_authenticated", [("user1", "pass1")], indirect=True) | ||
| def test_authenticated_push_env_credentials_precedence(self, tart, vm_with_random_disk, docker_registry_authenticated): | ||
| with tempfile.NamedTemporaryFile(delete=False) as tf: | ||
| tf.write(_docker_credentials_store(docker_registry_authenticated.remote_host(), "notuser", "notpassword")) | ||
| tf.close() | ||
|
|
||
| env = { | ||
| "TART_REGISTRY_USERNAME": "user1", | ||
| "TART_REGISTRY_PASSWORD": "pass1", | ||
| "TART_DOCKER_CONFIG": tf.name | ||
| } | ||
| tart.run(["push", "--insecure", vm_with_random_disk, docker_registry_authenticated.remote_name(vm_with_random_disk)], env) | ||
|
|
||
| @pytest.mark.dependency() | ||
| @pytest.mark.parametrize("docker_registry_authenticated", [("user1", "pass1")], indirect=True) | ||
| def test_authenticated_push_invalid_env_path_error(self, tart, vm_with_random_disk, docker_registry_authenticated): | ||
| env = { "TART_DOCKER_CONFIG": "/temp/this-file-does-not-exist" } | ||
|
|
||
| _, stderr, returncode = tart.run( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can just declare a new method: The |
||
| ["push", "--insecure", vm_with_random_disk, docker_registry_authenticated.remote_name(vm_with_random_disk)], | ||
| env, | ||
| raise_on_nonzero_returncode=False | ||
| ) | ||
|
|
||
| expected_error = f"Registry authentication failed. Could not find docker configuration at '/temp/this-file-does-not-exist'." | ||
|
|
||
| assert returncode == 1, f"Tart should fail with exit code 1 but failed with {returncode}." | ||
| assert expected_error in stderr, f"Expected error '{expected_error}' not found in stderr: {stderr}" | ||
|
|
||
| @staticmethod | ||
| def _calculate_speed_per_second(amount_transferred, time_taken): | ||
|
|
@@ -53,3 +121,20 @@ def vm_with_random_disk(tart): | |
| yield vm_name | ||
|
|
||
| tart.run(["delete", vm_name]) | ||
|
|
||
| def _docker_credentials_store(host, user, password): | ||
| # Encode "username:password" in Base64 | ||
| auth_string = f"{user}:{password}" | ||
| auth_b64 = base64.b64encode(auth_string.encode()).decode() | ||
|
|
||
| # Create JSON structure | ||
| docker_auth = { | ||
| "auths": { | ||
| host: { | ||
| "auth": auth_b64 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| # Convert dictionary to JSON | ||
| return json.dumps(docker_auth).encode() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an extraneous space compared to the rest of the codebase.