Skip to content

Add BN_bn2binpad API and enable OpenVPN master CI testing#10148

Open
julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
julek-wolfssl:openvpn-master-bn2binpad
Open

Add BN_bn2binpad API and enable OpenVPN master CI testing#10148
julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
julek-wolfssl:openvpn-master-bn2binpad

Conversation

@julek-wolfssl
Copy link
Copy Markdown
Member

No description provided.

@julek-wolfssl julek-wolfssl self-assigned this Apr 7, 2026
Copilot AI review requested due to automatic review settings April 7, 2026 12:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds OpenSSL-compatible BN_bn2binpad support to wolfSSL’s OpenSSL shim layer and updates the OpenVPN GitHub Actions workflow to test against OpenVPN master again now that the API exists.

Changes:

  • Added wolfSSL_BN_bn2binpad declaration + BN_bn2binpad macro mapping.
  • Implemented wolfSSL_BN_bn2binpad with left-zero-padding semantics and size checks.
  • Expanded BN API tests and updated OpenVPN CI matrix to include master.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
wolfssl/openssl/bn.h Exposes wolfSSL_BN_bn2binpad and maps BN_bn2binpad to it.
src/ssl_bn.c Implements wolfSSL_BN_bn2binpad encoding/padding behavior.
tests/api/test_ossl_bn.c Adds coverage for BN_bn2binpad success/error cases.
.github/workflows/openvpn.yml Re-enables testing against OpenVPN master in CI matrix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings April 8, 2026 10:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@julek-wolfssl julek-wolfssl force-pushed the openvpn-master-bn2binpad branch from d8e9584 to 312de40 Compare April 8, 2026 13:06
Copilot AI review requested due to automatic review settings April 8, 2026 16:11
@julek-wolfssl julek-wolfssl force-pushed the openvpn-master-bn2binpad branch from 312de40 to 175cdaa Compare April 8, 2026 16:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +501 to +504
if (numBytes > toLen) {
WOLFSSL_MSG("BN too large for toLen");
return WOLFSSL_FATAL_ERROR;
}
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

The API contract in the comment says this function returns -1 on error, but the implementation returns WOLFSSL_FATAL_ERROR. This works only if WOLFSSL_FATAL_ERROR is guaranteed to be -1 across all builds/configurations. To keep OpenSSL-compat behavior explicit and stable, return -1 directly (or ensure WOLFSSL_FATAL_ERROR is definitionally -1 for this API surface).

Copilot uses AI. Check for mistakes.
Comment on lines +331 to +333
ExpectIntEQ(BN_bn2binpad(NULL, padOut, sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(&emptyBN, padOut, sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(a, NULL, sizeof(padOut)), -1);
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

sizeof(padOut) is a size_t but BN_bn2binpad takes an int length. Some toolchains treat this as a sign-conversion warning (and in general it’s safer to avoid implicit narrowing). Consider casting explicitly, e.g. (int)sizeof(padOut), for the test calls that pass sizeof(...).

Suggested change
ExpectIntEQ(BN_bn2binpad(NULL, padOut, sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(&emptyBN, padOut, sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(a, NULL, sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(NULL, padOut, (int)sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(&emptyBN, padOut, (int)sizeof(padOut)), -1);
ExpectIntEQ(BN_bn2binpad(a, NULL, (int)sizeof(padOut)), -1);

Copilot uses AI. Check for mistakes.
check_hidden: true
# Add comma separated list of words that occur multiple times that should be ignored (sorted alphabetically, case sensitive)
ignore_words_list: adin,aNULL,brunch,carryIn,chainG,ciph,cLen,cliKs,dout,haveA,inCreated,inOut,inout,larg,LEAPYEAR,Merget,optionA,parm,parms,repid,rIn,userA,ser,siz,te,Te,HSI,failT,
ignore_words_list: adin,aNULL,brunch,carryIn,chainG,ciph,cLen,cliKs,dout,haveA,inCreated,inOut,inout,larg,LEAPYEAR,Merget,optionA,parm,parms,repid,rIn,userA,ser,siz,te,Te,HSI,failT,toLen,
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

The workflow comment says ignore_words_list should be sorted alphabetically (case sensitive). Adding toLen at the end breaks that rule; please insert it in sorted order to keep the list maintainable and consistent with the documented expectation.

Suggested change
ignore_words_list: adin,aNULL,brunch,carryIn,chainG,ciph,cLen,cliKs,dout,haveA,inCreated,inOut,inout,larg,LEAPYEAR,Merget,optionA,parm,parms,repid,rIn,userA,ser,siz,te,Te,HSI,failT,toLen,
ignore_words_list: adin,aNULL,brunch,carryIn,chainG,ciph,cLen,cliKs,dout,haveA,inCreated,inOut,inout,larg,LEAPYEAR,Merget,optionA,parm,parms,repid,rIn,ser,siz,te,toLen,userA,Te,HSI,failT,

Copilot uses AI. Check for mistakes.
@julek-wolfssl julek-wolfssl force-pushed the openvpn-master-bn2binpad branch from 175cdaa to 0c43158 Compare April 9, 2026 11:47
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