Skip to content

Conversation

@dameng324
Copy link
Contributor

@dameng324 dameng324 commented Mar 28, 2024

As discussed in dotnet/sdk-container-builds#558 (reply in thread)

When add http/https prefix, It will check the local docker config, if found it is an insecure registery, add http prefix instead of https prefix as before.

By the way, this pr fix another bug in DockerCli.GetDockerConfig. If docker daemon is not running, It will throw a DockerLoadException instead of return a invaild result.

I test in my local machine.

daemon.json:

{
  "builder": {
    "gc": {
      "defaultKeepStorage": "20GB",
      "enabled": true
    }
  },
  "experimental": false,
  "insecure-registries": [
    "dockerhub.shengguanda.com"
  ]
}

test code:

public static void TestTryExpandRegistryToUri()
{
    var uri = ContainerHelpers.TryExpandRegistryToUri("localhost:5000");
    Console.WriteLine(uri);
    var uri2 = ContainerHelpers.TryExpandRegistryToUri("dockerhub.shengguanda.com");
    Console.WriteLine(uri2);
    var uri3 = ContainerHelpers.TryExpandRegistryToUri("dockerhub.shengguanda.com:5000");
    Console.WriteLine(uri3);
    var uri4 = ContainerHelpers.TryExpandRegistryToUri("dockerhub.com");
    Console.WriteLine(uri4);
}

result:

http://localhost:5000/
http://dockerhub.shengguanda.com/
https://dockerhub.shengguanda.com:5000/
https://dockerhub.com/

@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Mar 28, 2024
@dameng324
Copy link
Contributor Author

@dotnet-policy-service agree

@baronfel
Copy link
Member

baronfel commented Mar 29, 2024

Separately, this change made a bunch of other tests fail because some of the test systems don't have Docker available - I think if we can't launch Docker we should default to the previous behavior here - the registry should be inferred to be secure-by-default.

@baronfel
Copy link
Member

This is also relevant to the issue request dotnet/sdk-container-builds#338.

@danmoseley
Copy link
Member

Just curious, would rid potentially lead to unexpectedly getting an image from an insecure registry due to it for some reason not being in the secure registry I expected (or that registry not responding)

@baronfel
Copy link
Member

baronfel commented Mar 29, 2024

That's possible, especially if you didn't fully qualify the image name - e.g. dotnet/aspnet vs mcr.microsoft.com/dotnet/aspnet. In that case, the search algorithm of your engine would engage and could potentially expand the truncated image name into another fully-qualified image name. Podman, for example, has some docs on this process:

If the image reference in the command line argument does not contain a registry, it is referred to as ashort-name reference. If the image is a ‘short-name’ reference, Podman prompts the user for the specific container registry to pull the image from, if an alias for the short-name has not been specified in the short-name-aliases.conf.

The SDK, however, currently only expands 'partial' image names into Docker Hub references, so we aren't directly vulnerable to hijacking in that way.

@dameng324
Copy link
Contributor Author

Thanks for your review.

  1. I will change to previous behavior of DockerCli.GetDockerConfig.
  2. Add Podman support.
  3. run test.cmd and see is there any error appear.

@dameng324
Copy link
Contributor Author

The work is done. All test passed. please check again @baronfel

@baronfel baronfel added Area-Containers Related to dotnet SDK containers functionality and removed Area-Infrastructure labels Apr 9, 2024
@baronfel baronfel requested a review from a team April 9, 2024 20:52
Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

I have a question about the conditional, and I'd like another dev on the containers team to check.

@dameng324
Copy link
Contributor Author

Ok,Is there anything else you need me to do?

@baronfel baronfel requested a review from a team April 13, 2024 17:08
@baronfel
Copy link
Member

@dameng324 nope, I think this is good to go! Thank you for your work on this feature. I'm going to approve it, but I've also tagged the other SDK Containers feature maintainers so they can give it a look.

@baronfel
Copy link
Member

/backport to release/8.0.4xx

@github-actions
Copy link
Contributor

Started backporting to release/8.0.4xx: https://github.com/dotnet/sdk/actions/runs/9098104332

@baronfel
Copy link
Member

Thank you for this excellent contribution @dameng324 - I've started a backport to 8.0.400 so that this will release in August's SDK release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants