Add support for docker-credential-stores through environment

[nholloh]

Motivation

Larger organizations running tart often also run their own OCI registry, where VM images are being managed. Different departments within the organization may have limited access to each others images. That is, they may require different sets of credentials to authenticate with a given OCI registry.

Currently, tart only supports a docker-credential-store through a file in ~/.docker/config.json within the host system. This is not always feasible, as 2 concurrently issued builds in the CI may overwrite each others config.json leading to race conditions during OCI authentication.

Alternatively, all authentication with OCI registries can be overridden by setting the TART_REGISTRY_USERNAME and TART_REGISTRY_PASSWORD variables. However this comes with its own set of issues, as the credentials are not associated with any host. Should a user decide to opt for a publically available image (e.g. macos-image-templates) over the internal ones, the default behaviour would have tart send internal credentials to a public OCI registry.

Current workaround

The current workaround is to execute a script before tart attempts to fetch the image from the OCI registry, where we manually parse a docker-credential-store json string from the environment, select the appropriate credential and set it in tart's TART_REGISTRY_USERNAME/TART_REGISTRY_PASSWORD environment variables. While this works, it is an unnecessary burden to place on larger organizations, especially if there are existing approaches for similar cases.

Proposed solution

Similar to the DOCKER_CONFIG environment variable used by Docker, we could allow for setting a docker-credential-store filepath as environment variable TART_DOCKER_CONFIG. I chose to prefix the environment variable with TART_ to remain consistent with existing variables and to avoid conflicts with existing docker setups. tart would then evaluate the contents of the file in the same way as it currently does the ~/.docker/config.json. Since the environment variable is specific to the instance of the job being run on the machine and bound to the matching process, concurrently run image fetch operations would not run into the race condition detailed in the motivation above.

Detailed design

The changes mainly revolve around the retrieve(host:) function in the DockerConfigCredentialsProvider class. The external interface as CredentialProvider remains intact, but the function internally now additionally evaluates the environment. If no configuration is available, nil is returned.

func retrieve(host: String) throws -> (String, String)? {
    guard let configFileURL = try configFileURL else {
      return nil
    }

    let config = try JSONDecoder().decode(DockerConfig.self, from: Data(contentsOf: configFileURL))
    return try retrieveCredentials(for: host, from: config)
}

The config path from the environment takes precedence over the config from default location in ~/.docker/config.json. That is, if a config is configured through the environment and there is a config in the default location, the values from the environment config will be used. The default location config will not be considered.

  private var configFileURL: URL? {
    get throws {
      return try configFileURLFromEnvironment ?? dockerConfigFileURL
    }
  }

The config file in the default location is optional and does not produce an error if it is not present. However, if a config file was configured through the environment it is expected to be present and will produce an error if not found.

  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
    }
  }

Tests

As discussed below, I added integration tests to validate the changes. To do so, the docker_registry.py was extended so that it can spawn an instance of the registry container which requires configurable authentication. tart.py now allows to configure environment variables to be set for the execution of tart, enabling the injection of credentials for authentication.

The following tests have been added:

• test_authenticated_push_from_env_config
• test_authenticated_push_from_docker_config
• test_authenticated_push_env_path_precedence
• test_authenticated_push_env_credentials_precedence
• test_authenticated_push_invalid_env_path_error

Other changes

I fixed a typo in one of the test file names.

[CLAassistant]

All committers have signed the CLA.

[edigaryev]

Hi Niklas 👋

Thanks for your contribution and for taking the time to provide such a details explanation!

I appreciate the effort, but I feel the approach might be a bit more complex than necessary to achieve your goal.

Please take a look at the following alternative:

diff --git a/Sources/tart/Credentials/DockerConfigCredentialsProvider.swift b/Sources/tart/Credentials/DockerConfigCredentialsProvider.swift
index 6be901f..a50de8e 100644
--- a/Sources/tart/Credentials/DockerConfigCredentialsProvider.swift
+++ b/Sources/tart/Credentials/DockerConfigCredentialsProvider.swift
@@ -2,7 +2,11 @@ import Foundation
 
 class DockerConfigCredentialsProvider: CredentialsProvider {
   func retrieve(host: String) throws -> (String, String)? {
-    let dockerConfigURL = FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent(".docker").appendingPathComponent("config.json")
+    let dockerConfigURL = if let tartDockerConfig = ProcessInfo.processInfo.environment["TART_DOCKER_CONFIG"] {
+      URL(fileURLWithPath: tartDockerConfig)
+    } else {
+      FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent(".docker").appendingPathComponent("config.json")
+    }
     if !FileManager.default.fileExists(atPath: dockerConfigURL.path) {
       return nil
     }

The test(s) can be added to integration-tests/, there's no need to mock anything.

[nholloh]

The suggestion you provided works, but has some functional key differences. In my design, I chose to allow the user to have both, a config from the file system and a config in the environment. While environment takes precedence, if a host is not present in the environment it can still be loaded from the config in the file system. Furthermore, if one of the configs is invalid, the other one will still be evaluated. You can check the unit tests for the details on this.

I also verified my understanding of your approach with the tests I have implemented, but couple of tests now fail.

At the end of the day this is a design choice and we can go either way. Regarding the tests, would you like me to remove the unit tests and add integration tests instead or are you fine with keeping the unit tests (and adding integration)?

[edigaryev]

At the end of the day this is a design choice and we can go either way.

I'm all in for a simpler approach.

Also, falling back to a standard ~/.docker/config.json location when TART_DOCKER_CONFIG is explicitly set could be considered magical by some.

Regarding the tests, would you like me to remove the unit tests and add integration tests instead or are you fine with keeping the unit tests (and adding integration)?

I think we're better off with integration tests since there's no need to introduce unnecessary abstractions to the CredentialsProvider implementation this way.

Note that you can also have integration tests in Swift (see LayerizerTests for an example).

/cc @fkorotkov

[nholloh]

Checking the integration tests I am not sure I understand the approach entirely. How would you

1. pass in the environment variable?
2. make sure that tart authenticates with the right set of credentials against a registry (or the registry runner)?

[edigaryev]

1. pass in the environment variable?

For that you may need to extend the Tart.run()'s signature slightly to accept an additional env argument and append it accordingly here:

tart/integration-tests/tart.py

Lines 24 to 32 in f682970

	def run(self, args):
	env = os.environ.copy()
	env.update({"TART_HOME": self.tmp_dir.name})
	completed_process = subprocess.run(["tart"] + args, env=env, capture_output=True)
	completed_process.check_returncode()
	return completed_process.stdout.decode("utf-8"), completed_process.stderr.decode("utf-8")

2. make sure that tart authenticates with the right set of credentials against a registry (or the registry runner)?

This can be achieved by simply making sure that tart pull and/or tart push succeeds against an authenticated registry instance.

We currently use registry:2, to make it authenticated for a given test, you might want to configure native basic auth, which again can be made optional in the DockerRegistry initializer, so that only specific tests will get the authenticated registry instance:

tart/integration-tests/docker_registry.py

Lines 10 to 12 in 0b693f6

	def __init__(self):
	super().__init__("registry:2")
	self.with_exposed_ports(self._default_exposed_port)

[fkorotkov]

+1 for @edigaryev's:

Also, falling back to a standard ~/.docker/config.json location when TART_DOCKER_CONFIG is explicitly set could be considered magical by some.

IMO it's better to fail fast then fallback.

I do like @nholloh's TART_DOCKER_AUTH_CONFIG name more though. Just TART_DOCKER_CONFIG seems not clear enough, Tart only uses this config for authentication.

[nholloh]

@edigaryev thanks for your explanation and input. I'm working on the changes and there may be multiple pushes in the process to get the integration tests right. I'll post an update here once I am done.

[edigaryev]

I do like @nholloh's TART_DOCKER_AUTH_CONFIG name more though. Just TART_DOCKER_CONFIG seems not clear enough, Tart only uses this config for authentication.

The AUTH part here is a bit misleading because ~/.docker/config.json is a general configuration file and it is not auth-specific.

Another point is that DOCKER_CONFIG is an well-known environment variable used by Docker.

[nholloh]

The issue I see with this naming is that ~/.docker/config.json is a general configuration file and it is not auth-specific.

The other point is that DOCKER_CONFIG is an well-known environment variable used by Docker.

Since this change is a proposal in the context of CI, specifically due to the issues we're facing on GitLab, I see value in following GitLab's naming scheme.

Additionally, the DOCKER_CONFIG variable you reference is described as:

The location of your client configuration files.

While I see your point that the configuration may contain more information than just authentication, the new environment variable I am proposing is carrying the file's contents rather than a path to the file. Using the same naming scheme for entirely different content could lead to confusion around its behaviour. Providing a file path instead of the content would not resolve the issue mentioned above without further workarounds. Therefore, I still think explicitly choosing a different name makes sense.

Alternatively, how about TART_DOCKER_CONFIG_JSON?

[edigaryev]

Since this change is a proposal in the context of CI, specifically due to the issues we're facing on GitLab, I see value in following GitLab's naming scheme.

This seems like a rather stretched assumption because Tart is used in various environments, not just GitLab, and there our users may find this environment variable useful as well.

I am proposing is carrying the file's contents rather than a path to the file

Providing a file path instead of the content would not resolve the issue mentioned above without further workarounds.

Just to clarify, is there a reason this approach doesn't this work for you?

script:
  - echo "$DOCKER_CONFIG_BASE64" | base64 -d > ./config.json
  - export TART_DOCKER_CONFIG=./config.json
  - tart ...

It's used in GitLab for KUBECONFIG, GOOGLE_APPLICATION_CREDENTIALS and other environment variables that contain a path to a file.

The file will be created in the project's directory and shouldn't cause any conflicts with other jobs.

[nholloh]

Coming from iOS development, I don't really know my way around the different tools used in infra and hosting. That is, if it really is GitLab who deviate from the convention to pass file paths into a tool as environment variable for the OCI registry authentication, then I am completely with you. In this case we should absolutely pass the file path as env var.

To make things align with GitLab's way of providing credentials, maybe we could add the feature to the prepare command of the gitlab-tart-executor. It could then take a TART_DOCKER_AUTH_CONFIG (config file content), writes it to a temporary file and passes the path to that file as TART_DOCKER_CONFIG to tart? This would give GitLab users a familiar tool to work with while abstracting away the added complexity of writing the content to a temporary file.

[edigaryev]

@nholloh that sounds like good trade-off to me, especially since we'd still need to change the GitLab Tart Executor to support this because GitLab Runner prepends CUSTOM_ENV_ to CI/CD environment variables.

See for example how we do this for TART_REGISTRY_{USERNAME,PASSWORD}: https://github.com/cirruslabs/gitlab-tart-executor/blob/e03add8340a51aa1e51d9e5269b42951a1a3e713/internal/commands/prepare/prepare.go#L279-L307.

However, I still think that a more appropriate name for the environment variable should be a suffix to TART_DOCKER_CONFIG to make it more discoverable. For example, TART_DOCKER_CONFIG_CONTENTS, TART_DOCKER_CONFIG_JSON or TART_DOCKER_CONFIG_BASE64.

[nholloh]

@edigaryev sounds good to me. I'm currently working on the changes and noticed one more detail:

In my opinion the behaviour for File not found should differ between the default docker location and a file I explicitly request to be used through the TART_DOCKER_CONFIG variable. For the default location, it should remain as is, a silent failure to retrieve credentials so that tart continues to check the Keychain for credentials. If, however, the TART_DOCKER_CONFIG env var was set explicitly, I would expect tart to fail with an error and tell me the file was not found.

Whats your opinion on this?

[edigaryev]

If, however, the TART_DOCKER_CONFIG env var was set explicitly, I would expect tart to fail with an error and tell me the file was not found.

Sounds reasonable to me, as long as the user will get a clear error message explaining the what's wrong.

[nholloh]

Done, the changes are implemented. Feel free to take a look. The error message now reads:

Error: Registry authentication failed. Could not find docker configuration at '/tmp/abc'. The file doesn’t exist.

It uses the standard apple keys and error codes for the FileNotFound error. Also, I updated the initial description of the pull request to match the current state of implementation after our discussion.

[Fred78290]

Just a remark!
Don't think that docker registry are only authentified with user/password. So it means that often docker-credential-helpers is needed and .docker/config.json is not enough.

As example the authent to AWS ECR, using AWS secret/access key or ephemeral token. Azure too.

As ref: https://aws.amazon.com/fr/blogs/compute/authenticating-amazon-ecr-repositories-for-docker-cli-with-credential-helper/

[nholloh]

Just a remark!

Don't think that docker registry are only authentified with user/password. So it means that often docker-credential-helpers is needed and .docker/config.json is not enough.

As example the authent to AWS ECR, using AWS secret/access key or ephemeral token. Azure too.

As ref: https://aws.amazon.com/fr/blogs/compute/authenticating-amazon-ecr-repositories-for-docker-cli-with-credential-helper/

Would this not be covered through the existing implementation?

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)
    }

    return nil
  }

