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.

Sign up for GitHub

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

Jump to bottom

ta: pkcs11: Add certificate object support #4797

Merged
merged 1 commit into from
Oct 5, 2021
Merged

ta: pkcs11: Add certificate object support #4797

merged 1 commit into from
Oct 5, 2021

Conversation

vesajaaskelainen
Copy link
Contributor

@vesajaaskelainen vesajaaskelainen commented Aug 13, 2021 •
edited
Loading

Adds support for:

PKCS #11 Cryptographic Token Interface Base Specification Version 2.40
Plus Errata 01

4.6 Certificate objects
4.6.3 X.509 public key certificate objects

Relates to:

Related PRs:

Signed-off-by: Vesa Jääskeläinen vesa.jaaskelainen@vaisala.com

Before there is tests available here are some manual steps on how to test it out.

NSS certutil is the easiest command line tool to import certificates:

# Your token settings
export PKCS11_MODULE=/usr/lib/libckteec.so.0
export PKCS11_SLOT=0
export PKCS11_TOKEN=token
export PKCS11_SO_PIN=1234567890
export PKCS11_USER_PIN=1234

# 0. Initialize token
pkcs11-tool --module ${PKCS11_MODULE} --slot-index ${PKCS11_SLOT} --init-token --label ${PKCS11_TOKEN} --so-pin ${PKCS11_SO_PIN}
pkcs11-tool --module ${PKCS11_MODULE} --slot-index ${PKCS11_SLOT} --init-pin --login --so-pin ${PKCS11_SO_PIN} --new-pin ${PKCS11_USER_PIN}

# 1. Configure NSS with OP-TEE (can also be rootfs build time thing)
modutil -dbdir sql:/etc/pki/nssdb -list

# Note: this writes to the nssdb location so needs to be writable during configuration time
modutil -dbdir sql:/etc/pki/nssdb -add "OP-TEE" -libfile ${PKCS11_MODULE} -mechanisms RSA

# 2. Some runtime helpers to make life easier with scripts
mkdir -p /run/pki && echo ${PKCS11_USER_PIN} > /run/pki/pin && dd if=/dev/random of=/run/pki/noise bs=1 count=32

# 3. Import your selected certificate (Root CA in this example)
export X509_CERT_LABEL=GlobalSign_Root_CA
export X509_CERT_FILE=/usr/share/ca-certificates/mozilla/GlobalSign_Root_CA.crt

certutil -d sql:/etc/pki/nssdb -h ${PKCS11_TOKEN} -f /run/pki/pin -A -n ${X509_CERT_LABEL} -i ${X509_CERT_FILE} -t "C,,"

# 4. Check that it is there with pkcs11-tool
pkcs11-tool --module ${PKCS11_MODULE} --token ${PKCS11_TOKEN} --login --pin ${PKCS11_USER_PIN} --list-objects --type cert

# 5. Generate key and CSR with NSS:
export RSA_KEY_LABEL=rsa-key

# Note: -z argument makes life easier with scripting and that random is not really used for key generation
certutil -z /run/pki/noise -d sql:/etc/pki/nssdb -h ${PKCS11_TOKEN} -f /run/pki/pin -R -k rsa -g 2048 -n ${RSA_KEY_LABEL} -a -o test.csr -Z SHA256 -s "C=FI, O=Manufacture, CN=Device, serialNumber=L12345" --keyUsage digitalSignature,keyAgreement --extKeyUsage serverAuth,clientAuth

# 6. Observer all objects
pkcs11-tool --module ${PKCS11_MODULE} --token ${PKCS11_TOKEN} --login --pin ${PKCS11_USER_PIN} --list-objects
# For some reason the label is lost and not stored by NSS (at least the version under testing)

# 7. Import signed certificate (for above CSR)
certutil -d sql:/etc/pki/nssdb -h ${PKCS11_TOKEN} -f /run/pki/pin -A -n ${RSA_KEY_LABEL} -i test.crt -t "C,,"

# 8. Observer all objects
pkcs11-tool --module ${PKCS11_MODULE} --token ${PKCS11_TOKEN} --login --pin ${PKCS11_USER_PIN} --list-objects
# Observe that ID for public key, private key and for certificate are the same.

# 9. Dump DER of certificate and make is readable with openssl
pkcs11-tool --module ${PKCS11_MODULE} --token ${PKCS11_TOKEN} --login --pin ${PKCS11_USER_PIN} --read-object --label ${RSA_KEY_LABEL} --type cert | openssl x509 -text -noout -inform DER

@vesajaaskelainen
Copy link
Contributor Author

Note: I try to cook up some simple xtest certificate test while the RSA test PRs are in review.

@vesajaaskelainen vesajaaskelainen mentioned this pull request Aug 14, 2021
Merged
@vesajaaskelainen vesajaaskelainen mentioned this pull request Aug 27, 2021
Merged
etienne-lms
etienne-lms reviewed Sep 9, 2021
View reviewed changes
ta/pkcs11/src/pkcs11_attributes.c Outdated
rc = get_attribute_ptr(*out, PKCS11_CKA_CERTIFICATE_CATEGORY, NULL,
&attr_size);
if ((rc == PKCS11_CKR_OK && attr_size == 0) ||
rc == PKCS11_RV_NOT_FOUND) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: could prevent few line breaks:

	if ((rc == PKCS11_CKR_OK && !attr_size) || rc == PKCS11_RV_NOT_FOUND) {
		rc = set_attribute(out, PKCS11_CKA_CERTIFICATE_CATEGORY,
				   &cert_category, sizeof(cert_category));
		if (rc)
			return rc;
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, prefer have cert_category defined in this block. Easier to read up:

	if ((rc == PKCS11_CKR_OK && !attr_size) || rc == PKCS11_RV_NOT_FOUND) {
+		uint32_t cert_category = PKCS11_CK_CERTIFICATE_CATEGORY_UNSPECIFIED;
+
		rc = set_attribute(out, PKCS11_CKA_CERTIFICATE_CATEGORY,
				   &cert_category, sizeof(cert_category));
		if (rc)
			return rc;
	}

ditto for name_hash_alg in the next instruction block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving cert_category there causes checkpatch issue -- so I left variables where they were.

Did the line break changes.

ta/pkcs11/src/pkcs11_attributes.c Show resolved Hide resolved
ta/pkcs11/include/pkcs11_ta.h Show resolved Hide resolved
ta/pkcs11/src/pkcs11_attributes.c Outdated

static const uint32_t pkcs11_certificate_optional[] = {
PKCS11_CKA_CERTIFICATE_CATEGORY, PKCS11_CKA_CHECK_VALUE,
PKCS11_CKA_START_DATE, PKCS11_CKA_END_DATE, PKCS11_CKA_PUBLIC_KEY_INFO
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: prefer add a comma at line end. (ditto line 370)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did same for pkcs11_x509_certificate_optional

etienne-lms
etienne-lms reviewed Sep 21, 2021
View reviewed changes
ta/pkcs11/include/pkcs11_ta.h Show resolved Hide resolved
ta/pkcs11/src/pkcs11_attributes.c Outdated
size_t mandated_count = 0;
size_t optional_count = 0;
uint32_t attr_size = 0;
uint32_t cert_category = PKCS11_CK_CERTIFICATE_CATEGORY_UNSPECIFIED;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should maybe be renamed default_category to avoid confusion.

Same a next line: name_hash_alg => default_hash_alg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

ta/pkcs11/src/pkcs11_attributes.c Outdated
if ((rc == PKCS11_CKR_OK && !attr_size) || rc == PKCS11_RV_NOT_FOUND) {
rc = set_attribute(out, PKCS11_CKA_CERTIFICATE_CATEGORY,
&cert_category,
sizeof(cert_category));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check the already existing category is a known value?

Ditto for the hash algo ID checked right below.

Copy link
Contributor Author

@vesajaaskelainen vesajaaskelainen Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good question.

For hash -- hash value and hash algo is fully processed outside of PKCS11 TA so I suppose we shouldn't touch that. We can check the size for that -> will add that.

For category we may as well validate that. -- will add code for that.

ta/pkcs11/src/pkcs11_attributes.c Show resolved Hide resolved
@vesajaaskelainen vesajaaskelainen mentioned this pull request Oct 1, 2021
Merged
@vesajaaskelainen
Copy link
Contributor Author

@etienne-lms sorry for the delay on changes -- now should be updated and comments addressed.

@vesajaaskelainen
Copy link
Contributor Author

While adding test cases for error handling I noticed there was problem in libckteec so created a new PR for those:
OP-TEE/optee_client#287

etienne-lms
etienne-lms reviewed Oct 4, 2021
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

minor comment.
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> once addressed.

ta/pkcs11/src/attributes.h

if (get_attribute(head, PKCS11_CKA_CERTIFICATE_TYPE, &type, &size))
return PKCS11_CKC_UNDEFINED_ID;

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: assert(size == sizeof(uint32_t));
or I think you could even drop fetching size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel better if we fetch it ;) ... but will add the assert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh -- none of the other methods with the same construct have it -- should we add assert to those too?

Copy link
Contributor Author

@vesajaaskelainen vesajaaskelainen Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

get_u32_attribute() would have sanity check embedded -- shall we use this and no assert?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, get_u32_attribute() and no extra assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with that.

@vesajaaskelainen
ta: pkcs11: Add certificate object support
264748b 
Adds support for:

PKCS OP-TEE#11 Cryptographic Token Interface Base Specification Version 2.40
Plus Errata 01

4.6 Certificate objects
4.6.3 X.509 public key certificate objects

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
@vesajaaskelainen
Copy link
Contributor Author

Comments addressed.

Thanks for the review! Applied tags.

@jforissier jforissier merged commit 4137952 into OP-TEE:master Oct 5, 2021
@etienne-lms etienne-lms mentioned this pull request Nov 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienne-lms etienne-lms etienne-lms left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vesajaaskelainen @etienne-lms @jforissier