IGN-7871 PKCS#11 signing #43

brianeray · 2024-02-20T22:58:02Z

📖 Background

The signModule custom task in the gradle-module-plugin (a.k.a. io.ia.sdk.modl) supports PKCS#12 file-based Java keystores for signing modules. Certain software supply chain best practices recommend using PKCS#11-based hardware security modules (HSMs) for more secure keystore implementations. The private keys never leave the HSM, and the HSM itself signs the module digests.

⚙️ Summary

Enhance the signModule task to allow for signing via HSM and other PKCS#11-compliant keystores. (Which need not be hardware tokens like YubiKeys, but may be cloud-based or other keystores implemented in software that are typically not local to the build machine.)

More specifically, add this capability via new property/option pkcs11CfgFile.

📝 Reviewer Notes

Use the Hide whitespace setting on the Files changed tab to eliminate a bunch of the delinting noise.

Because we call down to module-signer to do the actual signing, and because its filter is narrowed to keys of type SHA256withRSA, the key in the keystore must be that type. (This constraint predates the current enhancement.) At some point we'll likely enhance module-signer to support more key algorithms such as Elliptic Curve and whatever post-quantum algorithms ultimately pass muster with NIST and related entities.

On initial PR submission the version is 0.2.0-SNAPSHOT. After the PR is approved and any other validation--preferably some some testing by IA and partner developers--~~the PR will emerge from Draft status and~~ the version will bump to 0.2.0. We may have to hand-crank publication to the Gradle Plugin repository as I'm not sure whether the release.yml workflow is working.

🧪 QA Notes

This was integration tested against a YubiKey 5 NFC with the wrinkle that the slot 9a authorization key was used, not the 9c signing key. There are notes in SignModuleTest as to why this workaround is necessary. (At some point we might be able to sign with the "correct" YK5 slot but it appears to require some tricky callback code.)

Fixes IGN-7871 (/issues/41)

Add HSM/hardware token keystore capability via new property `pkcs11CfgFile`. Unlike filesystem-based `keystoreFile`, the signing key is stored in a device such as a YubiKey, with driver and other `SunPKCS11` provider properties specified in a configuration file for the `signModule` task. Other changes include refactoring existing tests in `SignModuleTest`, Spotless delinting in `gradle-module-plugin`, and a few minor test and typo fixes. NOTE 1: This is currently versioned at `0.2.0-SNAPSHOT`. After the PR is approved and any other validation--preferably some some testing by IA and partner developers--this will bump to `0.2.0`. NOTE 2: This was integration tested against a YubiKey 5 NFC with the wrinkle that the slot `9a` authorization key was used, not the `9c` signing key. There are notes in `SignModuleTest` as to why this workaround is necessary. (At some point we might be able to sign with the "correct" YK5 slot but it appears to require some tricky callback code.) NOTE 3: Because we call down to `module-signer` to do the actual signing, and because its filter is narrowed to keys of type `SHA256withRSA`, the key in the keystore must be that type. (This constraint predates the current enhancement.) At some point we'll likely enhance `module-signer` to support more key algorithms such as Elliptic Curve and whatever post-quantum algorithms ultimately pass muster with NIST and related entities.

brianeray · 2024-02-20T23:11:43Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

+            ignition.signing.certAlias=selfsigned
+            ignition.signing.keystorePassword=password
+            ignition.signing.certFile=./certificate.pem
+            ignition.signing.certPassword=password


No DRY/re-use on the constant values because 1) we want to test those constants too, not just the logic, and 2) clarify test inputs for the reader--More BDD-oriented.

brianeray · 2024-02-20T23:12:29Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

-    val keystore: Path,
-    val certFile: Path,
+    val keystore: Path?,
+    val certFile: Path?,


Laying the groundwork for fetching the cert file straight from the HSM. Not for all use cases, but some.

brianeray · 2024-02-20T23:14:23Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

@@ -50,25 +48,36 @@ open class BaseTest {
        return name
    }

