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

feat(#370) Added mTLS support to docker endpoint #548

Conversation

vlaskal
Copy link
Contributor

@vlaskal vlaskal commented Aug 2, 2022

This PR add two options for docker endpoint authentication.

  1. it is support of simple secured endpoint by TLS
  2. it is support of client authentication via mTLS

PR contains 2 commits on for each option.

This PR is draft only to review approach of implementation.

@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 4b618c7 to 1f4209f Compare August 2, 2022 15:40
@vlaskal
Copy link
Contributor Author

vlaskal commented Aug 2, 2022

I am not sure why few tests failed when locally run fine. I think that my code is out of test scope.

What this PR miss are:

  • Test strategy for new TlsEndpointAuthenticationProvider and MTlsEndpointAuthenticationProvider
  • Update build agents to be able to test these providers
  • Cleanup based on last PRs
  • Review of supportability in .NET Standard 2.1 (Do we test both .NET Standards?)
  • Used Custom configurations

@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch 3 times, most recently from 339a9c0 to 822cd9a Compare August 7, 2022 10:03
@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 822cd9a to 1fb07a3 Compare August 12, 2022 22:29
@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 1fb07a3 to 5ea3404 Compare August 20, 2022 18:13
@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 5ea3404 to e29a896 Compare August 27, 2022 19:22
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

I would like to split this pull request into two smaller chunks. The first one should just contain the changes regarding ICustomConfiguration (reading the custom configuration values incl. tests). The second one the TLS implementation. It makes the review easier and we can focus just on the TLS part next. What do you think? In the meantime I'll setup the TLS test environment. Thanks again.

src/Testcontainers/Configurations/CustomConfiguration.cs Outdated Show resolved Hide resolved
src/Testcontainers/Configurations/CustomConfiguration.cs Outdated Show resolved Hide resolved
src/Testcontainers/Configurations/CustomConfiguration.cs Outdated Show resolved Hide resolved
src/Testcontainers/Configurations/ICustomConfiguration.cs Outdated Show resolved Hide resolved
src/Testcontainers/Configurations/ICustomConfiguration.cs Outdated Show resolved Hide resolved
@vlaskal
Copy link
Contributor Author

vlaskal commented Sep 3, 2022

I would like to split this pull request into two smaller chunks. The first one should just contain the changes regarding ICustomConfiguration (reading the custom configuration values incl. tests). The second one the TLS implementation. It makes the review easier and we can focus just on the TLS part next. What do you think? In the meantime I'll setup the TLS test environment. Thanks again.

Ok, will try to prepare it soon.

@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from e29a896 to a78b3b7 Compare September 3, 2022 20:29
@HofmeisterAn
Copy link
Collaborator

We can use something like this to create a test instance:

public sealed class GitHub : IAsyncLifetime
{
  private const string CertsDirectoryName = "certs";

  private static readonly string ContainerCertDirectoryPath = Path.Combine("/", CertsDirectoryName);

  private static readonly string HostCertDirectoryPath = Path.Combine(Path.GetTempPath(), CertsDirectoryName);

  private readonly ITestcontainersContainer tlsContainer = new TestcontainersBuilder<TestcontainersContainer>()
    .WithImage("docker:20.10-dind")
    .WithPrivileged(true)
    .WithEnvironment("DOCKER_CERT_PATH", ContainerCertDirectoryPath)
    .WithEnvironment("DOCKER_TLS_CERTDIR", ContainerCertDirectoryPath)
    .WithEnvironment("DOCKER_TLS", "1")
    .WithEnvironment("DOCKER_TLS_VERIFY", "1")
    .WithBindMount(HostCertDirectoryPath, ContainerCertDirectoryPath, AccessMode.ReadWrite)
    .Build();

  [Fact]
  public Task PullRequest548()
  {
    return Task.CompletedTask;
  }

  public Task InitializeAsync()
  {
    return this.tlsContainer.StartAsync();
  }

  public Task DisposeAsync()
  {
    return this.tlsContainer.DisposeAsync().AsTask();
  }
}

This generates the certificates too.

@vlaskal
Copy link
Contributor Author

vlaskal commented Sep 6, 2022

Ok will try to use it. But I will update PR to latest master then will split it to two smaller chunks with tests.
Then we can deal with one feature in a time.

@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from a78b3b7 to 0cde71d Compare September 6, 2022 19:03
@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 0cde71d to 751e60e Compare October 9, 2022 13:14
@vlaskal vlaskal changed the title Feature/370 add support tls secured docker endpoint Feature/370 add mTls support to docker endpoint Oct 9, 2022
@vlaskal
Copy link
Contributor Author

vlaskal commented Oct 9, 2022

@HofmeisterAn I updated mTls implementation based on implementation of Tls in PR #597 including inheritance to not repeat code. Uou can look at implementation and chak it. Also here is a test which tests that mTls work.

@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch 3 times, most recently from b787bfc to 1de1d1a Compare October 9, 2022 16:18
@vlaskal vlaskal changed the title Feature/370 add mTls support to docker endpoint Feature/370 add mTLS support to docker endpoint Oct 9, 2022
@vlaskal vlaskal changed the title Feature/370 add mTLS support to docker endpoint feat(#370) Added mTLS support to docker endpoint Oct 9, 2022
@vlaskal vlaskal marked this pull request as ready for review October 10, 2022 11:53
@vlaskal vlaskal requested a review from HofmeisterAn October 10, 2022 12:04
@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from d8af486 to b2db92e Compare October 10, 2022 22:19
@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch 2 times, most recently from 1cf86c9 to 420f8e1 Compare October 11, 2022 07:00
@HofmeisterAn HofmeisterAn force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from c2a4cb0 to 8da5ab3 Compare October 12, 2022 16:16
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

I think the pull request is on the right track. I would like to make sure that the configurations for the fixtures are correct, afterwards we can finalize the providers. Setting the fixtures explicit to tlsverify or tls should be the next step (dunno if that is possible via containers command args). Maybe we need to generate certificates by ourself and check them in to set the configuration explicit.

@vlaskal vlaskal force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 2e11eae to f159775 Compare October 13, 2022 21:17
@vlaskal vlaskal requested a review from HofmeisterAn October 13, 2022 21:17
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

@vlaskal Would you take a look at my recent changes? I think I just need to fix 2 tests, then we are good to merge. What do you think?

@vlaskal
Copy link
Contributor Author

vlaskal commented Oct 17, 2022

@vlaskal Would you take a look at my recent changes? I think I just need to fix 2 tests, then we are good to merge. What do you think?

Done. I think we are close.

Copy link
Contributor Author

@vlaskal vlaskal left a comment

Choose a reason for hiding this comment

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

It looks as final.

@vlaskal vlaskal requested a review from HofmeisterAn October 20, 2022 05:32
@HofmeisterAn HofmeisterAn force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from accecfe to 7fd700f Compare October 20, 2022 06:50
@HofmeisterAn HofmeisterAn force-pushed the feature/370-add-support-tls-secured-docker-endpoint branch from 7fd700f to ed1f5ed Compare October 20, 2022 07:23
@HofmeisterAn HofmeisterAn merged commit 480fc82 into testcontainers:develop Oct 20, 2022
@HofmeisterAn
Copy link
Collaborator

Closes #370.

@HofmeisterAn HofmeisterAn deleted the feature/370-add-support-tls-secured-docker-endpoint branch October 20, 2022 07:38
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.

2 participants