Skip to content

getting-started: Align with upstream getting started snippets, comment about pedagogical code, and reduce stack usage #49

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
Merged
Patater merged 2 commits into from
Nov 8, 2019
Merged

getting-started: Align with upstream getting started snippets, comment about pedagogical code, and reduce stack usage #49

Patater merged 2 commits into from
Nov 8, 2019

Conversation

@Patater
Copy link
Contributor

@Patater Patater commented Nov 7, 2019 •
edited
Loading

  • Improve the example by commenting where global keys are used for strictly pedagogical purposes.
  • Align code snippets with changes in Mbed Crypto's getting started guide, to ensure we keep testing our getting started guide (as this example is the only time those snippets are tested, currently)
  • Reduce stack usage by using global static const keys, instead of #defines for initializer lists to place keys on the stack.

@Patater Patater added the bug Something isn't working label Nov 7, 2019
@Patater
Copy link
Contributor Author

Patater commented Nov 7, 2019

Tested with ARMC6 and the develop profile. IAR not yet tested.

gilles-peskine-arm
gilles-peskine-arm suggested changes Nov 7, 2019
View reviewed changes
Copy link

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Ok for the workaround, but this should be documented as a workaround in the code itself, especially since this is in example code.

getting-started/main.cpp Outdated
psa_status_t status;
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
uint8_t key[] = RSA_KEY;
static uint8_t key[] = RSA_KEY;
Copy link

@gilles-peskine-arm gilles-peskine-arm Nov 7, 2019

Choose a reason for hiding this comment

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

Having a static variable is unusual, and usually done when the value needs to be preserved between calls, which is not the case here. Please add a comment in the code to indicate why these two variables are static.

getting-started/main.cpp Outdated
psa_status_t status;
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
uint8_t key[] = RSA_KEY;
static uint8_t key[] = RSA_KEY;
Copy link

@gilles-peskine-arm gilles-peskine-arm Nov 7, 2019

Choose a reason for hiding this comment

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

Preexisting, but would be nice to do what you're at it: key should be const.

@Patater
Copy link
Contributor Author

Patater commented Nov 7, 2019

This is not meant as any sort of workaround. This is meant to reduce stack usage to make the example more portable. We could use a global key instead if use of static in this situation appears uncouth.

@Patater
Copy link
Contributor Author

Patater commented Nov 7, 2019

New patchset uses static const for key and adds a comment explaining why the writable signature is static, as that might be a bit surprising. static const is idiomatic enough that it doesn't need a comment.

Patater
Patater commented Nov 7, 2019
View reviewed changes
getting-started/main.cpp Outdated
uint8_t hash[] = "INPUT_FOR_SIGN";
uint8_t signature[PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE] = {0};
/* 'signature' is static to reduce stack usage. */
static uint8_t signature[PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE] = {0};
Copy link
Contributor Author

@Patater Patater Nov 7, 2019

Choose a reason for hiding this comment

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

Making signature static is not necessary for the targets we run this example on so far. So, if we dislike adding static here, we can remove it as we don't need it yet. Just making the key static const is good enough to pass the tests with ARMC6-develop at least.

@Patater
Copy link
Contributor Author

Patater commented Nov 7, 2019

Buddy PR at ARMmbed/mbed-crypto#317

dgreen-arm
dgreen-arm approved these changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

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

With these changes, the example works with latest Mbed OS using ARMC6 or IAR

@Patater
getting-started: Explain why we have global keys
e9cb761 
Make it more clear that one should not have global, hard-coded keys in
their code. We do this for example purposes only and real-world
applications should follow secure practices.
@Patater
getting-started: Align with getting_started.md snippets
e46b6ad 
The getting started guide in Mbed Crypto was recently changed to make it
more obvious that `AES_KEY` and `RSA_KEY` were shorthand for key
material. The guide did this by using function parameters instead of
all-caps shorthand. In order to better match what is in our getting
started guide with this getting started example, we make the same
function parameter changes here, passing global key data in via
`main()`.

Passing global key data in via `main()` has the additional benefit of
reducing stack usage of each example snippet function. This enables the
example to run on more boards, those with more limited stack space.
@Patater Patater changed the title getting-started: Reduce RSA sign stack usage getting-started: Align with upstream getting started snippets, comment about pedogogical code, and reduce stack usage Nov 8, 2019
@Patater Patater changed the title getting-started: Align with upstream getting started snippets, comment about pedogogical code, and reduce stack usage getting-started: Align with upstream getting started snippets, comment about pedagogical code, and reduce stack usage Nov 8, 2019
@Patater
Copy link
Contributor Author

Patater commented Nov 8, 2019

New patchset makes the keys global static const instead to reduce RAM usage, while also improving the readability of the example.

dgreen-arm
dgreen-arm approved these changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

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

Example runs successfully on ARMC6 and IAR, LGTM

@gilles-peskine-arm
Copy link

gilles-peskine-arm commented Nov 8, 2019

e46b6ad passed CI with ARM Compiler reinstated in https://jenkins-internal.mbed.com/job/mbed-crypto-release-ci-testing/577 .

@Patater Patater merged commit a3362c9 into ARMmbed:master Nov 8, 2019
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Nov 8, 2019
Merged
@Patater Patater mentioned this pull request Nov 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@dgreen-arm dgreen-arm dgreen-arm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Patater @gilles-peskine-arm @dgreen-arm