nixos/aesmd: add qcnl config by rvolosatovs · Pull Request #181347 · NixOS/nixpkgs

rvolosatovs · 2022-07-13T11:06:07Z

Description of changes

QCNL is used by AESMD internally, this allows for configuring the library.

See https://profianinc.github.io/nixpkgs/options.html#opt-services.aesmd.qcnl.settings for rendered doc

Things done

nixos/modules/services/security/aesmd.nix

nixos/tests/aesmd.nix

rvolosatovs · 2022-07-25T16:43:16Z

@veehaitch PR updated as discussed, PTAL!
Do we need to add some kind of alias for the aesmd.settings level? It looks like a breaking change, since original AESMD PR was merged in 22.05 #148593

I highly doubt anyone apart from us uses it though, since there's no kernel package with SGX support in nixpkgs anyway AFAIK

veehaitch · 2022-08-04T14:34:39Z

Looks good to me, thanks! However, you have to convert the description = fields to Markdown to resolve the conflicts.

We should definitely add an alias through mkRenamedOptionModule. We have Kernel support for SGX since #153525:

# zcat /proc/config.gz | grep "SGX"
CONFIG_X86_SGX=y
CONFIG_X86_SGX_KVM=y

nixos/modules/services/security/aesmd.nix

veehaitch

Thanks for working on this!

rvolosatovs · 2022-08-22T11:59:37Z

Any update on this PR?

SuperSandro2000 · 2022-08-31T13:44:40Z

nixos/modules/services/security/aesmd.nix

-        options.defaultQuotingType = mkOption {
-          type = with types; nullOr (enum [ "ecdsa_256" "epid_linkable" "epid_unlinkable" ]);
+        options.retryDelay = mkOption {
+          type = with types; nullOr ints.u32;


First time I am seeing this type being used.

SuperSandro2000 · 2022-08-31T13:45:08Z

nixos/modules/services/security/aesmd.nix

+          type = with types; nullOr ints.u32;
+          default = null;
+          example = 6;
+          description = mdDoc ''
+            Maximum retry times for QCNL. When `null` or set to `0`, no retry will be performed.


Suggested change

type = with types; nullOr ints.u32;

default = null;

example = 6;

description = mdDoc ''

Maximum retry times for QCNL. When `null` or set to `0`, no retry will be performed.

type = types.ints.u32;

default = 0;

example = 6;

description = mdDoc ''

Maximum retry times for QCNL. When set to `0`, no retry will be performed.

Why not do this instead?

I agree that's better UX, but I directly matched upstream behavior here https://github.com/intel/SGXDataCenterAttestationPrimitives/blob/68a77a852cd911a44a97733aec870e9bd93a3b86/QuoteGeneration/qcnl/linux/sgx_default_qcnl.conf#L23-L26

I don't think we need to mirror the null here. Not defined would be equal to the option having not default and for clarity we can then also set the default to 0 which is equal to null.

haraldh · 2023-04-19T16:15:48Z

@rvolosatovs @SuperSandro2000 anything needed for this PR to be merged?

rvolosatovs · 2023-04-19T16:17:55Z

@rvolosatovs @SuperSandro2000 anything needed for this PR to be merged?

I'll try to get this addressed today #181347 (comment) and it should be good to go

haraldh · 2023-05-11T18:13:39Z

@rvolosatovs @SuperSandro2000 anything needed for this PR to be merged?

I'll try to get this addressed today #181347 (comment) and it should be good to go

@rvolosatovs any progress? 😉

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

Flatten to better match other services Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

rvolosatovs · 2023-06-12T09:37:13Z

@rvolosatovs @SuperSandro2000 anything needed for this PR to be merged?

I'll try to get this addressed today #181347 (comment) and it should be good to go

@rvolosatovs any progress? wink

Getting back to this now, sorry for the delay - I have unarchived the source repository to be able to update the branch, but honestly not sure what to do about #181347 (comment) I did a direct translation of upstream config here, if the intention is to "do things better" in the NixOS module, then it'd make sense to not change this one field semantics, but also other fields. I'd rather just stick with upstream, which would simplify things like e.g. migration from existing configs and further maintenance in case upstream changes something

SuperSandro2000 · 2023-06-20T11:47:03Z

nixos/modules/services/security/aesmd.nix

-          ];
+            "${aesmdConfigFile}:/etc/aesmd.conf"
+          ]
+          ++ optional (!isNull cfg.qcnl) (let


Suggested change

++ optional (!isNull cfg.qcnl) (let

++ optional (cfg.qcnl != null) (let

SuperSandro2000 · 2023-06-20T11:52:58Z

nixos/modules/services/security/aesmd.nix

+        };
+        options.useSecureCert = mkOption {
+          type = with types; nullOr bool;
+          default = null;


What does null mean here? true? From the description this is unclear.

rvolosatovs requested a review from veehaitch July 13, 2022 11:06

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 13, 2022

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 13, 2022

rvolosatovs force-pushed the feat/aesmd-qcnl branch 2 times, most recently from 9104719 to 81ebdf6 Compare July 13, 2022 13:29

veehaitch reviewed Jul 13, 2022

View reviewed changes

nixos/modules/services/security/aesmd.nix Outdated Show resolved Hide resolved

nixos/modules/services/security/aesmd.nix Outdated Show resolved Hide resolved

nixos/tests/aesmd.nix Outdated Show resolved Hide resolved

rvolosatovs force-pushed the feat/aesmd-qcnl branch from 81ebdf6 to 8824c6e Compare July 25, 2022 16:38

rvolosatovs requested a review from veehaitch July 25, 2022 16:40

rvolosatovs force-pushed the feat/aesmd-qcnl branch from 8824c6e to 5513d11 Compare August 9, 2022 07:10

veehaitch reviewed Aug 9, 2022

View reviewed changes

nixos/modules/services/security/aesmd.nix Outdated Show resolved Hide resolved

rvolosatovs force-pushed the feat/aesmd-qcnl branch from 5513d11 to 4be9397 Compare August 9, 2022 07:54

veehaitch approved these changes Aug 12, 2022

View reviewed changes

bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 12, 2022

SuperSandro2000 reviewed Aug 31, 2022

View reviewed changes

rvolosatovs added 3 commits June 12, 2023 11:29

nixos/aesmd: add qcnl config

f590bcc

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

nixos/aesmd: remove settings attribute

f845f17

Flatten to better match other services Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

nixos/aesmd: drop the lib prefix

a7fab34

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

rvolosatovs force-pushed the feat/aesmd-qcnl branch from 4be9397 to a7fab34 Compare June 12, 2023 09:39

SuperSandro2000 reviewed Jun 20, 2023

View reviewed changes

wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 7, 2023

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024

nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 25, 2025

	++ optional (!isNull cfg.qcnl) (let
	++ optional (cfg.qcnl != null) (let

Uh oh!

Conversation

rvolosatovs commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rvolosatovs commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veehaitch commented Aug 4, 2022

Uh oh!

Uh oh!

veehaitch left a comment

Choose a reason for hiding this comment

Uh oh!

rvolosatovs commented Aug 22, 2022

Uh oh!

SuperSandro2000 Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

rvolosatovs Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

haraldh commented Apr 19, 2023

Uh oh!

rvolosatovs commented Apr 19, 2023

Uh oh!

haraldh commented May 11, 2023

Uh oh!

rvolosatovs commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SuperSandro2000 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rvolosatovs commented Jul 13, 2022 •

edited

Loading

rvolosatovs commented Jul 25, 2022 •

edited

Loading

rvolosatovs commented Jun 12, 2023 •

edited

Loading