This patch adds support for configuration files #51

kgiusti · 2025-08-21T23:44:56Z

Once the split-roles-and-keys patch lands on main this PR will need to be rebased against main

cmd/config_test.go

cmd/owner.go

cmd/config_test.go

build/package/rpm/go-vendor-tools.toml

runcom · 2025-09-12T07:55:54Z

CONFIG.md

+go-fdo-server manufacturing --config config.yaml
+
+# Using JSON, over ride address given in configuration file
+go-fdo-server owner --config config.json 127.0.0.1:8080


is there any way to disable JSON? 😄 no biggie but toml seems enough

@runcom yeah looks like we can limit which file types viper will allow. Did you want to support only toml?

that'd be my vote, but let's discuss if you all have opinions, we can always introduce more types later if we need

Thanks @runcom - I'm good with supporting only one file format since that simplifies both the docs and the test complexity. Toml's fine tho I'm flexible on the particular format if there's a consensus for an alternative.

@ben-krieger @mmartinv - what's your opinions?

TOML seems like a good choice. It handles using PEM-encoded content well. There's no good reason to accept multiple formats.

I personally prefer YAML as I think is more intuitive and easier to understand, I would also organize the fsim commands a bit differently:

# Global settings # we will need to modify this if we support other database in the future, probably something like: # db: # driver: "sqlite|postgres|mysql|.." # dsn: .... db: "fdo-server.db" db-pass: "MySecurePassword123!" debug: true http: ssl: false insecure-tls: false cert: "/path/to/server.crt" key: "/path/to/server.key" listen: "127.0.0.1:8080" # Manufacturing server specific settings manufacturing: key: "/path/to/manufacturing.key" rvinfo: - dns: [ rvserver1.example.com ] device_port: 8041 owner_port: 8041 protocol: http ip: [ 192.168.1.1 ] - .... device-ca: cert: "/path/to/device.ca" key: "/path/to/device.key" # Owner server specific settings owner: cert: "/path/to/owner.crt" key: "/path/to/owner.key" reuse-credentials: true external-address: "0.0.0.0:8443" fsim: - fdo.command: - execute: true may_fail: true return_stdout: true return_stderr: true cmd: sh args: [ "-c", "echo Current date: ; date"] - execute: true may_fail: true return_stdout: true return_stderr: false cmd: dmidecode args: [ "--quiet", "--dump-bin", "/var/lib/fdo/client/dmidecode.bin" ] - execute: true may_fail: true return_stdout: true return_stderr: false cmd: bash args: - "-c" - | #! /bin/bash set -xeuo pipefail echo "Current Date: date dmidecode --quiet --dump-bin /var/lib/vdo/client/dmidecode - fdo.wget: dir: /var/lib/fdo/downloads files: - url: "https://example.com/file1" dest: path/relative/to/downloads_dir - url: "https://example.com/file2" dest: /absolute/path/file - fdo.upload: dir: /var/lib/fdo/uploads files: - src: "/absolute/path/in/device/upload1.txt" dst: "file/relative/to/owner/uploads/dir" - src: "relative/path/in/device/upload2.txt" # maybe this has no sense. dst: "file/relative/to/owner/uploads/dir" - fdo.download: dir: /var/lib/fdo/downloads files: - src: "path/relative/to/owner/donwload/dir/download1.txt" dst: "/absolute/path/in/the/device" - fdo.csr: ...

ok, let's please enable yaml and toml and that's it :) just don't lose time/sleep on this and we can get going :) we can drop one or the other later on, we're not released either but today we have nothing so either is fine

@mmartinv what you're suggesting in terms of format and content of the config file is far superior to what this PR proposes, which is a simple mapping of the existing CLI configuration to a file. I think the solution to issue #15 (FSIM ordering) should be responsible for updating the existing configuration format to something similar to what you're proposing above rather than this patch.

kgiusti · 2025-09-22T18:03:18Z

Ok the code documents use of YAML and TOML and includes some validation of both.

sarmahaj

Tested for the intended feature support. Config file only , both command line and config to check precedence and some random testing for validation of config.
Some minor comments for edge use case and for readme.
Config file feature looks great and will be very handy for end-user, thank you !

sarmahaj · 2025-09-25T20:53:59Z

CONFIG.md

