SPKI: Generate private keys

[oncilla]

Adds:

• support to generate private keys form keys.toml

runGenKey is not replaced currently, because it is in use by the
trust/v2 tests. It will be replaced as soon as TRC and certificate
signing with the new key format is available.

---

This change is 

[scrye]

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla)

---

go/tools/scion-pki/internal/v2/conf/key.go, line 106 at r1 (raw file):

}

// KeyMeta defines the

Missing docstring.

---

go/tools/scion-pki/internal/v2/conf/validity.go, line 41 at r1 (raw file):

// Eval returns the validity period. If the not before time is the zero
// value, the input time is used.
func (v Validity) Eval(now time.Time) scrypto.Validity {

Rename parameter now to notBefore.

Reword docstring to:

The not before parameter is only used if the struct's not before field value is zero.

I think it's easier for callers to reason from the point of view of the parameter, so it's a bit more intutive if the sentence starts out with ti.

---

go/tools/scion-pki/internal/v2/conf/validity.go, line 42 at r1 (raw file):

// value, the input time is used.
func (v Validity) Eval(now time.Time) scrypto.Validity {
	notBefore := now

This can be dropped after the rename.

---

go/tools/scion-pki/internal/v2/keys/priv.go, line 34 at r1 (raw file):

)

func runPrivKey(asMap map[addr.ISD][]addr.IA, dirs pkicmn.Dirs) error {

Function name does not describe behavior. It's more like, generateAndSavePrivateKeys. From a higher level point of view, this package has a few issues related to separation of concerns.

The awkward name of the function is a hint that it implements multiple somewhat-unrelated behaviors: reading configs (file system operation), generating crypto material (crypto computation), writing output (file system operation). It would be nice if it read like:

keyConfigs, err := loadKeyConfigs(...)
keys, err := generateKeys(keyConfigs, ...)
dirConfigs, err := createDirectories(...)
err := storeKeys(dirConfigs, keys)

This way the pipeline is a bit clearer, and it's easier to pinpoint where an error occurred (read error? first step; crypto error? second step; write error? last steps).

Code is also more testable, because a single function call doesn't call as much functionality as before. For example, storeKeys could be tested using golden files, same for loadKeyConfigsand generateKeys. So the current test would be split into 3 smaller tests.

Another advantage is that file manipulation would not be nested deeply within generation code, which would make understanding where everything is saved easier. Also, someone interested only in updating the crypto algorithms would look in a single function.

I know this is quite subjective, and it involves quite a bit of work, but let me know what you think.

[oncilla]

Reviewable status: 6 of 10 files reviewed, 4 unresolved discussions (waiting on @scrye)

---

go/tools/scion-pki/internal/v2/conf/key.go, line 106 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Missing docstring.

Done.

---

go/tools/scion-pki/internal/v2/conf/validity.go, line 41 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Rename parameter now to notBefore.

Reword docstring to:

The not before parameter is only used if the struct's not before field value is zero.

I think it's easier for callers to reason from the point of view of the parameter, so it's a bit more intutive if the sentence starts out with ti.

Done.

---

go/tools/scion-pki/internal/v2/conf/validity.go, line 42 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This can be dropped after the rename.

Done.

---

go/tools/scion-pki/internal/v2/keys/priv.go, line 34 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Function name does not describe behavior. It's more like, generateAndSavePrivateKeys. From a higher level point of view, this package has a few issues related to separation of concerns.

The awkward name of the function is a hint that it implements multiple somewhat-unrelated behaviors: reading configs (file system operation), generating crypto material (crypto computation), writing output (file system operation). It would be nice if it read like:

keyConfigs, err := loadKeyConfigs(...)
keys, err := generateKeys(keyConfigs, ...)
dirConfigs, err := createDirectories(...)
err := storeKeys(dirConfigs, keys)

This way the pipeline is a bit clearer, and it's easier to pinpoint where an error occurred (read error? first step; crypto error? second step; write error? last steps).

Code is also more testable, because a single function call doesn't call as much functionality as before. For example, storeKeys could be tested using golden files, same for loadKeyConfigsand generateKeys. So the current test would be split into 3 smaller tests.

Another advantage is that file manipulation would not be nested deeply within generation code, which would make understanding where everything is saved easier. Also, someone interested only in updating the crypto algorithms would look in a single function.

I know this is quite subjective, and it involves quite a bit of work, but let me know what you think.

I like the approach.

The current code inherited some defects of the v1 control flow. I think it looks much better now.

[scrye]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[scrye]

Reviewable status: complete! all files reviewed, all discussions resolved