Support for EC keys, fix Attributes processing by ya-mouse · Pull Request #369 · google/native-pkcs11

ya-mouse · 2024-11-22T19:01:20Z

This change adds proper handling for EC keys (addressing #303) as well as proper RSA parameters calculation.

NOTE: For instance, openssl expects that EC params (curve OID) and points are in specific uncompressed and padded format. In Go encoding EC_POINTS be like:

case C.CKA_EC_POINT:
    encodedPoint, err := encodeECPoint(key.Curve, key.X, key.Y)
    ...
    setValue(attr, unsafe.Pointer(&encodedPoint[0]), C.CK_ULONG(len(encodedPoint)))

func encodeECPoint(curve elliptic.Curve, x, y *big.Int) ([]byte, error) {
        fieldSize := (curve.Params().BitSize + 7) / 8
        xPadded := make([]byte, fieldSize)
        yPadded := make([]byte, fieldSize)
        copy(xPadded[fieldSize-len(x.Bytes()):], x.Bytes())
        copy(yPadded[fieldSize-len(y.Bytes()):], y.Bytes())

        ecPoint := append([]byte{0x04}, append(xPadded, yPadded...)...)

        return asn1.Marshal(asn1.RawValue{
                Tag:   asn1.TagOctetString,
                Bytes: ecPoint,
        })
}

I faced this issue when implementing fake custom backend using regular PEM/DER files or passing signature produced by AWS KMS.
Perhaps, as later improvement we can try to instantiate specific signature from DER bytes and then repack params into expected format. Now this is offloaded to the backend.

The changes were checked with my custom backend using RSA2048 and ECC NIST-P256 keys via OpenSSL.

google-cla · 2024-11-22T19:01:25Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ya-mouse · 2024-11-22T20:01:42Z

native-pkcs11/src/lib.rs

+                            "Unsupported attribute type - marking as unavailable"
+                        );
+                        attribute.ulValueLen = CK_UNAVAILABLE_INFORMATION;
+                        // result = Err(Error::AttributeTypeInvalid(attribute.type_));


we should skip unknown attributes otherwise a tool that handles pkcs11 calls silently tries to interpret random values from the template: pkcs11-tool and p11-kit produces weird values for attributes when inspecting with pkcs11-spy.

Can you please split the bug fix for attribute parsing into a separate PR?

Gently ping :)

ya-mouse · 2024-11-22T20:02:41Z

native-pkcs11-traits/src/lib.rs


 pub type Result<T> = std::result::Result<T, Box<dyn std::error::Error>>;
-pub type Digest = [u8; 20];
+pub type Digest = [u8; 64];


was small to fit ECC P-256 digest value

ya-mouse · 2024-11-22T20:04:18Z

native-pkcs11-core/src/object.rs

+                AttributeType::Verify => Some(Attribute::Verify(true)),
+                AttributeType::VerifyRecover => Some(Attribute::VerifyRecover(false)),
+                AttributeType::Wrap => Some(Attribute::Wrap(false)),
+                AttributeType::Encrypt => Some(Attribute::Encrypt(false)),


Added some missed attributes with default values that pkcs11-tool/p11-kit tools complains about.

Apologies for the churn but can you also make this change in a separate PR? I'm working on adding integration tests for pkcs11-tool output I would like to use to validate this change.

brandonweeks · 2024-12-18T21:31:17Z

Thank you for the PR! Just to set expectations, I probably won't have an opportunity to review and test until the new year.

brandonweeks · 2025-02-20T20:12:29Z

native-pkcs11-core/src/object.rs

+    // For EC keys, the algorithm parameters contain the curve OID
+    // For EC keys, the subject public key is the EC point
+    Some((
+        ObjectIdentifier::from_bytes(spki.algorithm.parameters.unwrap().value())


spki.algorithm.parameters_oid().unwrap().to_der().unwrap() seems to do the same thing.

brandonweeks · 2025-02-20T20:16:25Z

native-pkcs11-core/src/object.rs

+            .unwrap()
+            .to_der()
+            .unwrap(),
+        OctetString::new(spki.subject_public_key.raw_bytes()).unwrap().to_der().unwrap(),


spki.subject_public_key.to_der().unwrap() also seems like an easier way to get the key as DER encoded unless I'm missing something.

brandonweeks · 2025-02-20T20:26:10Z

native-pkcs11-core/src/object.rs

+    // Parse the DER-encoded SPKI
+    let spki: SubjectPublicKeyInfoRef<'_> = SubjectPublicKeyInfoRef::try_from(der_bytes).unwrap();
+    // Parse the RSA public key bytes from the SPKI
+    let rsa_pubkey = RsaPublicKey::from_der(spki.subject_public_key.raw_bytes()).ok()?;


RsaPublicKey::from_der(spki.subject_public_key.as_bytes()?).ok()? works as well.

brandonweeks · 2025-02-20T20:28:37Z

native-pkcs11-core/src/object.rs

+    ))
+}
+
+fn extract_rsa_params(der_bytes: &[u8]) -> Option<(Vec<u8>, Vec<u8>, u64)> {


Nit: return a usize from the method and do the type casting further down.

brandonweeks · 2025-02-20T20:29:33Z

native-pkcs11-core/src/object.rs

+    let spki: SubjectPublicKeyInfoRef<'_> = SubjectPublicKeyInfoRef::try_from(der_bytes).unwrap();
+    // Parse the RSA public key bytes from the SPKI
+    let rsa_pubkey = RsaPublicKey::from_der(spki.subject_public_key.raw_bytes()).ok()?;
+    let modulus = rsa_pubkey.modulus.as_bytes();


Nit: call rsa_pubkey.modulus.as_bytes().to_vec() in the struct constructor instead of assigning to a variable.

ya-mouse force-pushed the keys-attributes branch from 9c95185 to 8387a48 Compare November 22, 2024 19:09

ya-mouse commented Nov 22, 2024

View reviewed changes

brandonweeks self-requested a review December 11, 2024 01:13

ya-mouse force-pushed the keys-attributes branch from 8387a48 to cf42ba3 Compare December 18, 2024 10:42

ya-mouse force-pushed the keys-attributes branch from 9302927 to 32315ac Compare December 20, 2024 07:53

ya-mouse force-pushed the keys-attributes branch 2 times, most recently from f84d10f to 2c5b44e Compare January 14, 2025 19:21

Support for EC keys, fix Attributes processing

b66b6e0

brandonweeks force-pushed the keys-attributes branch from 2c5b44e to b66b6e0 Compare February 20, 2025 20:09

brandonweeks reviewed Feb 20, 2025

View reviewed changes

Comments

Conversation

ya-mouse commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla bot commented Nov 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonweeks commented Dec 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ya-mouse commented Nov 22, 2024 •

edited

Loading