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

Support for EC keys, fix Attributes processing #369

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ya-mouse
Copy link

@ya-mouse ya-mouse commented Nov 22, 2024

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.

Copy link

google-cla bot commented Nov 22, 2024

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@@ -21,7 +21,7 @@ use std::{
use x509_cert::der::Decode;

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

Choose a reason for hiding this comment

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

was small to fit ECC P-256 digest value

AttributeType::Verify => Some(Attribute::Verify(true)),
AttributeType::VerifyRecover => Some(Attribute::VerifyRecover(false)),
AttributeType::Wrap => Some(Attribute::Wrap(false)),
AttributeType::Encrypt => Some(Attribute::Encrypt(false)),
Copy link
Author

Choose a reason for hiding this comment

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

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

@brandonweeks
Copy link
Member

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

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.

2 participants