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

add cpp linking test #1963

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion .github/workflows/basic.yml
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,36 @@ jobs:
run: ninja gen_docs
working-directory: build


cppcheck:
name: Check C++ linking with example program
runs-on: ubuntu-latest
container: openquantumsafe/ci-ubuntu-latest:latest
env:
SIG_NAME: ml_dsa_65
steps:
- name: Checkout code
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # pin@v4
- name: Configure
run: |
mkdir build && \
cd build && \
cmake -GNinja -DOQS_STRICT_WARNINGS=ON \
-GNinja \
-DOQS_MINIMAL_BUILD="SIG_$SIG_NAME" \
--warn-uninitialized .. > config.log 2>&1 && \
cat config.log && \
cmake -LA -N .. && \
! (grep -i "uninitialized variable" config.log)
- name: Build liboqs
run: ninja
working-directory: build
- name: Link with C++ program
run: |
g++ ../cpp/cpp_linking_example.cpp -g \
-I./include -L./lib -loqs -lcrypto -std=c++11 -o example_sig && \
./example_sig
working-directory: build

fuzzbuildcheck:
name: Check that code passes a basic fuzzing build
needs: [ workflowcheck, stylecheck, upstreamcheck ]
Expand Down
182 changes: 182 additions & 0 deletions cpp/cpp_linking_example.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/*
* example_sig.cpp
*
* Minimal C++ example of using a post-quantum signature implemented in liboqs.
*
* SPDX-License-Identifier: MIT
*/

#include <cstdint>
#include <cstdlib>
#include <cstring>
#include <iostream>
#include <memory>

#include <oqs/oqs.h>

constexpr size_t MESSAGE_LEN = 50;

/* Cleaning up memory etc */
void cleanup_stack(uint8_t *secret_key, size_t secret_key_len);

struct OQSSecureDeleter {
size_t length;

explicit OQSSecureDeleter(size_t len) : length(len) {}

void operator()(uint8_t* ptr) const {
if (ptr) {
OQS_MEM_secure_free(ptr, length);
}
}
};

struct OQSInsecureDeleter {
void operator()(uint8_t* ptr) {
if (ptr) {
OQS_MEM_insecure_free(ptr);
}
}
};

struct OQSSigDeleter {
void operator()(OQS_SIG* sig) {
if (sig) {
OQS_SIG_free(sig);
}
}
};


/* This function gives an example of the signing operations
* using only compile-time macros and allocating variables
* statically on the stack, calling a specific algorithm's functions
* directly.
*
* The macros OQS_SIG_dilithium_2_length_* and the functions OQS_SIG_dilithium_2_*
* are only defined if the algorithm dilithium_2 was enabled at compile-time
* which must be checked using the OQS_ENABLE_SIG_dilithium_2 macro.
*
* <oqs/oqsconfig.h>, which is included in <oqs/oqs.h>, contains macros
* indicating which algorithms were enabled when this instance of liboqs
* was compiled.
*/
static OQS_STATUS example_stack(void) {

#ifdef OQS_ENABLE_SIG_dilithium_2
Copy link
Member

Choose a reason for hiding this comment

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

Hmm -- I'm not sure I understand this: CI seems to disable Dilithium2 and this test basically only runs when Dilithium2 is enabled, so what is actually tested in CI @aidenfoxivey @ryjones ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is a good catch - it should probably be modified to fix this. I don't have access to the repository to make a change right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aidenfoxivey propose a change in a review comment

Copy link
Member

Choose a reason for hiding this comment

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

I don't have access to the repository to make a change right now.

Why can't you do a proper PR from your own repo @aidenfoxivey ? Proceeding with this PR would make @ryjones the author of record -- which simply is wrong on several counts.

If the problem is that no reviewer/committer presses the "execute CI" button on a PR, well, then this project has some serious problem of getting enough attention...

@aidenfoxivey if in turn you have problems with the contribution process (where actually the way how to run CI also locally is described) please let us know as part of your PR: We want to improve our documentation such that new contributors can easily, well, contribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baentsch the issue was that the PR from his repo cannot execute all of the CI due to no access to secrets.

Copy link
Member

Choose a reason for hiding this comment

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

@ryjones I see at least two other solutions to this problem: 1) improve the CI process such as to remove this limitation: anyone should be able to do a PR and see most-if-not-all CI run (to then fix him/herself); 2) execute CI locally (as is documented).

Working around problem 1) by taking ownership of a PR is kind but neither scalable (you cannot handle by manually pasting in review comments by dozens of contributors having the same problem) nor "right" (e.g., code authorship attribution should be easy to infer both from a legal and ethical perspective).

So, would it be OK for you to make a proposal to improve 1) @ryjones as this seems to be related to admin aspects? I (or maybe @SWilson4?) could work with @aidenfoxivey to make sure this contribution "gets in" with proper attribution. OK for everyone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baentsch It isn't obvious, but I did do non-zero work on this PR.

As far as token availability, this is a limitation (and one I agree with) of the GitHub platform. The way to "work around it" is to set up guardrails around jobs (like triggering CircleCI) that require the job to be within the repo, not a forked repo. I planned to do this later in a follow-on PR for multiple jobs in the org, but that is an early next year thing, not a next week thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding next steps, I'm fine to make another pull request and move my changes there. In addition, I'm absolutely fine to just have this merged as is. I'll defer to others on the best approach for this.

Copy link
Member

Choose a reason for hiding this comment

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

It isn't obvious, but I did do non-zero work on this PR.

Thanks for letting me know, @ryjones . I indeed didn't see this. So who would you see as the person that should be primarily attributed with this code (and how to properly attribute the other)?

