Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @karampok and @lukedirtwalker)

a discussion (no related file):
I am not sure that if we have the same keymeta but

1. in request the algo/version to be empty (as implementation validation)
2. in certs not to be empty

will break something

go/lib/scrypto/cert/renewal/keytype.go, line 26 at r1 (raw file):

// Because KeyType is used as a map key, it cannot be a string type. (see:
// https://github.com/golang/go/issues/33298)
type KeyType int

this type looks a subset for the cert.KeyType, can we just use the other type and expect some switch case will not run?

go/lib/scrypto/cert/renewal/keytype.go, line 30 at r1 (raw file):

// KeyType values
const (
	unknownKey KeyType = iota

I will switch to KeyType(cert.UnknownKey), iota is a big confusing if there is no no second value.
Or maybe wrap the on the original UnmarshalText

go/lib/scrypto/cert/renewal/keytype_json_test.go, line 30 at r1 (raw file):

func TestKeyTypeUnmarshalJSON(t *testing.T) {
	tests := map[string]struct {
		Input     string

(nit) should be lower case

go/lib/scrypto/cert/renewal/request.go, line 37 at r1 (raw file):

// KeyMeta is the meta information about a key.
type KeyMeta struct {

Can we avoid a new struct and have directly the type KeyMeta []byte

go/lib/scrypto/cert/renewal/request.go, line 48 at r1 (raw file):

	// Version indicates the requested certificate version.
	Version scrypto.Version `json:"version"`
	// FormatVersion is the request format version.

the description is different that the doc, is the TRC needed? Version of the TRC/certificate format (currently 1).

go/lib/scrypto/cert/renewal/request.go, line 58 at r1 (raw file):

	Validity *scrypto.Validity `json:"validity"`
	// Keys holds all keys authenticated by this certificate.
	Keys map[cert.KeyType]KeyMeta `json:"keys"`

If this is the only difference, then I will modify the cert.Base structure to include everything apart from this.

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @karampok and @lukedirtwalker)

go/lib/scrypto/cert/renewal/request.go, line 39 at r1 (raw file):

type KeyMeta struct {
	// Key is the public key.
	Key []byte

here a json tag might be missing

go/lib/scrypto/cert/renewal/request.go, line 58 at r1 (raw file):

Also I am not sure if worth having a map or just having an more explicit struct (with omitempty)

type AutoGenerated struct {
	Subject       string `json:"subject"`
	Version       int    `json:"version"`
	FormatVersion int    `json:"format_version"`
	Description   string `json:"description"`
	Keys          struct {
		Signing struct {
			Key string `json:"key"`
		} `json:"signing"`
		Revocation struct {
			Key string `json:"key"`
		} `json:"revocation"`
	} `json:"keys"`
	Validity struct {
		NotBefore int `json:"not_before"`
		NotAfter  int `json:"not_after"`
	} `json:"validity"`
}

Reviewable status: 0 of 5 files reviewed, 8 unresolved discussions (waiting on @karampok)

a discussion (no related file):

I don't think we gain that much from sharing, that it would be worth doing this. The approach this PR does is very explicit and it is clear what is allowed and what not. Sharing would mean more of this is hidden in the validation function.

go/lib/scrypto/cert/renewal/keytype.go, line 26 at r1 (raw file):

I think it is more explicit like this.

go/lib/scrypto/cert/renewal/keytype.go, line 30 at r1 (raw file):

Done.

go/lib/scrypto/cert/renewal/keytype_json_test.go, line 30 at r1 (raw file):

IMO that doesn't really matter in tests, I like it uppercase. It's the API of this test struct. But if you prefer I can change it.

go/lib/scrypto/cert/renewal/request.go, line 37 at r1 (raw file):

No, the specification dictates to have a struct.

go/lib/scrypto/cert/renewal/request.go, line 39 at r1 (raw file):

good catch, done.

go/lib/scrypto/cert/renewal/request.go, line 48 at r1 (raw file):

That is wrong in the document.

go/lib/scrypto/cert/renewal/request.go, line 58 at r1 (raw file):

Done.

Reviewed 1 of 5 files at r1, 1 of 1 files at r2, 1 of 5 files at r3.
Reviewable status: 3 of 5 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

go/lib/scrypto/cert/base.go, line 173 at r3 (raw file):

const (
	// UnknownKey is the default value for the KeyType, it's invalid.
	UnknownKey KeyType = iota

is it needed now? You can let it

go/lib/scrypto/cert/renewal/BUILD.bazel, line 6 at r3 (raw file):

    name = "go_default_library",
    srcs = [
        "keytype.go",

needed?

go/lib/scrypto/cert/renewal/BUILD.bazel, line 23 at r3 (raw file):

    name = "go_default_test",
    srcs = [
        "keytype_json_test.go",

needed?

go/lib/scrypto/cert/renewal/request.go, line 37 at r1 (raw file):

can you point me out where is that?

go/lib/scrypto/cert/renewal/request.go, line 43 at r3 (raw file):

// Keys contains the public keys.
type Keys struct {

maybe rename this to keySection (name is not very important can be dummy like KeysMetadataSection)

go/lib/scrypto/cert/renewal/request.go, line 64 at r3 (raw file):

	Validity *scrypto.Validity `json:"validity"`
	// Keys holds all keys authenticated by this certificate.
	Keys Keys `json:"keys"`

I always embed the struct to avoid a ackward naming.
Here we Keys that is of type Key that has something with filed Key

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

go/lib/scrypto/cert/base.go, line 173 at r3 (raw file):

Done.

go/lib/scrypto/cert/renewal/BUILD.bazel, line 6 at r3 (raw file):

Done.

go/lib/scrypto/cert/renewal/BUILD.bazel, line 23 at r3 (raw file):

Done.

go/lib/scrypto/cert/renewal/request.go, line 37 at r1 (raw file):

here: https://github.com/scionproto/scion/blob/master/doc/CertificateRenewal.md#request-info-fields

go/lib/scrypto/cert/renewal/request.go, line 43 at r3 (raw file):

I don't know like this it's immediately clear that this is the keys field from the spec, no=?

go/lib/scrypto/cert/renewal/request.go, line 64 at r3 (raw file):

Yeah but construction is very annoying with embedding.

Reviewed 1 of 2 files at r4.
Reviewable status: 1 of 3 files reviewed, all discussions resolved

go/lib/scrypto/cert/renewal/request.go, line 37 at r1 (raw file):

we could wait to sync on that a bit

go/lib/scrypto/cert/renewal/request.go, line 43 at r3 (raw file):

resolved, not really strong opinion on that
( i tend to highlight words to find the short-distance definition, so that pattern breaks my flow, but yeah that is not an argument here)

go/lib/scrypto/cert/renewal/request.go, line 64 at r3 (raw file):

we had this discussion :), resolved

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion

go/lib/scrypto/cert/renewal/request.go, line 192 at r4 (raw file):

type Protected struct {
	Algorithm  string             `json:"alg"`
	KeyType    cert.KeyType       `json:"key_type"`

Note to self: this should not use cert.KeyType

Reviewed 1 of 5 files at r3, 3 of 3 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewed 5 of 5 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved