Skip to content

add full support to wolfcrypt tests for random.c cryptocbs#7271

Merged
douzzer merged 2 commits intowolfSSL:masterfrom
bigbrett:cryptocb-random-wctestfix
Feb 28, 2024
Merged

add full support to wolfcrypt tests for random.c cryptocbs#7271
douzzer merged 2 commits intowolfSSL:masterfrom
bigbrett:cryptocb-random-wctestfix

Conversation

@bigbrett
Copy link
Copy Markdown
Contributor

@bigbrett bigbrett commented Feb 23, 2024

Description

  • Adds a new _ex function for RNG initialization that takes a devid argument and adds return code for better error propagation
  • Enables NO_DEV_RANDOM (e.g. bare metal) targets to use a cryptoCb for DRBG seed generation

Testing

Unit tests pass with new additions

@bigbrett bigbrett assigned wolfSSL-Bot and bigbrett and unassigned wolfSSL-Bot and bigbrett Feb 23, 2024
dgarske
dgarske previously approved these changes Feb 26, 2024
Comment thread wolfcrypt/src/random.c Outdated
}


WOLFSSL_ABI
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be WOLFSSL_API instead of WOLFSSL_ABI?

Copy link
Copy Markdown
Contributor Author

@bigbrett bigbrett Feb 26, 2024

Choose a reason for hiding this comment

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

@JacobBarthelmeh I just used the same function annotation as wc_rng_new, assuming this new function would inherit the same properties as the non-"_ex" version. Note: It is declared as both WOLFSSL_API and WOLFSSL_ABI in the header file.

I see the same pattern used for a few other _ex() functions that we have added (but not all). I assumed we could always add ABI functions, just not subtract. What is the guidance here, would you like me to change it to just WOLFSSL_API?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, once an API is WOLFSSL_ABI for a release it can not be altered. We try to keep public API the same in any event, but WOLFSSL_ABI is binding it. I'd lean towards not adding it to WOLFSSL_ABI until required, but is only a comment not something to hold up this PR if wanting it to start it out as WOLFSSL_ABI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @JacobBarthelmeh that we don't need to be boxing ourselves in on this. For one thing, a minor and useful refactor is possible here to allow nonce to be passed in as a const byte *. If we make it WOLFSSL_ABI we can't do that.

I just did an automated survey of the existing WOLFSSL_ABI functions in wolfCrypt and there's only two _ex() functions in the set, wc_ecc_make_key_ex() and wc_ecc_init_ex(). You can be sure there was some specific reason(s) a customer needed those stabilized.

Btw the total number of WOLFSSL_ABI prototypes in wolfCrypt is just 45, of 1120 WOLFSSL_APIs. In the native TLS layer we currently have 52, of 1711 WOLFSSL_APIs. They're rare birds!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great, I agree on all counts. Thanks for the feedback. I'll remove WOLFSSL_ABI and we can always add it back in if we want to lock it down

Comment thread wolfcrypt/src/random.c Outdated
rng = (WC_RNG*)XMALLOC(sizeof(WC_RNG), heap, DYNAMIC_TYPE_RNG);
if (rng) {
int error = _InitRng(rng, nonce, nonceSz, heap, devId) != 0;
if (error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's probably a mistake to double down on the blinded-error-code oversight in wc_rng_new(). This is the _ex version so this is an opportunity to fix it, with something like

int wc_rng_new_ex(WC_RNG **rng, byte* nonce, word32 nonceSz, void* heap, int devId) {
    int ret;
    *rng = (WC_RNG*)XMALLOC(sizeof(WC_RNG), heap, DYNAMIC_TYPE_RNG);
    [...]
    return ret;
}

Also error shouldn't be used here because it collides with a glibc function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@douzzer addressed API changes in latest commit, lmk what you think

Comment thread wolfcrypt/src/random.c Outdated
}


WOLFSSL_ABI
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @JacobBarthelmeh that we don't need to be boxing ourselves in on this. For one thing, a minor and useful refactor is possible here to allow nonce to be passed in as a const byte *. If we make it WOLFSSL_ABI we can't do that.

I just did an automated survey of the existing WOLFSSL_ABI functions in wolfCrypt and there's only two _ex() functions in the set, wc_ecc_make_key_ex() and wc_ecc_init_ex(). You can be sure there was some specific reason(s) a customer needed those stabilized.

Btw the total number of WOLFSSL_ABI prototypes in wolfCrypt is just 45, of 1120 WOLFSSL_APIs. In the native TLS layer we currently have 52, of 1711 WOLFSSL_APIs. They're rare birds!

@bigbrett
Copy link
Copy Markdown
Contributor Author

Jenkins retest this please

@douzzer douzzer merged commit af31fbc into wolfSSL:master Feb 28, 2024
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.

5 participants