Skip to content

sys/fido2: model Kconfig #17435

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

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Conversation

Ollrogge
Copy link
Member

Contribution description

This models Kconfig for the FIDO2 implementation.

Testing procedure

Run tests/sys_fido2_ctap with TEST_KCONFIG=1.

@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework labels Dec 22, 2021
@Ollrogge
Copy link
Member Author

Ollrogge commented Jan 5, 2022

Hey @leandrolanzieri would you mind taking a look ?

@Ollrogge
Copy link
Member Author

@leandrolanzieri I noticed that modeling KConfig for FIDO2 breaks the configurability of FIDO2 with menuconfig. How do I fix this?

@leandrolanzieri
Copy link
Contributor

@leandrolanzieri I noticed that modeling KConfig for FIDO2 breaks the configurability of FIDO2 with menuconfig. How do I fix this?

What do you mean that it 'breaks the configurability'?

@Ollrogge
Copy link
Member Author

@leandrolanzieri I noticed that modeling KConfig for FIDO2 breaks the configurability of FIDO2 with menuconfig. How do I fix this?

What do you mean that it 'breaks the configurability'?

Nvm. Executing TEST_KCONFIG=1 make menuconfig instead of make menuconfig makes it work.

@Ollrogge
Copy link
Member Author

@leandrolanzieri Anything else to change or does it look fine now?

@leandrolanzieri
Copy link
Contributor

@Ollrogge I tested locally and found one missing package and one module discrepancy, comparing to the Makefile dependency resolution:

  • we need to add the fido2_tests package to Kconfig and enable it in the app configuration
  • given that Kconfig defaults to HWRNG whenever it is available but makefile does not, we need to force it in the application to the default one (not related to this PR, it is needed in multiple places that use random).

The following patch should solve both issues:

diff --git a/pkg/Kconfig b/pkg/Kconfig
index ece75a5525..66ed1644b5 100644
--- a/pkg/Kconfig
+++ b/pkg/Kconfig
@@ -19,6 +19,7 @@ rsource "elk/Kconfig"
 rsource "emlearn/Kconfig"
 rsource "esp8266_sdk/Kconfig"
 rsource "fff/Kconfig"
+rsource "fido2_tests/Kconfig"
 rsource "gecko_sdk/Kconfig"
 rsource "gemmlowp/Kconfig"
 rsource "hacl/Kconfig"
diff --git a/pkg/fido2_tests/Kconfig b/pkg/fido2_tests/Kconfig
new file mode 100644
index 0000000000..9648b31c7d
--- /dev/null
+++ b/pkg/fido2_tests/Kconfig
@@ -0,0 +1,4 @@
+config PACKAGE_FIDO2_TESTS
+    bool "FIDO2 tests"
+    help
+        Test suite for FIDO2, U2F, and other security key functions.
diff --git a/tests/sys_fido2_ctap/app.config.test b/tests/sys_fido2_ctap/app.config.test
index 04d26692af..5d3f10b646 100644
--- a/tests/sys_fido2_ctap/app.config.test
+++ b/tests/sys_fido2_ctap/app.config.test
@@ -4,3 +4,10 @@ CONFIG_MODULE_FIDO2=y
 CONFIG_MODULE_FIDO2_CTAP=y
 CONFIG_MODULE_FIDO2_CTAP_TRANSPORT=y
 CONFIG_MODULE_FIDO2_CTAP_TRANSPORT_HID=y
+
+CONFIG_PACKAGE_FIDO2_TESTS=y
+
+# Should be autoselecting the MODULE_PRNG_HWRNG if possible
+# Since the makefile cannot we have to override until end of migration
+# Remove when TEST_KCONFIG is complete
+CONFIG_MODULE_PRNG_MUSL_LCG=y

Feel free to squash everything directly.

@github-actions github-actions bot added the Area: pkg Area: External package ports label Jan 31, 2022
@Ollrogge
Copy link
Member Author

Applied your patch and squashed :)

@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 31, 2022
@leandrolanzieri
Copy link
Contributor

Let's see what Murdock says

@Ollrogge
Copy link
Member Author

Ollrogge commented Feb 1, 2022

Let's see what Murdock says

Looks good :)

Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

ACK

@leandrolanzieri leandrolanzieri added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Feb 1, 2022
@leandrolanzieri leandrolanzieri added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 1, 2022
@leandrolanzieri leandrolanzieri added this to the Release 2022.04 milestone Feb 1, 2022
@leandrolanzieri leandrolanzieri added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Feb 1, 2022
@leandrolanzieri leandrolanzieri merged commit 1e300a4 into RIOT-OS:master Feb 1, 2022
@leandrolanzieri
Copy link
Contributor

Thanks @Ollrogge for this!

@Ollrogge
Copy link
Member Author

Ollrogge commented Feb 1, 2022

Thanks @Ollrogge for this!

Thanks for the help !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants