Refactor configuration

[nsmith5]

Issue

Fulcio configuration is a bit of an inconsistent experience right now for the end user and as a developer. From an operator perspective:

• The OIDC provider details live on a config file
• If you use PKCS#11 like softhsm its config is a different file
• There are many flags for most of the configuration
• The flags are consistent about using _ versus - as a seperator (e.g --ct-log-url vs --gcp_private_ca_parent)
• There isn't an equivalent environment variable for most of these configurations

From a developer perspective we make calls to directly to viper.GetString("flag-name") to get config. Because of the dynamic nature of that call it means you need to keep track that this data was validated previously. For example:

• We define the --fileca-cert here
• Validate it here
• And then finally use it down here

Potential Solutions / Improvements

From an operator perspective its nice when you can use config file, flag or env var to achieve the same thing. Eg. with a configuration file like

server:
  port: 3000
  host: 0.0.0.0
ca:
  file:
    key: /path/to/key.pem
    passwdFile: /path/to/passwdfile.txt
    cert: /path/to/cert.pem
    watch: true

You can set an flag like --server.port=3003 or set env var FULCIO_SERVER_PORT=3003 for example to modify the port.

From a developer perspective we load this configuration and validate data once on start up and then just pass the confige struct down into each component or via context stuffing. This could look like e.g

type Config struct {
  Server struct {
    Port int
    Host string
  }
  CA struct {
    File struct {
      Key string
      PasswdFile string
    }
  }
}

func main() {
  var conf Config
  err := viper.Unmarshal(&conf)
  if err != nil {
    panic("Oh dear!")
  }
}

To bind the config options to environment variables we can use viper.AutomaticEnv() and to bind all the flags we can use viper.BindPFlags().

Pros:

• We load and validate config once upfront into a strongly typed data structure
• We can test against that data structure / mock more easily
• Operator has options for configuration
• Maybe easier to document?

Cons:

• We probably need to make some breaking changes to the current configuration to set this on the right track
• Is more options more attack surface? Wasn't sure if there was a security implication to this.

[jdolitsky]

Came here to open this. Looks like some of it resolved in #315 yesterday, but there are still discrepancies related to _ and -. Also config vs. config-path may confuse some people.

[nsmith5]

Yooooo that is awesome work. Yeah I think I'd still like to see the following

• Marshal into a data structure early and stop using viper.GetString
• Consistency between _ and -
• Clarify config versus config-path etc

Maybe a few other things. But perhaps we should make little issues for each?

[jdolitsky]

That sounds good. I'm wondering how the nested structure on config file you provided will play with viper.. maybe separating with a dot like --server.port=3003 would work, and that solves the _ vs - problem altogether

[nsmith5]

The nested structure on the config file works with viper (I've done this multiple times in other projects) so that e.g FULCIO_CA_FILE_CERTPATH will unmarshal into the struct

type Config struct {
  CA struct {
    File struct {
      CertPath string
    }
  }
}

The one I don't know about is automagically mapping that to --ca.file.certpath=foo. I'm sure I've seen a go project that had that behaviour, but I can't find it for the life of me. Maybe that is a kingpin feature?

[nsmith5]

Here is example without that cool flag behavior in one my projects https://github.com/nsmith5/rekor-sidekick/blob/main/cli.go#L28-L57

[nsmith5]

Perhaps it was one of these projects used for that flag thing

• https://github.com/octago/sflags
• https://github.com/jaffee/commandeer#features
• https://github.com/anacrolix/tagflag

[lukehinds]

I am happy to park 1.0 for this , but first does anyone plan to work on this and what would be the ETA?

[nsmith5]

I can take this unless @jdolitsky wants to or already has started work (happy to collab too!). I've got a bunch of bandwidth to make this happen starting on Thursday. I think it would take me ~1 week with code review and all the pipeline breakage it will cause

[jdolitsky]

@nsmith5 go for it - I'll reach out on slack and see if theres anything to help with there

[nsmith5]

@lukehinds @jdolitsky I totally over estimated my skills and underestimated how much of a refactor this would be. I've had a poke at this but it feels like a bit of a task. The configuration code is fairly tightly coupled to other things around the code base so its a little challenging to refactor configuration with having to refactor all of those. Here is an example

• https://github.com/sigstore/fulcio/blob/main/pkg/config/config.go#L48

You can see that the configuration of OIDC providers is coupled to caching of issuer verifiers in a LRU.

Perhaps someone else would be able to set this on the right path quickly, but I don't think I've got the chops for it. One other approach might be worth thinking about:

• Release 1.0
• Do some ground work in 1.x to decouple configuration and centralize it without breaking the current configuration API
• Cut over to a more consistent configuration API for 2.0

Limme know what ya'll think :D

[lukehinds]

I agree and let's to do this later. A big refactor right before a major release is inviting problems, so you're wise to call this.