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

Fix demos to work with new wolfHSM API #17

Merged
merged 18 commits into from
Nov 5, 2024

Conversation

bigbrett
Copy link
Contributor

@bigbrett bigbrett commented Nov 1, 2024

  • Updates demo code to use new wolfHSM API
  • Fixes error code propagation in crypto demos

Based on #15 but with additional improvements

Requires curve25519 fixes introduced in wolfSSL/wolfHSM#83. CI will fail until that merges.

@bigbrett bigbrett requested a review from billphipps November 1, 2024 21:56
@bigbrett bigbrett mentioned this pull request Nov 1, 2024
Closed
billphipps
billphipps previously approved these changes Nov 5, 2024
Copy link
Contributor

@billphipps billphipps left a comment

Choose a reason for hiding this comment

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

Looks good! Consider reenabling ecc in these tests.


#if defined(WOLFSSL_CMAC)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have additional constraints on CMAC like !NO_AES and WOLFSSL_AES_DIRECT. Do you capture these in the lower level? I'd love to simplify this compile-time logic to exactly what you have written here.

@@ -48,8 +48,7 @@
int wh_DemoClient_CryptoRsa(whClientContext* clientContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also need WOLFSSL_KEY_GEN for the make rsakey below?

if (ret != 0) {
printf("Failed to wh_Client_GetKeyIdRsa %d\n", ret);
return ret;
int evictRet = wh_Client_GetKeyIdEcc(eccPrivate, &keyId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this supposed to be wh_Client_EccGetKeyId(...)?

@@ -116,7 +120,7 @@ int wh_DemoClient_KeystoreCommitKey(whClientContext* clientContext)
return WH_ERROR_OK;
}


#ifndef NO_AES
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to need AES_CBC as well


/* Temporarily set this to key export function */
#define WOLFSSL_KEY_GEN
#define HAVE_CURVE25519
#define HAVE_ECC
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend to reenable this to make sure we have coverage there as well. Ok to leave it off for now and fix it in a later PR

Copy link
Contributor

@billphipps billphipps left a comment

Choose a reason for hiding this comment

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

LGTM!

#!/bin/bash

# ECC Keys
openssl genpkey -algorithm EC -pkeyopt ec_paramgen_curve:prime256v1 -out alice-ecc256-key.pem
Copy link
Contributor

Choose a reason for hiding this comment

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

Grumble. Hate relying on openssl. Maybe we should leave some pregenerated keys in PEM format in the repo as well in case they don't have openssl available? No change recommended at this point. Just a thought.

@billphipps billphipps merged commit 98fa58d into wolfSSL:main Nov 5, 2024
1 check passed
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