-
Notifications
You must be signed in to change notification settings - Fork 7.3k
wifi: enterprise: Add support for runtime certificates #87656
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
Conversation
37adaa1
to
c167779
Compare
@MaochenWang1 I have not done any AP mode testing on this as nRF70 doesn't support AP + Enterprise mode, it would e good if NXP can do some tests. |
Sure, will review, test and feedback |
FYI, I am seeing issues even with build time certs, debugging now will push fixes soon. |
c167779
to
ee1ba90
Compare
I have resolved the compliance except for below: I tried to get checkpatch ignore using
|
ee1ba90
to
52257b7
Compare
Tested end-end, it is working fine. |
52257b7
to
07de5fe
Compare
07de5fe
to
ee8a511
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build system changes OK
To facilitate installation of the certificates, a helper script is provided in the ``samples/net/wifi/test_certs`` directory. The script can be used to install the certificates at runtime. | ||
|
||
.. code-block:: bash | ||
|
||
$ west build -p -b <board> samples/net/wifi -- -DEXTRA_CONF_FILE=overlay-enterprise-variable-bufs.conf | ||
$ samples/net/wifi/test_certs/install_certs.py -p samples/net/wifi/test_certs/rsa2k | ||
|
||
The script will install the certificates in the ``rsa2k`` directory to the TLS credentials store in the device over UART and using TLS credentials shell commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so all devices are supporting the AT commands this tool seems to be using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't use AT command, but TLS credentials shell (part of Zephyr)
Compile time certificates | ||
------------------------- | ||
|
||
Test certificates in PEM format are committed to the repo at :zephyr_file:`samples/net/wifi/test_certs` and the during the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really not a fan of the proliferation of binary certificates in-tree. Can't we just have instructions on what steps people sjhould take to generate them? It seems to me as this would be much more useful, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but for a quick testing it's useful to have a golden certs
that just work. Else any mistakes in cert generation are tough to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need a tested set of ready made certificates in the samples, otherwise it is difficult to verify things automated way. We certainly can/should have also instructions how the user can generate them too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We certainly can/should have also instructions how the user can generate them too.
I have already added a link to the script that I had used to generate these certs in the wifi docs: (https://docs.zephyrproject.org/latest/connectivity/networking/api/wifi.html#wi-fi-enterprise-test-x-509-certificate-header-generation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but for a quick testing it's useful to have a
golden certs
that just work. Else any mistakes in cert generation are tough to debug.
Sorry but I still don't get why it's not possible to generate the certificates on the fly? Should be a few dozen lines of code using Python's cryptography
package, I think? And this would have the merit to actually provide more guidance to the end user.
Also, where are these certificates coming from and are we even allowed to redistribute them (I wouldn't be surprised if they are part of some kind of Wi-Fi Alliance certification suite and not meant to be accessed by non-members ...)?
$ openssl x509 -in client.pem -text -noout
Certificate:
Data:
Version: 3 (0x2)
Serial Number:
97:d4:07:ec:a6:05:15:13
Signature Algorithm: sha384WithRSAEncryption
Issuer: C=US, L=Santa Clara, O=Wi-Fi Alliance, CN=Suite B RSA 3k Root CA
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do they appear to be issued by Wi-Fi Alliance then?
Sorry, I am missing something. Where is Wi-Fi alliance coming from? The issuer is
Example certificate authority
, no?
Not for the rsa3k certs, no, and I have concerns these can't be redistributed outside of Wi-Fi Alliance certification program. This does need to be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, I understand the confusion now, I was looking the ones that I have added in this PR. The WFA ones are existing ones already in main
I have just moved them to rsa3k
directory, they are submitted by NXP. + @MaochenWang1 @fengming-ye can you please respond to Ben's question about redistributing those certs in Zephyr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kartben the Wi-Fi alliance certs are already in main and this PR is just moving them around. I suggest that we deal the cert distribution issue and possible removal from zephyr main in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jukka could you please create a tracking issue for this redistribution license problem, so we ensure that this is resolved ASAP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a tracking issue for this redistribution license problem, so we ensure that this is resolved ASAP?
Sure, it is here #88771
@kartben please review my responses, thanks. |
3d2d415
37878ea
to
7cf172b
Compare
Using TLS credentials library add support for run-time certificates where the installed certs are retrieved from the credential store (as of now only volatile backend is tested). This helps in production environments. Implements zephyrproject-rtos#79564. Signed-off-by: Chaitanya Tata <[email protected]>
The volatile backend stores the credentials on the heap, so, explicitly add a config option that can be overridden in case there are more certs than the default. Signed-off-by: Chaitanya Tata <[email protected]>
Instead of having an overlay move the Enterprise configurations to a dedicated snippet so that it can be enabled with any sample. Can be used along with Wi-Fi snippet e.g., `-S "wifi-ipv4;wifi-enterprise"`. Signed-off-by: Chaitanya Tata <[email protected]>
Enable TLS credentials shell to manager Wi-Fi enterprise certs. Signed-off-by: Chaitanya Tata <[email protected]>
Deletion of credential should use the pointer from the reference slot not the temporary buffer, this causes a crash (unknown error). Signed-off-by: Chaitanya Tata <[email protected]>
Certificates usage depends on STA/AP mode, but we don't have that information at a build time, so, make all certs as optional and if a file isn't found then generate an empty header so that corresponding C code will be built. Any missing mandatory certificates will be validated before connection and connection is failed. Signed-off-by: Chaitanya Tata <[email protected]>
RSA3K based certs are not supported on all platforms, so, keep both variants, rsa2k (the older certs but with longer expiry 9999 days) and rsa3k (latest ones) and we can have more variants in this folders. Also, add a cmake variable to override the path with default as rsa3k. Signed-off-by: Chaitanya Tata <[email protected]>
The command should work with existing certs rather than a generic example, also fix the key-management. Signed-off-by: Chaitanya Tata <[email protected]>
This is needed to ensure run-time certs feature builds. Signed-off-by: Chaitanya Tata <[email protected]>
7cf172b
to
9d1e159
Compare
Dismissing my request for changes since apparently there is pressure for being able to merge this "downstream" and I am obviously getting in the way.
the Wi-Fi alliance certs are already in main and this PR is just moving them around
This is potentially a serious issue, so I am not sure I understand the "let's pretend it's ok (and it might very well be, don't get me wrong) and let's just deal with it later".
Implements #79564
The certificate installation is handled by a helper script which simplifies installation process, but even without that PR
cred
shell can be used manually but it's cumbersome.