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

net: Add credentials shell #64343

Merged
merged 3 commits into from Nov 14, 2023

Conversation

glarsennordic
Copy link
Contributor

These commits introduce a TLS credentials shell allowing credentials to be installed and managed from any shell backend

@jukkar jukkar requested review from d3zd3z and ceolin October 25, 2023 07:35
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

There are quite a few compliance reports, please look them over.

subsys/net/lib/tls_credentials/tls_internal.h Outdated Show resolved Hide resolved
subsys/net/lib/tls_credentials/Kconfig.shell Outdated Show resolved Hide resolved
doc/connectivity/networking/api/tls_credentials_shell.rst Outdated Show resolved Hide resolved
subsys/net/lib/tls_credentials/tls_credentials_shell.c Outdated Show resolved Hide resolved
subsys/net/lib/tls_credentials/tls_credentials_shell.c Outdated Show resolved Hide resolved
subsys/net/lib/tls_credentials/tls_credentials_shell.c Outdated Show resolved Hide resolved
subsys/net/lib/tls_credentials/tls_credentials_shell.c Outdated Show resolved Hide resolved
subsys/net/lib/tls_credentials/tls_credentials_shell.c Outdated Show resolved Hide resolved
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I wrote comment yesterday, but messed something up and GH did not publish them...

@glarsennordic
Copy link
Contributor Author

TBH that's a way better place to cross-link anyways, will do.

@glarsennordic
Copy link
Contributor Author

Did the docs build time out? Gonna try another force push

@glarsennordic glarsennordic force-pushed the cred_shell branch 2 times, most recently from 0397156 to 6d92d71 Compare November 2, 2023 00:52
rlubos
rlubos previously approved these changes Nov 6, 2023
Adds an internal credential_digest for generating a string digest of
credentials.

Such digests would allow users of a prospective TLS credentials shell to
verify the contents of a given credential without directly accessing
those contents.

Offloading the digest process to the underlying backend allows backends
for which private portions are not directly accessible to be eventually
supported.

Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>
Add (internal) support for sectag iterating.

Also officially marks negative sectag values as reserved for internal
use.

This will allow a prospective TLS credentials shell to iterate over all
available credentials.

Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>
@rlubos
Copy link
Contributor

rlubos commented Nov 10, 2023

@glarsennordic Please check the compliance, it reported some nits.

Adds a shell interface for TLS Credentials, allowing management of
credentials via the Zephyr shell

Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>
@fabiobaltieri fabiobaltieri merged commit 9f093ab into zephyrproject-rtos:main Nov 14, 2023
19 checks passed
@kartben
Copy link
Collaborator

kartben commented Nov 14, 2023

A late comment: @glarsennordic you may want to configure your text editor for enforcing the max 100 char line limit for .rst content, as the added page definitely fell through the cracks. Thanks!

@glarsennordic
Copy link
Contributor Author

glarsennordic commented Nov 14, 2023

@kartben Sorry, it is standard practice where I work for .rst files to exceed 100 chars, and for each sentence to exist on a single line, and for each line to have only a single sentence. I assumed that was the same with Zephyr, is that not the case?

I notice there is inconsistency on this within the Zephyr docs. Some .rst files appear to use multi-line sentences, whereas others do not. Should CI compliance checks be updated to check line length in .rst files?

@fabiobaltieri
Copy link
Member

I notice there is inconsistency on this within the Zephyr docs. Some .rst files appear to use multi-line sentences, whereas others do not. Should CI compliance checks be updated to check line length in .rst files?

Yeah that'd be a nice thing to have and have one less thing on the reviewer to keep an eye on.

@kartben
Copy link
Collaborator

kartben commented Nov 14, 2023

I notice there is inconsistency on this within the Zephyr docs. Some .rst files appear to use multi-line sentences, whereas others do not. Should CI compliance checks be updated to check line length in .rst files?

Yeah that'd be a nice thing to have and have one less thing on the reviewer to keep an eye on.

I am not familiar enough with the checkpatch script to evaluate how easy it would be to take exceptions into account. There will always be some tables, really long URLs etc that will have to be treated as exceptions. I should look into https://pypi.org/project/doc8/ linter again though...

@fabiobaltieri
Copy link
Member

I am not familiar enough with the checkpatch script to evaluate how easy it would be to take exceptions into account. There will always be some tables, really long URLs etc that will have to be treated as exceptions. I should look into https://pypi.org/project/doc8/ linter again though...

Nah forget checkpatch, the idea is to phase it out, look for https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/ci/check_compliance.py, the KeepSorted one I recently added is prob a good starting point, you can filter for the file you care about (.rst ones I guess) and then cycle through the content and check what you want to check.

@kartben
Copy link
Collaborator

kartben commented Nov 14, 2023

Nah forget checkpatch, the idea is to phase it out, look for https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/ci/check_compliance.py, the KeepSorted one I recently added is prob a good starting point, you can filter for the file you care about (.rst ones I guess) and then cycle through the content and check what you want to check.

Thanks for the heads-up! I might give doc8 a serious try (plus, it can be used programmatically which is nice). The only somewhat tricky but will be to only flag the errors that are related to the lines that were touched.

@fabiobaltieri
Copy link
Member

Thanks for the heads-up! I might give doc8 a serious try (plus, it can be used programmatically which is nice). The only somewhat tricky but will be to only flag the errors that are related to the lines that were touched.

None of our checks does that at the moment, you may have to bite the bullet and fix all files before deploying, that's how it went for yamllint.

@kartben
Copy link
Collaborator

kartben commented Nov 14, 2023

Thanks for the heads-up! I might give doc8 a serious try (plus, it can be used programmatically which is nice). The only somewhat tricky but will be to only flag the errors that are related to the lines that were touched.

None of our checks does that at the moment, you may have to bite the bullet and fix all files before deploying, that's how it went for yamllint.

Well checkpatch does work on the diff and the diff only, does it not?

@fabiobaltieri
Copy link
Member

Well checkpatch does work on the diff and the diff only, does it not?

It does, but Perl :-)

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

Successfully merging this pull request may close these issues.

None yet

8 participants