My proposal would be to do it "as usual", i.e., ask @aidenfoxivey to do a PR from his/her repo and you propose your contributions during the PR: That way, both authors are recorded and we see CI run where possible (and trigger the rest). @aidenfoxivey, it would be nice if you'd locally execute the new test via CI (also checking that it actually runs :) before the PR. If you encounter problems in following the documented guidance on that, please let us know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baentsch : @aidenfoxivey did most of the work, I did some of the work. I'm fine with having my attribution removed as Aiden would have made it there eventually.

I agree with having @aidenfoxivey create a new PR. As it's been approved twice, I think it should be merged as-is and allow Aiden to iterate in follow-up PRs.


OQS_STATUS rc;

uint8_t public_key[OQS_SIG_dilithium_2_length_public_key];
uint8_t secret_key[OQS_SIG_dilithium_2_length_secret_key];
uint8_t message[MESSAGE_LEN];
uint8_t signature[OQS_SIG_dilithium_2_length_signature];
size_t message_len = MESSAGE_LEN;
size_t signature_len;

// let's create a random test message to sign
OQS_randombytes(message, message_len);

rc = OQS_SIG_dilithium_2_keypair(public_key, secret_key);
if (rc != OQS_SUCCESS) {
std::cerr << "ERROR: OQS_SIG_dilithium_2_keypair failed!" << std::endl;
cleanup_stack(secret_key, OQS_SIG_dilithium_2_length_secret_key);
return OQS_ERROR;
}
rc = OQS_SIG_dilithium_2_sign(signature, &signature_len, message, message_len, secret_key);
if (rc != OQS_SUCCESS) {
std::cerr << "ERROR: OQS_SIG_dilithium_2_sign failed!" << std::endl;
cleanup_stack(secret_key, OQS_SIG_dilithium_2_length_secret_key);
return OQS_ERROR;
}
rc = OQS_SIG_dilithium_2_verify(message, message_len, signature, signature_len, public_key);
if (rc != OQS_SUCCESS) {
std::cerr << "ERROR: OQS_SIG_dilithium_2_verify failed!" << std::endl;
cleanup_stack(secret_key, OQS_SIG_dilithium_2_length_secret_key);
return OQS_ERROR;
}

std::cout << "[example_stack] OQS_SIG_dilithium_2 operations completed" << std::endl;
cleanup_stack(secret_key, OQS_SIG_dilithium_2_length_secret_key);
return OQS_SUCCESS; // success!

#else

std::cout << "[example_stack] OQS_SIG_dilithium_2 was not enabled at compile-time" << std::endl;
return OQS_SUCCESS;

#endif
}

/* This function gives an example of the signing operations,
* allocating variables dynamically on the heap and calling the generic
* OQS_SIG object.
*
* This does not require the use of compile-time macros to check if the
* algorithm in question was enabled at compile-time; instead, the caller
* must check that the OQS_SIG object returned is not nullptr.
*/
static OQS_STATUS example_heap(void) {

#ifdef OQS_ENABLE_SIG_dilithium_2

size_t message_len = MESSAGE_LEN;
size_t signature_len;
OQS_STATUS rc;

std::unique_ptr<OQS_SIG, OQSSigDeleter> sig(OQS_SIG_new((OQS_SIG_alg_dilithium_2)));
if (sig == nullptr) {
throw std::runtime_error("[example_heap] OQS_SIG_alg_dilithium_2 was not enabled at compile-time.");
}
std::unique_ptr<uint8_t[], OQSInsecureDeleter> public_key(static_cast<uint8_t*>(malloc(sig->length_public_key)));
std::unique_ptr<uint8_t[], OQSSecureDeleter> secret_key(static_cast<uint8_t*>(malloc(sig->length_secret_key)), OQSSecureDeleter(sig->length_secret_key));
std::unique_ptr<uint8_t[], OQSInsecureDeleter> message(static_cast<uint8_t*>(malloc(message_len)));
std::unique_ptr<uint8_t[], OQSInsecureDeleter> signature(static_cast<uint8_t*>(malloc(sig->length_signature)));
if ((public_key == nullptr) || (secret_key == nullptr) || (message == nullptr) || (signature == nullptr)) {
throw std::runtime_error("ERROR: malloc failed!");
}

// let's create a random test message to sign
OQS_randombytes(message.get(), message_len);

rc = OQS_SIG_keypair(sig.get(), public_key.get(), secret_key.get());
if (rc != OQS_SUCCESS) {
throw std::runtime_error("ERROR: OQS_SIG_keypair failed!");
}
rc = OQS_SIG_sign(sig.get(), signature.get(), &signature_len, message.get(), message_len, secret_key.get());
if (rc != OQS_SUCCESS) {
throw std::runtime_error("ERROR: OQS_SIG_sign failed!");
}
rc = OQS_SIG_verify(sig.get(), message.get(), message_len, signature.get(), signature_len, public_key.get());
if (rc != OQS_SUCCESS) {
throw std::runtime_error("ERROR: OQS_SIG_verify failed!");
}

std::cout << "[example_heap] OQS_SIG_dilithium_2 operations completed." << std::endl;
return OQS_SUCCESS; // success
#else

std::cout << "[example_heap] OQS_SIG_dilithium_2 was not enabled at compile-time." << std::endl;
return OQS_SUCCESS;

#endif
}

int main() {
OQS_init();
try {
example_stack();
example_heap();
}
catch (std::exception e) {
std::cerr << e.what() << std::endl;
OQS_destroy();
return EXIT_FAILURE;
}
OQS_destroy();
return EXIT_SUCCESS;
}

void cleanup_stack(uint8_t *secret_key, size_t secret_key_len) {
OQS_MEM_cleanse(secret_key, secret_key_len);
}
Loading