+address = "127.0.0.1:8041"
+```
+
+## Usage


Empty section..

sarmahaj · 2025-09-25T21:28:20Z

CONFIG.md

+
+The rendezvous server only uses the global configuration options listed above.
+
+## Configuration File Examples


I think it will be good to provide config files as an example under go-fdo-server/configs/ folder . Provide both YAML and TOML config files for all servers covering all possible config options . In future easy to maintain and useful for development purpose.
Looks like we already have config files there but not YAML or TOML formatted.

Agreed - I'd like to see the examples taken out of this files and put elsewhere. That gives us the ability to check them during CI to detect rot.

I want to get @mmartinv opinion here as well. Does it make sense to follow the rust fdo implementation and have an examples/config subdir for these? Should we convert the systemd env config files to a yaml/toml config?

I don't think we need to do all the changes related to configuration file support in this PR. We can move from the sysconfig based configuration to configuration file based configuration in a follow up PR (both are supposed to work so nothing should break).

It's a good idea to add configuration examples in examples/configs directory and we will also need to add the default configuration to configs/go-fdo-server/go-fdo-server-{manufacturer,owner,rendezvous}.{yaml or toml}.

I think we can do that in the follow up PR alongside the required RPM changes.

sarmahaj · 2025-09-25T21:42:06Z

CONFIG.md

+| Key | Type | Description | Required |
+|-----|------|-------------|----------|
+| `manufacturing-key` | string | Manufacturing private key path | Yes |
+| `device-ca-cert` | string | Device certificate path | Yes |


nit: Device certificate path -> Device CA certificate path

sarmahaj · 2025-09-26T11:44:32Z

cmd/owner.go

 			for _, urlString := range wgets {
 				url, err := url.Parse(urlString)
 				if err != nil || url.Path == "" {
 					continue


We should return from here displaying error that URL is invalid or nil instead of continue . We may fail during on boarding.
Or this behavior is expected since wget URL is not supposed to be mandatory ?
In that case a warning msg about the invalid URL so that user knows the loaded config file has issues. (but still onboarding will fail ,right ?)

Good catch! I think you found a bigger defect here: none of the fsim command line parameters seem to be validated. They should be validated when the command line is parsed rather than failing later when the first device is onboarded. And as you pointed out there should be errors logged here - like where the fsim download file is read rather than exiting the server.

I'm going to open an issue for this so the fix doesn't get bogged down in the config file design work in this PR.

sarmahaj · 2025-09-29T11:30:19Z

cmd/root.go

+	if debug {
+		logLevel.Set(slog.LevelDebug)
+	}
+	insecureTLS = viper.GetBool("insecure-tls")


I have one doubt around insecure-tls config. If that is set to true , means server should be using self-signed certs (generated n memory and use them instead of using from the user provided path) , But even if insecure-tls is set we use cert and key from the user provided path server-cert-path & server-key-path.
Are we missing part to generate the self-signed certs in this case ? Please cmiiw. (Also sorry not related to this PR )
If we set this flag true(and thats did not provide server-cert-path and server-key-path), we get "no TLS cert or key provided" this error.

let's follow up on this in #74 (it's a different track indeed)

Integrates the Viper package to allow the server subcommands to be configured via a file or the CLI. CLI takes precedence. With this change the Viper instance becomes the authoritative source for the configuration as it will manage both the Cobra CLI and the configuration file. Unfortunately Viper does not support updating configuration variables that are passed to Cobra by reference. This means that all configuration state must be accessed via the Viper API. Therefore this patch removes passing references to configuration variables to Cobra and instead uses logic to initialize these variables from Viper early in the command execution code. Viper also does NOT honor any of Cobra's "required" meta data when configuration data is read from a file (i.e. "MarkFlagRequired", "MarkFlagMutuallyExclusive", etc. are ignored). Therefore the code must manually check that required flags are set. This patch removes the Cobra required flags settings and moves all validation checks to where the configuration is pulled from the Viper API. Only CLI and file-based configuration is supported - environment variable-based configuration is TBD. Closes: fido-device-onboard#44 Signed-off-by: Kenneth Giusti <[email protected]>

Signed-off-by: Kenneth Giusti <[email protected]>

cmd/owner.go

 	rootCmd.AddCommand(ownerCmd)

-	//serveCmd.Flags().StringVar(&externalAddress, "external-address", "", "External `addr`ess devices should connect to (default \"127.0.0.1:${LISTEN_PORT}\")")
+	// TODO: add to configuration file TBD


kgiusti · 2025-10-13T19:21:58Z

@mmartinv I'd like your opinion on this latest patch. I've tried to implement something along the lines of your comment above, with a few tweaks to allow embedding the configuration for all three roles in a single file. I haven't changed the CLI at all, and the FSIMs configuration remains CLI-only for now.

Note that I've mapped the proposed "http.ssl" configuration flag to the "UseTLS" operational flag in the code - I'm assuming that's your intent. For now I leave the "http.insecure-tls" flag unused, though I believe it should be used by the owner to skip the rendezvous server certificate validation on TO0 (see

go-fdo-server/internal/tls/tls.go

Line 28 in 3f0f697

InsecureSkipVerify: insecureTLS, //nolint:gosec

)

mmartinv · 2025-10-24T17:00:58Z

Let's please agree first on configuration file format. I opened a discussion #96 so we can debate before implementing the final solution. I am against sharing the same configuration file for all the servers BTW.

kgiusti requested review from mmartinv and runcom August 21, 2025 23:44

kgiusti linked an issue Aug 21, 2025 that may be closed by this pull request

File-based configuration #44

Open

github-advanced-security bot found potential problems Aug 21, 2025

View reviewed changes

kgiusti force-pushed the vipered-runcom branch from d94ca0a to 222a0e3 Compare September 10, 2025 13:55

github-advanced-security bot found potential problems Sep 10, 2025

View reviewed changes

cmd/config_test.go Fixed Show fixed Hide fixed

cmd/config_test.go Fixed Show fixed Hide fixed

cmd/config_test.go Fixed Show fixed Hide fixed

cmd/config_test.go Fixed Show fixed Hide fixed

kgiusti changed the title ~~This patch adds configuration by file support to Antonio's split-roles-and-keys patch~~ This patch adds support for configuration files Sep 10, 2025

kgiusti force-pushed the vipered-runcom branch 3 times, most recently from 5cdcb7d to f65fb0f Compare September 11, 2025 20:02

kgiusti commented Sep 11, 2025

View reviewed changes

build/package/rpm/go-vendor-tools.toml Show resolved Hide resolved

runcom reviewed Sep 12, 2025

View reviewed changes

kgiusti mentioned this pull request Sep 15, 2025

feat: add file-based configuration for serve command #49

Closed

kgiusti requested a review from ben-krieger September 15, 2025 12:55

kgiusti force-pushed the vipered-runcom branch 2 times, most recently from 7ef64b6 to 1a30dd5 Compare September 22, 2025 18:01

kgiusti requested a review from runcom September 22, 2025 18:07

mmartinv mentioned this pull request Sep 24, 2025

Flags Should Specify FSIM Order #15

Open

kgiusti force-pushed the vipered-runcom branch from 1a30dd5 to f73ac6c Compare September 25, 2025 14:43

sarmahaj reviewed Sep 29, 2025

View reviewed changes

kgiusti mentioned this pull request Sep 29, 2025

FSIM file and URL parameters need validation #78

Open

kgiusti force-pushed the vipered-runcom branch from f73ac6c to fb4b3f6 Compare September 29, 2025 14:56

kgiusti added 6 commits October 13, 2025 14:43

ci: add license info for yaml.v3 dependency

492e41b

Signed-off-by: Kenneth Giusti <[email protected]>

docs: remove redundant text

8529465

Signed-off-by: Kenneth Giusti <[email protected]>

fix: support only TOML and YAML config files

66d3ffe

Signed-off-by: Kenneth Giusti <[email protected]>

fix: updates from review feedback

3b901c6

Signed-off-by: Kenneth Giusti <[email protected]>

fix: use a structured configuration schema

6c48e0e

Signed-off-by: Kenneth Giusti <[email protected]>

kgiusti force-pushed the vipered-runcom branch from fb4b3f6 to 6c48e0e Compare October 13, 2025 18:44

github-advanced-security bot found potential problems Oct 13, 2025

View reviewed changes


		The rendezvous server only uses the global configuration options listed above.

		## Configuration File Examples

This patch adds support for configuration files #51

Are you sure you want to change the base?

This patch adds support for configuration files #51

Uh oh!

Conversation

kgiusti commented Aug 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmartinv Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgiusti Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgiusti commented Sep 22, 2025

Uh oh!

sarmahaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

kgiusti commented Oct 13, 2025

Uh oh!

mmartinv commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mmartinv Sep 18, 2025 •

edited

Loading

kgiusti Sep 22, 2025 •

edited

Loading

mmartinv commented Oct 24, 2025 •

edited

Loading