[Fred78290]

Would this not be covered through the existing implementation?

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)
    }

    return nil
  }

At minima with your implementation, on aws or azure you must create a temporary token to authent on private registry in your script such aws aws ecr-login....

But it not enough if you want integrate docker credentials helper mechanism with existing plugin.

Take a look at:

https://github.com/docker/docker-credential-helpers
https://github.com/awslabs/amazon-ecr-credential-helper

[edigaryev]

Please, try make the smallest amount of changes necessary to get your functionality across.

This will save both your (writing the code) and our time (reviewing it).

[edigaryev]

You can just throw a CredentialsProviderError.Failed(...) instead of declaring a new error here.

[edigaryev]

It would be nicer if we could avoid these marks, we don't use them anywhere else in the codebase.

[edigaryev]

This looks like an extraneous space compared to the rest of the codebase.

[edigaryev]

I think you can just declare a new method: run_with_returncode() that defaults to raise_on_nonzero_returncode=False to avoid the code churn for the rest of the run() calls.

The run() itself will call the new run_with_returncode() under the hood, passing all of its arguments to it and setting raise_on_nonzero_returncode=True.

[edigaryev]

Why use a temporary file when you can write to the destination directly?

with open(os.path.expanduser("~/.docker/config.json"), "w") as f:
    ...

[edigaryev]

Missing newline at the end of the file.

[edigaryev]

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 TART_DOCKER_CONFIG, throw an exception, read from the default location or return nil.

You can then call it from the retrieve() accordingly by introducing a minimal amount of changes.