SPKI: Add AS and Issuer certificate configuration

[oncilla]

This PR adds AS and issuer certificate configuration definitions.

The legacy configuration definitions are removed.

---

This change is 

[karampok]

a discussion (no related file):
Is there any small tutorial (bash commands, inputs) how to actually use the above function from high level.
Something like

1. Given the input file X
2. I execute the command Y
3. and I should observe the ouput Z

---

[karampok]

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

---

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

// ASFile returns the file where the AS certificate config is written to.
func ASFile(dir string, ia addr.IA, version scrypto.Version) string {

Who is using that?

---

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

// LoadAS loads the AS certificate configuration from the provided file. The
// contents are already validated.
func LoadAS(file string) (AS, error) {

What is the benefit of value and not pointer?

---

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

// Encode writes the encoded AS certificate config to the writer.
func (cfg AS) Encode(w io.Writer) error {

the name of the method receiver to be single letter (just a)

---

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

// Validate checks all values are set.
func (cfg AS) Validate() error {

The validation should ideally return all the errors.
E.g. if description is empty and the EncryptionKeyVersion is invalid, I want to know both no one by one.
In the past I used this https://github.com/asaskevich/govalidator to achieve that

---

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

)

func TestAS(t *testing.T) {

In a new code, I would like to have unit-test per function.
For every function(method) in as.go file to be a TestX in as_test.go. Always in the format of

input:= X 
want:= y
got := f(x)
assert (got,want)

---

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

	require.NoError(t, err)

	if *update {

This confuses me, where do I see which flag is that? It is not clear where I control the value

---

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

	"github.com/scionproto/scion/go/tools/scion-pki/internal/v2/conf"
	"github.com/scionproto/scion/go/tools/scion-pki/internal/v2/conf/testdata"

testdata is something that ideally should live in a _test package and filename.
In that case the testdata as name it would not needed.
Maybe worth exploring this change, we will need them (e.g. promtest)

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 15 at r1 (raw file):

// limitations under the License.

package testdata

ideally on _test package

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 31 at r1 (raw file):

// AS generates a AS certificate configuration for testing.
func AS(notBefore uint32) conf.AS {

maybe not exported, also different name mockAS

[oncilla]

Reviewable status: 3 of 13 files reviewed, 10 unresolved discussions (waiting on @karampok)

a discussion (no related file):

Previously, karampok (Konstantinos) wrote…

Is there any small tutorial (bash commands, inputs) how to actually use the above function from high level.
Something like

1. Given the input file X
2. I execute the command Y
3. and I should observe the ouput Z

What function do you mean?

This PR adds the configuration file format.
There is nothing yet you can do with it.
The functionality will follow, but I cannot put all of it into a single PR, otherwise it will be huge.

---

---

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

Previously, karampok (Konstantinos) wrote…

Who is using that?

soon ™️

---

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

Previously, karampok (Konstantinos) wrote…

What is the benefit of value and not pointer?

Immutability, I guess.
Do you feel it should be a pointer?

---

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

Previously, karampok (Konstantinos) wrote…

the name of the method receiver to be single letter (just a)

I think cfg is a better fit because it is more descriptive, and for conf.Isssuer i cannot take i as the receiver name.
Note that the methods are ~ the same between the two.

---

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

Previously, karampok (Konstantinos) wrote…

The validation should ideally return all the errors.
E.g. if description is empty and the EncryptionKeyVersion is invalid, I want to know both no one by one.
In the past I used this https://github.com/asaskevich/govalidator to achieve that

ha, that's a pretty cool library.

---

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

Previously, karampok (Konstantinos) wrote…

In a new code, I would like to have unit-test per function.
For every function(method) in as.go file to be a TestX in as_test.go. Always in the format of

input:= X 
want:= y
got := f(x)
assert (got,want)

Done.

---

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

Previously, karampok (Konstantinos) wrote…

This confuses me, where do I see which flag is that? It is not clear where I control the value

extracted from test.
It's just a convenience "test" to update the golden files.
Otherwise it gets ~painful to have golden files.

(it's just a simple cmd line flag)

---

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

Previously, karampok (Konstantinos) wrote…

testdata is something that ideally should live in a _test package and filename.
In that case the testdata as name it would not needed.
Maybe worth exploring this change, we will need them (e.g. promtest)

I do not follow.
testdata is a common pattern in go.

also, you cannot put them in a _test package, otherwise it is not importable.

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 15 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

ideally on _test package

how would that work?

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 31 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

maybe not exported, also different name mockAS

why different name? clients see it as testdata.AS which is pretty clear in its meaning.

[karampok]

Reviewed 1 of 10 files at r1, 1 of 8 files at r2.
Reviewable status: 5 of 13 files reviewed, 4 unresolved discussions (waiting on @oncilla)

a discussion (no related file):

Previously, Oncilla wrote…

What function do you mean?

This PR adds the configuration file format.
There is nothing yet you can do with it.
The functionality will follow, but I cannot put all of it into a single PR, otherwise it will be huge.

function = functionality

---

---

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

Previously, Oncilla wrote…

Immutability, I guess.
Do you feel it should be a pointer?

Somehow yes, and the return error should be nil, serrors.Wra(...).
But I do no really have argument, just feeling/personal taste.

Just for my knowledge, can you elaborate the immutability benefit you have in your mind?

---

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

Previously, Oncilla wrote…

I think cfg is a better fit because it is more descriptive, and for conf.Isssuer i cannot take i as the receiver name.
Note that the methods are ~ the same between the two.

I always suggest one letter (https://github.com/golang/go/wiki/CodeReviewComments#variable-names), being descriptive imo does not help me to
read the code better because I am trying to find the definition ofcfg, if it c I just know that is receiver.

And I have been bitten by someone once adding a global var (or package I do not remember) as cfg (which a popular name).
I would change to anything else than cfg :)

---

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

Previously, Oncilla wrote…

ha, that's a pretty cool library.

Not very beautiful but resolved

---

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

Previously, Oncilla wrote…

extracted from test.
It's just a convenience "test" to update the golden files.
Otherwise it gets ~painful to have golden files.

(it's just a simple cmd line flag)

Aha, so that is a hidden operation functionality to update the static test content.
It is supposed to be used only by dev, and never fail?right?

---

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

Previously, Oncilla wrote…

I do not follow.
testdata is a common pattern in go.

also, you cannot put them in a _test package, otherwise it is not importable.

resolve, there is a similar comment.

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 15 at r1 (raw file):

Previously, Oncilla wrote…

how would that work?

In that case, i would say that instead of introducing a new testdata package you have only a file conf_test.go with package conf_test and the same content.

In the general case how would you have a testdata package that you can exclude from the the test coverage statistics?
(and let's discuss it in person)

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 31 at r1 (raw file):

Previously, Oncilla wrote…

why different name? clients see it as testdata.AS which is pretty clear in its meaning.

I think I missed that client calls that function.

[oncilla]

Reviewable status: 3 of 16 files reviewed, 4 unresolved discussions (waiting on @karampok)

a discussion (no related file):

Previously, karampok (Konstantinos) wrote…

function = functionality

I'm working on a documentation PR that adds description how to use the tool etc

---

---

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

Previously, karampok (Konstantinos) wrote…

Somehow yes, and the return error should be nil, serrors.Wra(...).
But I do no really have argument, just feeling/personal taste.

Just for my knowledge, can you elaborate the immutability benefit you have in your mind?

When you pass a struct by value to a function, both the caller and the callee are sure that modifications to the struct do not affect the other party.

I don't think making it a pointer just so we can return nil, err is a legitimate reason.
Otherwise, basically everything needs to return pointers.

---

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

Previously, karampok (Konstantinos) wrote…

I always suggest one letter (https://github.com/golang/go/wiki/CodeReviewComments#variable-names), being descriptive imo does not help me to
read the code better because I am trying to find the definition ofcfg, if it c I just know that is receiver.

And I have been bitten by someone once adding a global var (or package I do not remember) as cfg (which a popular name).
I would change to anything else than cfg :)

You're pointing to the wrong section: https://github.com/golang/go/wiki/CodeReviewComments#receiver-names :)

I don't see how an additional package, or a new global variable could bite?

Maybe let's look at this from a different angle, what receiver name would you propose for conf.Issuer?
I'm strongly against using i because that is ~reserved for loop variables by convention.

---

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

Previously, karampok (Konstantinos) wrote…

Not very beautiful but resolved

hm, it is actually not resolved.
since the switch statement will only execute one case.

I looked at your library and it seems to only handle string fields, that is not really useful for the validation we want to do here.

I suggest we revert this change and go back to the original.

If it turns that there is a client demand on displaying all errors at once, we can revisit.

---

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

Previously, karampok (Konstantinos) wrote…

Aha, so that is a hidden operation functionality to update the static test content.
It is supposed to be used only by dev, and never fail?right?

it will never be executed unless a dev enables it.
It is never supposed to fail during normal operation.

---

go/tools/scion-pki/internal/v2/conf/testdata/as.go, line 15 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

In that case, i would say that instead of introducing a new testdata package you have only a file conf_test.go with package conf_test and the same content.

In the general case how would you have a testdata package that you can exclude from the the test coverage statistics?
(and let's discuss it in person)

Initially, the idea was to define the configurations for all spki tests in a central place: conf/testdata.
(see: keys.{pub, priv}_test.go)

However, I agree it is cleaner to define the configs close to the test, where they are actually used.
Refactored and removed test package.

[karampok]

Reviewed 14 of 15 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

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

Previously, Oncilla wrote…

You're pointing to the wrong section: https://github.com/golang/go/wiki/CodeReviewComments#receiver-names :)

I don't see how an additional package, or a new global variable could bite?

Maybe let's look at this from a different angle, what receiver name would you propose for conf.Issuer?
I'm strongly against using i because that is ~reserved for loop variables by convention.

In the past someone added a package like https://github.com/Confbase/cfg and then there were weird issues. And we had to rename.
(like someone once put ctx as a receiver name)

For me any single letter sounds . Anyway, if you have strong preference for cfg keep it.

---

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

Previously, Oncilla wrote…

hm, it is actually not resolved.
since the switch statement will only execute one case.

I looked at your library and it seems to only handle string fields, that is not really useful for the validation we want to do here.

I suggest we revert this change and go back to the original.

If it turns that there is a client demand on displaying all errors at once, we can revisit.

A full manual one-one field name with if statements could do the work
(to my view it is the same lines of code, same "not ideal" approach).

I do not think the client will have ever "strong" demand for that, so it would stay like that for long.
Nevertheless I am neither sure it worth the effort, so I resolve. Maybe worth adding a refactor task in your "backlog"

[karampok]

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