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

LoRaWAN LoRaMacCrypto.cpp Asserts Incorrect #6764

Closed
mattbrown015 opened this issue Apr 27, 2018 · 9 comments
Closed

LoRaWAN LoRaMacCrypto.cpp Asserts Incorrect #6764

mattbrown015 opened this issue Apr 27, 2018 · 9 comments

Comments

@mattbrown015
Copy link
Contributor

Description

  • Type: Bug
  • Priority: Minor

'features\lorawan\lorastack\mac\LoRaMacCrypto.cpp' contains some asserts that should fire if mbedtls is incorrectly configured e.g.:

MBED_ASSERT("[LoRaCrypto] Must enable AES, CMAC & CIPHER from mbedTLS");

I think these should actually be something like:

MBED_ASSERT(0 && "[LoRaCrypto] Must enable AES, CMAC & CIPHER from mbedTLS");

I made a mistake with my configuration and I think I would have diagnosed the problem faster if the asserts had fired.


Bug

mbed-os sha:
c8d72c5 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #6693 from kjbracey-arm/equeue-ticks

Expected behavior

Assert with incorrect mbedtls configuration.

Actual behavior

Returns failure code.

Steps to reproduce

Disable mbedtls while using the LoRaWAN stack.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 27, 2018

@AnttiKauppila @hasnainvirk Please review

@AnttiKauppila
Copy link

Arm internal ref: IOTCELL-841

@kivaisan
Copy link
Contributor

kivaisan commented May 2, 2018

Good finding. We'll fix this. I think MBED_STATIC_ASSERT is even better to detect the issue already during the compilation.

@mattbrown015
Copy link
Contributor Author

If you are going to make it a compile time failure you could get rid of all the stub functions and add an #error directive.

The implication of these stubs and comments in the code is that there could be a use-case for LoRaWAN without mbedTLS but I can't think what that use-case could be.

@kivaisan
Copy link
Contributor

kivaisan commented May 3, 2018

I think MBED_STATIC_ASSERT is even better to detect the issue already during the compilation.

This is not possible after all. We must be able to build mbed-os even when lora/mbedlts is not used.

@hasnainvirk
Copy link
Contributor

@mattbrown015 Are you happy with the PR relating to your issue ? It will be made available in next minor release ASAP.

@mattbrown015
Copy link
Contributor Author

I'm happy if you're happy! :-)

The changes in LoRaMacCrypto.cpp look like what I was expecting and the LoRaWAN stack still works with MbedTLS included. But, I haven't investigated what happens in the various problem cases i.e. MbedTLS incorrectly configured.

@mattbrown015
Copy link
Contributor Author

Do you want me to close this?

Is it my job to close it as I opened it?

@kivaisan
Copy link
Contributor

@mattbrown015 yes, please close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants