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: Use Bouncy Castle to generate CSRs for AWS private CA. #1905

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

robinsons
Copy link
Contributor

@robinsons robinsons commented Nov 8, 2024

Removes a dependency on openssl for generating CSRs and switches to Bouncy Castle APIs instead. This PR is based on the corresponding changes in #1602.

@wfa-reviewable
Copy link

This change is Reviewable

@robinsons robinsons force-pushed the robinsons-bouncy-castle branch 3 times, most recently from cb28805 to efd12a9 Compare November 11, 2024 22:29
@robinsons robinsons marked this pull request as ready for review November 11, 2024 23:11
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @robinsons and @sandip80)


src/main/kotlin/org/wfanet/panelmatch/common/certificates/aws/CertificateAuthority.kt line 52 at r1 (raw file):

  private val client: CreateCertificateClient,
  private val signatureAlgorithm: SignatureAlgorithm =
    SignatureAlgorithm.SHA_256_WITH_RSA_ENCRYPTION,

nit: can we use ECDSA by default? Most of the web has switched due to smaller key sizes for similar security.


MODULE.bazel line 236 at r1 (raw file):

        # Bouncy Castle
        "org.bouncycastle:bcpkix-jdk15on:1.70",

Use a newer version built for newer JDK. Note that jdk18on here means Java 1.8, i.e. JDK 8.

Suggestion:

jdk18on:1.79

imports/java/org/bouncycastle/bcpkix/BUILD.bazel line 1 at r1 (raw file):

package(default_visibility = ["//visibility:public"])

Imports follow the Java packages included by the target. This defines many subpackages in org.bouncycastle, but not all of them. Therefore the target label should be //imports/java/org/bouncycastle:bcpkix


src/main/kotlin/org/wfanet/panelmatch/common/certificates/GenerateCsr.kt line 29 at r1 (raw file):

 * algorithm.
 */
fun generateCsrFromKeyPair(

New non-extension functions should always be defined in the context of a class or object. e.g. object Certificates or object CertificateSigningRequests. The file and Bazel target name should match this top-level object.

@robinsons robinsons force-pushed the robinsons-bouncy-castle branch 3 times, most recently from adda655 to c76c092 Compare November 19, 2024 23:07
Copy link
Contributor Author

@robinsons robinsons left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 15 files reviewed, 4 unresolved discussions (waiting on @sandip80 and @SanjayVas)


MODULE.bazel line 236 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use a newer version built for newer JDK. Note that jdk18on here means Java 1.8, i.e. JDK 8.

Done.


src/main/kotlin/org/wfanet/panelmatch/common/certificates/aws/CertificateAuthority.kt line 52 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: can we use ECDSA by default? Most of the web has switched due to smaller key sizes for similar security.

Done, but I moved this to a flag since Origin's root cert uses RSA. I did set the default to ECDSA.


imports/java/org/bouncycastle/bcpkix/BUILD.bazel line 1 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Imports follow the Java packages included by the target. This defines many subpackages in org.bouncycastle, but not all of them. Therefore the target label should be //imports/java/org/bouncycastle:bcpkix

Done.


src/main/kotlin/org/wfanet/panelmatch/common/certificates/GenerateCsr.kt line 29 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

New non-extension functions should always be defined in the context of a class or object. e.g. object Certificates or object CertificateSigningRequests. The file and Bazel target name should match this top-level object.

Done.


src/main/kotlin/org/wfanet/panelmatch/client/deploy/example/aws/AwsExampleDaemon.kt line 170 at r3 (raw file):

  }

  private val SignatureAlgorithm.keyAlgorithm: String

@SanjayVas - I put this here for now, but would you care for me to make this an actual property of SignatureAlgorithm? I can do this in a follow-up PR if you think there'd be value.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 12 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @robinsons and @sandip80)


src/main/kotlin/org/wfanet/panelmatch/client/deploy/example/aws/AwsExampleDaemon.kt line 170 at r3 (raw file):

Previously, robinsons wrote…

@SanjayVas - I put this here for now, but would you care for me to make this an actual property of SignatureAlgorithm? I can do this in a follow-up PR if you think there'd be value.

This is basically the inverse of fromKeyAndHashAlgorithm, right? Not a bad idea to move it to SignatureAlgorithm itself eventually. Feel free to just leave a TODO for now.

Copy link
Contributor

@sandip80 sandip80 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 11 files at r1, 8 of 12 files at r2, 4 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @robinsons)

@robinsons robinsons merged commit 9d7e3d9 into main Nov 20, 2024
4 checks passed
@robinsons robinsons deleted the robinsons-bouncy-castle branch November 20, 2024 21:04
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.

4 participants