Skip to content

Conversation

@yourslab
Copy link
Contributor

@yourslab yourslab commented Aug 13, 2020

Once the certs are downloaded, they are copied from demos/certificates into build/bin/certificates. Integration tests also need these certificates, so they are copied to build/bin/tests/certificates whenever tests are built.

You can also disable the downloading of certificates by passing the CMake flag: -DDOWNLOAD_CERTS=0

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2020

Codecov Report

Merging #1126 into development will increase coverage by 2.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1126      +/-   ##
===============================================
+ Coverage        96.54%   98.85%   +2.30%     
===============================================
  Files                9        4       -5     
  Lines             5643     1309    -4334     
  Branches           641      393     -248     
===============================================
- Hits              5448     1294    -4154     
+ Misses               9        0       -9     
+ Partials           186       15     -171     
Impacted Files Coverage Δ
libraries/standard/mqtt/src/mqtt.c 99.48% <ø> (+5.06%) ⬆️
libraries/standard/mqtt/src/mqtt_lightweight.c 97.42% <ø> (+2.90%) ⬆️
libraries/standard/mqtt/src/mqtt_state.c 98.46% <ø> (+2.52%) ⬆️
libraries/standard/http/src/http_client.c 100.00% <100.00%> (+12.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9fc2ea...e3fbbe0. Read the comment docs.

muneebahmed10
muneebahmed10 previously approved these changes Aug 13, 2020
muneebahmed10
muneebahmed10 previously approved these changes Aug 13, 2020
aggarw13
aggarw13 previously approved these changes Aug 14, 2020
Copy link

@aggarw13 aggarw13 left a comment

Choose a reason for hiding this comment

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

My suggestion about setting the absolute path for the demo config could be considered for a separate PR.

if(ROOT_CA_CERT_PATH)
target_compile_definitions(
${DEMO_NAME} PRIVATE
ROOT_CA_CERT_PATH="${ROOT_CA_CERT_PATH}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the file path is passed as a cmake flag, will it still work properly from any directory if the given flag is a relative path and not an absolute path? If not, is it possible to parse this flag using get_filename_component and convert it into an absolute path first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

aggarg
aggarg previously approved these changes Aug 14, 2020
@yourslab yourslab merged commit 1e0fb37 into aws:development Aug 14, 2020

# Set prefix to PWD if any path flags are relative
if(DEFINED ENV{PWD})
if(NOT ROOT_CA_CERT_PATH MATCHES "/$")

Choose a reason for hiding this comment

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

What if these variables are not passed in the CMake command in which case they would (probably) have empty values?
Would these generate invalid variables for the credential variables in that case?

Copy link

@aggarw13 aggarw13 Aug 14, 2020

Choose a reason for hiding this comment

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

I tested the mqtt_demo_basic_tls locally with not providing the ROOT_CA_CERT_PATH with the following command:
cmake .. -DBUILD_TESTS="ON" -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS='-Wextra -Wall -O0 -ggdb'

The build logs show that the absolute path logic is causing the ROOT_CA_CERT_PATH CMake variable to be defined even though it wasn't passed by me, and thus, it is building the demo target with an incorrect value (of /home/ubuntu/Repos/aws-iot-device-sdk-embedded-C/build/ as can be seen in the logs)

╭─ubuntu@ip-172-31-24-209 ~/Repos/aws-iot-device-sdk-embedded-C/build  ‹development*› 
╰─➤  ./bin/mqtt_demo_basic_tls         
[INFO] [DEMO] [mqtt_demo_basic_tls.c:400] Establishing a TLS session to test.mosquitto.org:8883.
[DEBUG] [Sockets] [sockets_posix.c:170] Performing DNS lookup: Host=test.mosquitto.org.
[DEBUG] [Sockets] [sockets_posix.c:210] Attempting to connect to server: Host=test.mosquitto.org, IP address=5.196.95.208.
[DEBUG] [Sockets] [sockets_posix.c:225] Connected to IP address: 5.196.95.208.
[DEBUG] [Sockets] [sockets_posix.c:256] Established TCP connection: Server=test.mosquitto.org.

[DEBUG] [Transport_OpenSSL_Sockets] [openssl_posix.c:144] Attempting to open Root CA certificate: Path=/home/ubuntu/Repos/aws-iot-device-sdk-embedded-C/build/.
[ERROR] [Transport_OpenSSL_Sockets] [openssl_posix.c:195] PEM_read_X509 failed to parse root CA.
[ERROR] [Transport_OpenSSL_Sockets] [openssl_posix.c:474] Setting up credentials failed.

Choose a reason for hiding this comment

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

The fix is simple by just updating each of the nested if conditions to check if their respective CMake variables are defined

yourslab added a commit to yourslab/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 17, 2020
…loading (aws#1126)

* Add option to turn off downloading of certificates and create demos/certificates directory

* Remove Mosquitto certificate from list of certs to download

* Address PR comments

* Add CMake command line options to use for configuring demos

* Update README.md to contain extra flag for ROOT_CA_CERT_PATH

* Set prefix to PWD if any path flags are relative
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 27, 2020
…loading (aws#1126)

* Add option to turn off downloading of certificates and create demos/certificates directory

* Remove Mosquitto certificate from list of certs to download

* Address PR comments

* Add CMake command line options to use for configuring demos

* Update README.md to contain extra flag for ROOT_CA_CERT_PATH

* Set prefix to PWD if any path flags are relative
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 28, 2020
…loading (aws#1126)

* Add option to turn off downloading of certificates and create demos/certificates directory

* Remove Mosquitto certificate from list of certs to download

* Address PR comments

* Add CMake command line options to use for configuring demos

* Update README.md to contain extra flag for ROOT_CA_CERT_PATH

* Set prefix to PWD if any path flags are relative
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 31, 2020
…loading (aws#1126)

* Add option to turn off downloading of certificates and create demos/certificates directory

* Remove Mosquitto certificate from list of certs to download

* Address PR comments

* Add CMake command line options to use for configuring demos

* Update README.md to contain extra flag for ROOT_CA_CERT_PATH

* Set prefix to PWD if any path flags are relative
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Sep 1, 2020
…loading (aws#1126)

* Add option to turn off downloading of certificates and create demos/certificates directory

* Remove Mosquitto certificate from list of certs to download

* Address PR comments

* Add CMake command line options to use for configuring demos

* Update README.md to contain extra flag for ROOT_CA_CERT_PATH

* Set prefix to PWD if any path flags are relative
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