Add Yubikey public keys for the testing team

leivos-unikie commented 1 month ago

Description of changes

We have some dedicated Yubikeys for testing purpose. This PR adds those Yubikey public keys to debug builds for Lenovo-X1 target. This enables us to test yubikey service with mainline builds.

Checklist for things done

[x] Summary of the proposed changes in the PR description
[x] More detailed description in the commit message(s)
[x] Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
[x] Contribution guidelines followed
[ ] Ghaf documentation updated with the commit - https://tiiuae.github.io/ghaf/
[ ] PR linked to architecture documentation and requirement(s) (ticket id)
[x] Test procedure described (or includes tests). Select one or more:
- [x] Tested on Lenovo X1 x86_64
- [ ] Tested on Jetson Orin NX or AGX aarch64
- [ ] Tested on Polarfire riscv64
[ ] Author has run nix flake check --accept-flake-config and it passes
[ ] All automatic Github Action checks pass - see actions
[x] Author has added reviewers and removed PR draft status

Testing

nix build github:leivos-unikie/ghaf/tester_yubikeys#lenovo-x1-carbon-gen11-debug

Boot Lenovo-X1 with the image and test that all the added Yubikeys work: Try to login from lock screen by pressing Enter and touching the Yubikey.

Should also ensure that they don't work in release mode but currently nix build github:tiiuae/ghaf#lenovo-x1-carbon-gen11-release fails.

humaidq commented 1 month ago

This means the keys will be included in all builds downstream. Do we want to require anyone who uses Ghaf to lib.mkForce to replace these keys?

leivos-unikie commented 1 month ago

This means the keys will be included in all builds downstream. Do we want to require anyone who uses Ghaf to lib.mkForce to replace these keys?

I was also doubting this in our slack channel. Would it be ok to have them in debug mode only?

vunnyso commented 1 month ago

I think its okay if we keep public keys in debug mode for testing purpose. We are doing similar with ssh keys as well.

vilvo commented 1 month ago

In practice, my structural proposal for improving the development/testing yubikeys is as follows:

create a development module for yubikeys - use ghaf/modules/common/development/ssh.nix as an example
define properly named ghaf development option to enable predefined yubikeys
enable that option only for the debug profile

leivos-unikie commented 1 month ago

I don't agree with this implementation but I agree with this kind of usage on debug-builds for testing and development. The problem with this PR is that it adds the public keys directly to a public keys placeholder list which adds the keys to all builds - not only to -debug-builds. The extension of yubikey pub keys must take place via development modules - see

https://github.com/tiiuae/ghaf/blob/b4042547442db850c25e11be32f2eaa4412cbc55/modules/common/development/ssh.nix#L9

Not like this.

Yes, I was aware that it was not the proper implementation.

leivos-unikie commented 1 month ago

In practice, my structural proposal for improving the development/testing yubikeys is as follows:

1. create a development module for yubikeys - [use ghaf/modules/common/development/ssh.nix as an example](https://github.com/tiiuae/ghaf/blob/b4042547442db850c25e11be32f2eaa4412cbc55/modules/common/development/ssh.nix)

2. define properly named ghaf development option to enable predefined yubikeys

3. [enable that option only for the debug profile](https://github.com/tiiuae/ghaf/blob/b4042547442db850c25e11be32f2eaa4412cbc55/modules/common/profiles/debug.nix#L27)

Thanks for the advice. I got it working in debug mode. Building release variant fails currently (I filed a bug) so can't test that for now.

vilvo commented 1 month ago

Even though I approved the PR, please always fill the PR description field properly - preferably before requesting reviews. https://github.com/tiiuae/ghaf/pull/698#issue-2427357394 - add summary to description and fill the checklist where relevant. This helps both reviewers and integrators.

leivos-unikie commented 1 month ago

Automated build check fails for lenovo-x1 but locally nix build --show-trace github:leivos-unikie/ghaf/tester_yubikeys#lenovo-x1-carbon-gen11-debug succeeds.

leivos-unikie commented 1 month ago

Would it make sense to only add these keys in our reference targets only? Not in all development builds of downstream projects.

Because downstream users of the Ghaf framework may also use the development module/debug build, but that doesn't mean we preload our keys on their builds too? This is the same with SSH keys, that also has to be moved.

So you mean that it should be determined by target name (lenovo-x1-carbon-gen11-debug... ) if the keys are included and those who don't want the keys create some other target?

humaidq-tii commented 1 month ago

So you mean that it should be determined by target name (lenovo-x1-carbon-gen11-debug... ) if the keys are included and those who don't want the keys create some other target?

Not determined by target name necessarily. My proposal is we include these yubikeys and even move the SSH keys under modules/reference, so it is not included downstream.

milva-unikie commented 1 month ago

Tested on Lenovo-X1

Don't know if changes suggested above will be made to this pr, so adding my testing results from Friday :)

Tested 3 out of the 4 keys being added, Samuli has tested the last one
Can login with the keys
Test-automation passes

vilvo commented 1 month ago

My proposal is we include these yubikeys and even move the SSH keys under modules/reference, so it is not included downstream.

I support and like the proposal to restructure authorized public keys for internal use in -debug-images in a way that they don't get included in the downstream. Nevertheless, I propose we scope the work so that this PR enables testing team to operate and merge it first. And then do that refactoring in another PR. @humaidq - what do you think?

tiiuae / ghaf