-    protected fun prepSigningResourcesForModuleName(


Dead code AFAICT.

brianeray · 2024-02-20T23:16:54Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

+    open fun runTaskAndFail(
+        projectDir: File,
+        taskArgs: List<String>
+    ): BuildResult = setupRunner(projectDir, taskArgs).buildAndFail()


No need to try-catch on negative tests with buildAndFail.

brianeray · 2024-02-20T23:21:04Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/SignModuleTest.kt

+
+    @Test
+    // @Tag("IGN-7871")
+    fun `skip signing - no need for signing properties`() {


New test but on old use case. Just for more coverage.

brianeray · 2024-02-20T23:21:47Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/SignModuleTest.kt

+    // This is a test with an actual PKCS#11-compliant YubiKey 5, on Windows.
+    // As such it is typically set to @Ignore.
+    @Test
+    @Ignore


See comment a few lines up. We can't stick a YubiKey up on GitHub. 😄

brianeray · 2024-02-20T23:23:22Z

gradle-module-plugin/src/functionalTest/resources/certs/signing.properties

@@ -1,5 +0,0 @@
-ignition.signing.keystoreFile=./keystore.jks


Test fixture from older alpha version of the plugin, IIUC.

brianeray · 2024-02-20T23:24:57Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt

+                listOf(SKIP, null).none { v -> p.getOrNull() == v }
+            }
+        ) {
+            throw InvalidUserDataException(


If you squint at the Gradle API doc, this is the closest of their exception types that matches this situation.

brianeray · 2024-02-20T23:26:31Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt

-                "keystorePassword: ${"*".repeat(keystorePassword.length)}, " +
+            "Signing module with keystoreFile: ${keyStoreFile?.absolutePath}, " +
+                "pkcs11CfgFile: ${pkcs11CfgFile?.absolutePath}, " +
+                "keystorePassword: ${"*".repeat(20)}, " +


Leak no info whatsoever about secrets.

brianeray · 2024-02-20T23:26:53Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt


-        val privateKey: RSAPrivateKey = keyStore.getKey(certAlias, certPassword.toCharArray()) as RSAPrivateKey
+        val privateKey: PrivateKey = keyStore.getKey(certAlias, certPassword.toCharArray()) as PrivateKey


Laying the groundwork for other key algorithms.

brianeray · 2024-02-20T23:28:40Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt

@@ -194,6 +250,26 @@ open class SignModule @Inject constructor(_providers: ProviderFactory, _objects:
                throw Exception("Signing key file ${keystoreFile.absolutePath} did not exist!")
            }
        }
+
+        // PKCS#11 HSM (hardware key)-based keystore
+        if (pkcs11Cfg.isPresent) {


This and the whole getKeyStore function should arguably be refactored away in favor of module-signer's existing keystore + key-fetching logic.

brianeray · 2024-02-20T23:29:55Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt


        ModuleSigner(privateKey, cert.inputStream())
            .signModule(PrintStream(OutputStream.nullOutputStream()), unsignedModule, outFile)
    }
+
+    private fun loadKeyStore(ks: KeyStore, ksPwd: String, ksFile: File?) {


See comment above in getKeyStore about refactoring this away in favor of logic already in module-signer. Something for a future ticket.

paul-griffith

No real blockers - just some minor code cleanup opportunities I think

gradle-module-plugin/SIGNING_VIA_HSM.md

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

brianeray · 2024-02-21T19:23:30Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

        }

+        // uncomment if you need a little debugging
+        //println("props:\n$props")
+        gradlePropsFl.appendText(props)


For some reason I thought Kotlin was not auto-casting to String before.

brianeray · 2024-02-21T20:44:02Z

Holding off on merging until

I figure out how to publish a snapshot build that internal and external developers can kick around a bit.
Clean up that PR description.
"Promote" the version from 0.2.0-SNAPSHOT to release 0.2.0.
Publish that release version.
Maybe get Perry to take a look at it since he wrote most of this + filed the internal ticket.

PerryAJ

Have not had a chance to test with hardware myself so won't 'approve' yet, but the changes look good to me; if we have additional confirmed testers, I'm a go.

An integration partner has a Digicert KeyLocker cloud keystore, which requires passing explicitly down to the `:signModule` task neither `keystorePassword` nor `certPassword`; unlocking is via other means (env vars consumed by the vendor PKCS#11 module, IIUC). This commit makes those options/properties optional, translating their ommission as `null` down the call stacks. Interestingly, omitting `keystorePassword` is not even a blocker for PKCS#12 (file-based) keystores; it just skips an integrity check on keystore load when no keystore password is specified. NOTE: Plans are in place for `certFile` ommission as well. In many cases the cert chain can be read straight from the keystore. That will come in a future PR.

brianeray · 2024-03-12T20:34:59Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/BaseTest.kt

@@ -129,7 +129,7 @@ open class BaseTest {
        Files.createDirectories(targetDir)

        return resources.map { resource ->
-            ClassLoader.getSystemResourceAsStream("certs/$resource").let { inputStream ->
+            ClassLoader.getSystemResourceAsStream("certs/$resource").use { inputStream ->


Close potential stream leak.

brianeray · 2024-03-12T20:35:40Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/SignModuleTest.kt

-        assertContains(
-            out,
-            "file as 'ignition.signing.keystorePassword=<value>"
-        )


No longer immediate fatalities.

brianeray · 2024-03-12T20:36:55Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/SignModuleTest.kt

@@ -188,25 +176,49 @@ class SignModuleTest : BaseTest() {
            "--keystoreFile=${signResources.keystore}",
            "--certFile=${signResources.certFile}",
            "--certAlias=selfsigned",
+            // --keystorePassword is not required by :signModule task for most
+            // types of keystores, though can add an integrity check on ks load


Even PKCS#12 keystores don't care much about the keystore password on a simple KeyStore.load, only the cert password.

brianeray · 2024-03-12T20:37:47Z

gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/SignModuleTest.kt

+    // outside of the call stack. Simulate this, somehow, if we can figure
+    // out a good way to do so.
+    @Test
+    @Ignore


Iron this out in a future issue/ticket/PR.

brianeray · 2024-03-12T20:38:56Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt

-            unsignedModule,
-            signed.get().asFile
-        )
+        signModule(unsignedModule)


Sonar was whining about the number of args. Plus all but unsignedModule are instance properties.

brianeray · 2024-03-12T20:40:23Z

gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/SignModule.kt

        ksFile?.inputStream()
            // if PKCS#11 HSM (hardware key) keystore, forgo the input stream
            ?.takeUnless { ks.type == PKCS11_KS_TYPE }
            .use { maybeStream ->
-                ks.load(maybeStream, ksPwd.toCharArray())
+                ks.load(maybeStream, ksPwd?.toCharArray())


null here is not a blocker even on PKCS#12 (only?) keystores with passwords; see the Javadoc for the impact.

KathyApplebaum

lgtm

brianeray · 2024-03-22T16:27:23Z

QAed by both an integration partner using DigiCert KeyLocker and by IA QA with a YubiKey 5 and a genuine, non self-signed cert.

brianeray · 2024-03-22T21:47:41Z

See this IA forum post for instructions on using the new, 0.2.0 version of the plugin.

brianeray · 2024-03-22T23:38:58Z

Oddly enough we had a GitHub workflow that is still publishing to the Gradle Plugin Portal. 0.2.0 is up there now.

brianeray requested a review from PerryAJ February 20, 2024 22:58

brianeray commented Feb 20, 2024

View reviewed changes

brianeray requested a review from kevinherron February 20, 2024 23:47

IGN-7871 Fix typos + thinkos in the new guidance

c1993fd

paul-griffith requested changes Feb 21, 2024

View reviewed changes

brianeray added 4 commits February 21, 2024 10:06

IGN-7871 Zap guidance word salad

f6d6741

IGN-7871 De-obfuscate task flag validations

4758ecb

IGN-7871 Patch input stream leak

f33def9

IGN-7871 Slightly clearer test fixture helper

1fd982f

brianeray commented Feb 21, 2024

View reviewed changes

IGN-7871 Spotless de-linting latest commit(s)

958e5eb

paul-griffith previously approved these changes Feb 21, 2024

View reviewed changes

PerryAJ reviewed Feb 27, 2024

View reviewed changes

brianeray marked this pull request as ready for review February 27, 2024 16:51

brianeray mentioned this pull request Mar 4, 2024

Latest published gradle-module-plugin deploy task fails with exception #47

Closed

IGN-7871 Guidance on YubiKey 5 slot workaround

37f88f0

brianeray dismissed paul-griffith’s stale review via 37f88f0 March 6, 2024 22:05

brianeray added 2 commits March 6, 2024 15:37

IGN-7871 Fix existing README.md copy-pastas

4c03815

brianeray commented Mar 12, 2024

View reviewed changes

IGN-7871 Final release version 0.2.0

1a3814a

brianeray requested review from KathyApplebaum and removed request for kevinherron March 22, 2024 16:18

KathyApplebaum approved these changes Mar 22, 2024

View reviewed changes

brianeray merged commit 579957f into inductiveautomation:master Mar 22, 2024
1 check passed

brianeray deleted the IGN-7871-pkcs11 branch March 22, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGN-7871 PKCS#11 signing #43

IGN-7871 PKCS#11 signing #43

brianeray commented Feb 20, 2024 •

edited

brianeray Feb 20, 2024 •

edited

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

brianeray Feb 20, 2024

paul-griffith left a comment

brianeray Feb 21, 2024

brianeray commented Feb 21, 2024 •

edited

PerryAJ left a comment

brianeray Mar 12, 2024

brianeray Mar 12, 2024

brianeray Mar 12, 2024

brianeray Mar 12, 2024

brianeray Mar 12, 2024

brianeray Mar 12, 2024

KathyApplebaum left a comment

brianeray commented Mar 22, 2024

brianeray commented Mar 22, 2024

brianeray commented Mar 22, 2024


		val privateKey: RSAPrivateKey = keyStore.getKey(certAlias, certPassword.toCharArray()) as RSAPrivateKey
		val privateKey: PrivateKey = keyStore.getKey(certAlias, certPassword.toCharArray()) as PrivateKey

IGN-7871 PKCS#11 signing #43

IGN-7871 PKCS#11 signing #43

Conversation

brianeray commented Feb 20, 2024 • edited

📖 Background

⚙️ Summary

📝 Reviewer Notes

🧪 QA Notes

brianeray Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-griffith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianeray commented Feb 21, 2024 • edited

PerryAJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KathyApplebaum left a comment

Choose a reason for hiding this comment

brianeray commented Mar 22, 2024

brianeray commented Mar 22, 2024

brianeray commented Mar 22, 2024

brianeray commented Feb 20, 2024 •

edited

brianeray Feb 20, 2024 •

edited

brianeray commented Feb 21, 2024 •

edited