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

Enterprise_mode_+_wifi_configuraiton_api: update ODIN drivers to v3.7.0 RC1 #10454

Merged
merged 1 commit into from May 20, 2019

Conversation

Projects
None yet
10 participants
@aqib-ublox
Copy link
Contributor

commented Apr 23, 2019

Description

This pull requests contains

  • Enterprise mode supporting EAP_TLS and PEAP methods for enterprise security.

  • A new private API for wifi connect() as enterprise mode require certificates such as CA server and client certificate along with private key in addition to credentials for connection.

  • Wifi configuration API.

This release also contains following fixes

Tests Results

iar_mbed_os_log.txt
arm_mbed_os_logs.txt
gcc_arm_mbed_os_log.txt
arm_driver_log.txt
iar_driver_logs.txt
gcc_arm_driver_logs.txt
iar_ble_gatt_server.txt
gcc_arm_ble_gatt_server.txt
arm_ble_gatt_server.txt

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

A private API is provided for ODIN_W2 target against enterprise mode.

    nsapi_error_t connect(
        const char          *ssid,
        const char          *pass,
        nsapi_security_t  security,
        auth_cert_s         *cert_handle,
        const char          *username = NULL,
        const char          *user_pswd = NULL,
        uint8_t               channel = 0);

Application is required to pass certificate(CA cert or client cert) and key (private key) in PEM format currently. User is required to pass those certificates through cert_handle auth_cert_s and an appropriate security should be selected either NSAPI_SECURITY_EAP_TLS or NSAPI_SECURITY_PEAP.

For example:

static auth_cert_s   certificates;
_wifi = new OdinWiFiInterface(true);
#ifdef EAP_TLS_TESTING
    certificates.client_cert = &cert_data[0];
    certificates.client_prvt_key = &cert_data_key[0];
    certificates.ca_cert = NULL;
#elif defined(PEAP_TESTING)
    certificates.client_cert = NULL;
    certificates.client_prvt_key = NULL;
    certificates.ca_cert = &ca_cert_data[0];
#endif
_wifi->connect(ssid, pass, security, &certificates, _peap_username, _peap_user_pass, channel);

NOTE:
security could be NSAPI_SECURITY_EAP_TLS or NSAPI_SECURITY_PEAP

Configuration API

    virtual unsigned int get_config(void *setting);
    virtual void set_config(void *setting, cb_uint32 value);

@aqib-ublox aqib-ublox force-pushed the u-blox:ublox_odin_driver_os_5_v3.7.0_rc1 branch from 0dda8c4 to c98c35e Apr 23, 2019

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@MarceloSalazar can u ask ur team to start review as we need to target it in coming release i-e 12.2 most likely

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 25, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@michalpasztamobica could you re-review please ?

@michalpasztamobica
Copy link
Contributor

left a comment

I still think NSAPI_ERROR_CERT_SIZE is not necessary and NSAPI_ERROR_PARAMETER should be used instead.
@SeppoTakalo , what do you think?

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@SeppoTakalo what u suggest?

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@SeppoTakalo what u suggest?

Use NSAPI_ERROR_PARAMETER so you are then not causing any changes to public Mbed OS APIs.

@aqib-ublox aqib-ublox force-pushed the u-blox:ublox_odin_driver_os_5_v3.7.0_rc1 branch from c98c35e to 3eb8248 Apr 29, 2019

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@SeppoTakalo what u suggest?

Use NSAPI_ERROR_PARAMETER so you are then not causing any changes to public Mbed OS APIs.

it's removed and replaced with NSAPI_ERROR_PARAMETER

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@MarceloSalazar waiting for review to be finalised?

@MarceloSalazar

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@0xc0170
Copy link
Member

left a comment

The main concern is about tls config change, requested help from tls team.

