-
-
Notifications
You must be signed in to change notification settings - Fork 622
fixed Private key and certificates on repo #3666
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
fixed Private key and certificates on repo #3666
Conversation
c718b27 to
be002a5
Compare
Signed-off-by: guptapratykshh <[email protected]>
be002a5 to
00b9144
Compare
|
@mpilquist can you have a look at this PR , Thanks. |
| @@ -0,0 +1,94 @@ | |||
| /* | |||
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.
Note this file is JVM specific but name still has Js in it.
| */ | ||
| def createProvider: IO[TestCertificateProvider] = | ||
| // This will be implemented differently for each platform | ||
| PlatformSpecificCertificateProvider.create |
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.
If the implementations all shell out to openssl, why do they need to be platform specific? If instead we used, e.g., JCA on the JVM, node-forge on JS, and libopenssl on native, then I could see doing the platform independent approach.
| /** Cached certificate provider to avoid regenerating for each test | ||
| */ | ||
| private lazy val cachedProvider: IO[TestCertificateProvider] = | ||
| Ref.of[IO, Option[TestCertificateProvider]](None).flatMap { ref => |
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 will create a new Ref for each use of cachedProvider, defeating the attempt at caching.
| } | ||
|
|
||
| test("mTLS client verification fails if client cannot authenticate") { | ||
| test("mTLS client verification fails if client cannot authenticate".ignore) { |
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.
Leftover from debugging?
|
|
||
| def getCertificatePair: IO[TestCertificateProvider.CertificatePair] = | ||
| Files[IO].tempDirectory.use { tempDir => | ||
| val caKey = tempDir / "ca_key.pem" |
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.
No need to create a CA key/certificate. The generated certificate can be self signed. That will simplify this whole function. The self-signed cert + private key should be used as a client/server identity and the self-signed cert should be used in trust stores.
|
|
||
| /** Represents a certificate-key pair for testing | ||
| */ | ||
| case class CertificatePair( |
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.
Only need to return a single self-signed certificate and private key. Should also document the format that's returned (e.g. PEM, etc)
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.
I'd also rename this to be CertificateAndPrivateKey
|
|
||
| /** Creates a provider using OpenSSL (available on all supported platforms). | ||
| */ | ||
| def createProvider: IO[TestCertificateProvider] = |
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.
I'd remove this provider concept and just do this:
object TestCertificateProvider {
private val cachedValue: AtomicCell[IO, Option[CertificateAndPrivateKey]] = AtomicCell.of(None).unsafeRunSync()
def getCertificateAndPrivateKey: IO[CertificateAndPrivateKey] = {
cachedValue.evalUpdate {
case Some(cached) => IO.pure(cached)
case None => generateCertificateAndPrivateKey
}
}
private def generateCertificateAndPrivateKey: IO[CertificateAndPrivateKey] = ???
}82b6cb1 to
20a2ca7
Compare
20a2ca7 to
d7ae073
Compare
| privateKeyString: String | ||
| ) | ||
|
|
||
| private val cachedValue: AtomicReference[Option[CertificateAndPrivateKey]] = |
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.
See my other comment about using cats.effect.std.AtomicCell which simplifies this caching logic.
5052da6 to
fa569f9
Compare
|
i have tested locally all these tests are passing , but cli si failing |
|
Looks like the |
|
@guptapratykshh Take a look at the 2 commits from today for some further simplifications. |
Fixes #3611
Removed the static key.pem and cert.pem files from io/shared/src/test/resources to verify we are no longer relying on committed private keys , implemented ephemeral certificate generation for Scala Native using the openssl CLI. This generates a proper Root CA and signs a Server Certificate with strict RFC 5280 extensions (SKI, AKI, KeyUsage) to satisfy s2n-tls validation requirements and implemented ephemeral certificate generation for Scala.js using openssl (via fs2.io.process), ensuring we can run TLS tests on Node.js without the deleted static files.