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: Add support for RSA private key (RsaPrivateCrtKeyParameters) TLS authentication with protected Docker daemon sockets #978

Conversation

zuntio
Copy link
Contributor

@zuntio zuntio commented Aug 18, 2023

What does this PR do?

Adds switch case to determine if read object is actually AsymmetricCipherKeyPair or RsaPrivateCrtKeyParameters instead of hard casting it to AsymmetricCipherKeyPair.

Why is it important?

Tutorial of protecting socket results in pem key type which resolves as RsaPrivateCrtKeyParameters object type. Same outputs come from many other tutorials too so it is reasonable to support it.

Related issues

How to test this PR

Configure TLS protection to docker daemon according to tutorial and write simple hello-world usage.

@netlify
Copy link

netlify bot commented Aug 18, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 9491afd
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/64ecc5db985a67000851a8b6
😎 Deploy Preview https://deploy-preview-978--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

Thanks for the pull request 🙏. Do you think we can cover the changes with a test?

@zuntio
Copy link
Contributor Author

zuntio commented Aug 19, 2023

Thanks for the pull request 🙏. Do you think we can cover the changes with a test?

Seems that if I replace DockerVersion with newest 24.0.5, object is resolved as RsaPrivateCrtKeyParameters.

If you give me a green light I'll do some refactoring to ProtectDaemonSocket fixture to make version overrideable and add duplicate test with other version including some verification that different type of object was actually resolved?

Edit: verifying type seems a bit nasty since contents of X509Certificate2 are pretty similar between runs with different versions. Only length of raw data byte array differs and not propably goot property to match :)

@HofmeisterAn
Copy link
Collaborator

Seems that if I replace DockerVersion with newest 24.0.5, object is resolved as RsaPrivateCrtKeyParameters.

Ah, that is fortunate.

If you give me a green light I'll do some refactoring to ProtectDaemonSocket fixture to make version overrideable and add duplicate test with other version including some verification that different type of object was actually resolved?

Of course, certainly 👍. Couldn't we just verify if the generated key (inside _hostCertsDirectoryPath) matches the type we expect? This won't validate our internal resolution, but I think it's fine as long as we confirm that both types are working and not throwing an exception.

@zuntio
Copy link
Contributor Author

zuntio commented Aug 19, 2023

Couldn't we just verify if the generated key (inside _hostCertsDirectoryPath) matches the type we expect? This won't validate our internal resolution, but I think it's fine as long as we confirm that both types are working and not throwing an exception.

Sounds good. I'll write and commit it tomorrow along with fixture refac.

@HofmeisterAn
Copy link
Collaborator

Great, take your time. There's no rush at all. Enjoy the rest of the weekend 🥳.

@zuntio
Copy link
Contributor Author

zuntio commented Aug 21, 2023

Here you go. Lot of patterns to go with but decided to use IClassFixture pattern. Naming is bit off after realising this is all from IETF TLS deprecation and therefore OpenSSL has been updated at some point. Of course open for suggestions.

@zuntio zuntio force-pushed the feature/support-rsaprivatecrtkeyparameters-from-pem branch from 468f516 to c0a568e Compare August 21, 2023 19:21
@HofmeisterAn
Copy link
Collaborator

Thank you for making the changes. For your information: I will be able to do the review earliest by the end of this week (possibly at the beginning of next week).

@HofmeisterAn HofmeisterAn changed the title feat: support RsaPrivateCrtKeyParameters being resolved from key pem feat: Add support for RSA private key (RsaPrivateCrtKeyParameters) TLS authentication with protected Docker daemon sockets Aug 28, 2023
@HofmeisterAn HofmeisterAn merged commit b121dde into testcontainers:develop Aug 28, 2023
@HofmeisterAn
Copy link
Collaborator

Thanks again. PR looks good.

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Aug 28, 2023
@safalin1
Copy link

Just tested it, working great now. Thanks guys!

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.

[Bug]: InvalidCastException when running with docker with TLS
3 participants