Show resolved Hide resolved features/mbedtls/inc/mbedtls/config.h Outdated
@@ -0,0 +1,52 @@
/* ODIN-W2 implementation of WiFi Config Interface
* Copyright (c) 2016 u-blox Malmö AB

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 30, 2019

Member

year should be 2019

Add please SPDX identifier as well

This comment has been minimized.

Copy link
@aqib-ublox

aqib-ublox May 2, 2019

Author Contributor

also here, SPDX identifier

already there LICENSE-2.0 ?

This comment has been minimized.

Copy link
@0xc0170

0xc0170 May 2, 2019

Member

SPDX-License-Identifier: Apache-2.0 is needed, as in other files

@@ -0,0 +1,117 @@
/* ODIN-W2 user Config Interface
* Copyright (c) 2016 u-blox Malmö AB
*

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 30, 2019

Member

also here, SPDX identifier

This comment has been minimized.

Copy link
@aqib-ublox

aqib-ublox May 2, 2019

Author Contributor

@0xc0170 corrected

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Functionality change

Only in targets code - can go to the patch release ? I see above note about 5.12.2 .

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 30, 2019

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Functionality change

Only in targets code - can go to the patch release ? I see above note about 5.12.2 .

@0xc0170 couldn't get it? 5.12.2 is released so might be target in next patch release or u prefer it to be part of full release? in that case as per @MarceloSalazar code freeze for that is 30 May and that's too far. Please correct me if i am wrong

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Edited: this is extending security type, therefore it was as "functionality change". In this case, 5.13 would be appropriate.

@RonEld If you can help how to resolve that mbed config - it is the main outstanding one in this PR.

@aqib-ublox aqib-ublox force-pushed the u-blox:ublox_odin_driver_os_5_v3.7.0_rc1 branch from 3eb8248 to 3c1a2c2 May 2, 2019

@0xc0170
Copy link
Member

left a comment

Waiting for mbedtls config change

@aqib-ublox aqib-ublox force-pushed the u-blox:ublox_odin_driver_os_5_v3.7.0_rc1 branch from 3c1a2c2 to 0282f1e May 3, 2019

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@0xc0170 and @RonEld can u please acknowledge recent changset

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@0xc0170, @RonEld any update?

@RonEld

RonEld approved these changes May 7, 2019

Copy link
Contributor

left a comment

LGTM
The configuration for MBEDTLS_MPI_WINDOW_SIZE should be documented though, in case your users would like to change it in their user defined configuration.

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@MarceloSalazar

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@ARMmbed/mbed-os-maintainers please review and comment.
If possible, we'd like this to be released in the next patch release.

@k-stachowiak
Copy link
Contributor

left a comment

I left a question regarding a curious construct; otherwise I did not find any Mbed TLS related issues.

case ODIN_CFG_SET_FT_MODE: return cbTARGET_CFG_SET_FT_MODE;
case ODIN_CFG_GET_FT_MODE: return cbTARGET_CFG_GET_FT_MODE;
default:
MBED_ASSERT(true);

This comment has been minimized.

Copy link
@k-stachowiak

k-stachowiak May 8, 2019

Contributor

Is this supposed to be asserting true? It would seem more natural to assert false upon encountering an unrecognized identifier.

Edit: there is one more such case.

This comment has been minimized.

Copy link
@aqib-ublox

aqib-ublox May 8, 2019

Author Contributor

Yes that's right @AmmadRehmat please see this

This comment has been minimized.

Copy link
@aqib-ublox

aqib-ublox May 8, 2019

Author Contributor

@k-stachowiak it's corrected thanks for pointing out

@aqib-ublox aqib-ublox force-pushed the u-blox:ublox_odin_driver_os_5_v3.7.0_rc1 branch from 0282f1e to 43759c0 May 8, 2019

case ODIN_CFG_SET_FT_MODE: return cbTARGET_CFG_SET_FT_MODE;
case ODIN_CFG_GET_FT_MODE: return cbTARGET_CFG_GET_FT_MODE;
default:
MBED_ASSERT(false);

This comment has been minimized.

Copy link
@aqib-ublox

aqib-ublox May 8, 2019

Author Contributor

@k-stachowiak corrected

case ODIN_POWER_SAVE_MODE_DEEP_SLEEP: return cbTARGET_POWER_SAVE_MODE_DEEP_SLEEP;
default:
MBED_ASSERT(false);
}

This comment has been minimized.

Copy link
@aqib-ublox

aqib-ublox May 8, 2019

Author Contributor

@k-stachowiak corrected

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@0xc0170 any update?

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@aqib-ublox

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@MarceloSalazar whats status?

@adbridge adbridge added needs: CI and removed needs: work labels May 17, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@0xc0170 please confirm you are happy with the licence updates. Running the CI in the meantime

@mbed-ci

This comment has been minimized.

Copy link

commented May 17, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 20, 2019

@0xc0170 0xc0170 merged commit d4122b0 into ARMmbed:master May 20, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8556 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.