SPKI: Generate certificate chains #3453
Conversation
There was a problem hiding this comment.
Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @oncilla)
go/tools/scion-pki/internal/v2/certs/chain.go, line 34 at r1 (raw file):
type chainMeta struct { Chain cert.Chain
could we extend the cert.Chain with a version?
go/tools/scion-pki/internal/v2/certs/chain.go, line 43 at r1 (raw file):
} func (g chainGen) Run(asMap pkicmn.ASMap) error {
Description?
go/tools/scion-pki/internal/v2/certs/chain.go, line 67 at r1 (raw file):
} func (g chainGen) Generate(cfgs map[addr.IA]conf.AS) (map[addr.IA]chainMeta, error) {
Is that a public API? description, should it be tested alone?
go/tools/scion-pki/internal/v2/certs/chain.go, line 70 at r1 (raw file):
certs := make(map[addr.IA]chainMeta) for ia, cfg := range cfgs { signed, err := g.generate(ia, cfg)
signed? how the generate returns 'signed' cert?
go/tools/scion-pki/internal/v2/certs/chain.go, line 156 at r1 (raw file):
} func (g chainGen) newCert(ia addr.IA, cfg conf.AS, pubKeys map[cert.KeyType]keyconf.Key) *cert.AS {
Method or at least func (_ chainGen) to indicate not being used
go/tools/scion-pki/internal/v2/certs/chain.go, line 233 at r1 (raw file):
} func (g chainGen) createDirs(certs map[addr.IA]chainMeta) error {
personal taste, this (all help functions) to be functions not methods
go/tools/scion-pki/internal/v2/certs/chain_test.go, line 60 at r1 (raw file):
Dirs: pkicmn.Dirs{Root: "./testdata", Out: tmpDir}, } err = g.Run(chainASMap)
Maybe consider to add some behavioral comments on the test. (https://en.wikipedia.org/wiki/Behavior-driven_development)
Given folder X,Y with x,y,z
I run `g.Run`
And I expect a file w to have been created
go/tools/scion-pki/internal/v2/certs/cmd.go, line 96 at r1 (raw file):
return serrors.WrapStr("unable to generate certificate chains", err) } return nil
return g.Run(asMap) should be enough
go/tools/scion-pki/internal/v2/certs/cmd.go, line 102 at r1 (raw file):
func init() { Cmd.AddCommand(genIssuerCmd) Cmd.AddCommand(genChainCmd)
I cannot see any help messages like above (I would like a description of when i run the command what files are being created)
go/tools/scion-pki/internal/v2/certs/loader.go, line 61 at r1 (raw file):
for _, ias := range asMap { for _, ia := range ias { s := selector{
outside for loops
go/tools/scion-pki/internal/v2/certs/loader.go, line 66 at r1 (raw file):
Regex: `as-v(\d*)\.toml$`, } file, err := l.selectConfig(ia, s)
A verbose flag -v might be worth implementing.
Where is will print id details the steps (e.g. which files were selected)
go/tools/scion-pki/internal/v2/certs/loader_test.go, line 59 at r1 (raw file):
} for name, test := range tests { name, test := name, test
name,test:=tn,tc
go/tools/scion-pki/internal/v2/certs/loader_test.go, line 68 at r1 (raw file):
cfgs, err := l.LoadASConfigs(chainASMap) require.NoError(t, err) assert.Equal(t, test.Expected, cfgs[ia111].Version)
I would bring the chainASMap, and ia111 inside the test, it would make it more error resilient and readable.
Also the file
5 $ cat testdata/ISD1/ASff00_0_111/as-v1.toml
description = "AS certificate 1-ff00:0:111"
version = 1
signing_key_version = 1
encryption_key_version = 2
revocation_key_version = 2
issuer_ia = "1-ff00:0:110"
issuer_cert_version = 1
optional_distribution_points = []
[validity]
not_before = 1573033769
validity = "1y"
is small I would consider also to hardcode it in a string and write to a file rather that depend on external data.
go/tools/scion-pki/internal/v2/certs/verify.go, line 40 at r1 (raw file):
return serrors.WrapStr("unable to parse signed issuer certificate", err) } if _, err = v.verifyIssuer(signed); err != nil {
we could potential replace with
return v.verifyIssuer()
if we are not strict not to return the cert
go/tools/scion-pki/internal/v2/certs/verify.go, line 46 at r1 (raw file):
} func (v verifier) VerifyChain(raw []byte) error {
I am not very fond return only error
func ValidateX() bool,err {}
and separate the case
1. happy path - invalid X
2. error path - "file does not exists"
go/tools/scion-pki/internal/v2/certs/verify.go, line 73 at r1 (raw file):
} func (v verifier) verifyIssuer(signed cert.SignedIssuer) (*cert.Issuer, error) {
Is that required to method?
I find easier when we have functions with input/outputs and not have constantly jump out to the receiver initialization (if possible)
oncilla
force-pushed
the
pub-spki-gen-as
branch
from
efee23c to
3822ae3
Compare
There was a problem hiding this comment.
Reviewable status: 8 of 17 files reviewed, 16 unresolved discussions (waiting on @karampok)
go/tools/scion-pki/internal/v2/certs/chain.go, line 34 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
could we extend the cert.Chain with a version?
Chain is simply a container for unparsed AS and issuer certificate.
Adding a version to the chain would mean we need to parse it and assign the version.
This makes it also dangerous for disconnects between parsed data and packed data.
package internal containers that keep the parsed and packed data in one is fine in my opinion:
e.g. trust/v2/internal/decoded.Chain but when package boundaries are crossed, we should try to avoid it imo.
go/tools/scion-pki/internal/v2/certs/chain.go, line 43 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Description?
This is a private type, so no doc needed IMO.
It is pretty self-describing.
go/tools/scion-pki/internal/v2/certs/chain.go, line 67 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Is that a public API? description, should it be tested alone?
Done.
go/tools/scion-pki/internal/v2/certs/chain.go, line 70 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
signed? how the generate returns 'signed' cert?
Done.
go/tools/scion-pki/internal/v2/certs/chain.go, line 156 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Method or at least
func (_ chainGen)to indicate not being used
Done.
go/tools/scion-pki/internal/v2/certs/chain.go, line 233 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
personal taste, this (all help functions) to be functions not methods
I think here grouping makes sense because there is also issGen.createDirs.
But I have no strong opinion.
go/tools/scion-pki/internal/v2/certs/chain_test.go, line 60 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Maybe consider to add some behavioral comments on the test. (https://en.wikipedia.org/wiki/Behavior-driven_development)
Given folder X,Y with x,y,z I run `g.Run` And I expect a file w to have been created
Done.
go/tools/scion-pki/internal/v2/certs/cmd.go, line 96 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
return g.Run(asMap)should be enough
Done.
go/tools/scion-pki/internal/v2/certs/cmd.go, line 102 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
I cannot see any help messages like above (I would like a description of when i run the command what files are being created)
Agreed, in general the tool should be documented and have better help texts.
I would prefer doing that in a separate PR though.
This PR is blocking a lot of work because it is required to generate the crypto material for testing of the trust store.
go/tools/scion-pki/internal/v2/certs/loader.go, line 61 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
outside for loops
Ugh, making the same mistakes again. 😄
Done.
go/tools/scion-pki/internal/v2/certs/loader.go, line 66 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
A
verbose flag-vmight be worth implementing.
Where is will print id details the steps (e.g. which files were selected)
Agreed. I will create an issue tracking this.
go/tools/scion-pki/internal/v2/certs/loader_test.go, line 59 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
name,test:=tn,tc
Done.
go/tools/scion-pki/internal/v2/certs/loader_test.go, line 68 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
I would bring the chainASMap, and ia111 inside the test, it would make it more error resilient and readable.
Also the file5 $ cat testdata/ISD1/ASff00_0_111/as-v1.toml description = "AS certificate 1-ff00:0:111" version = 1 signing_key_version = 1 encryption_key_version = 2 revocation_key_version = 2 issuer_ia = "1-ff00:0:110" issuer_cert_version = 1 optional_distribution_points = [] [validity] not_before = 1573033769 validity = "1y"is small I would consider also to hardcode it in a string and write to a file rather that depend on external data.
The good thing about keeping it in the files and not hard coded is that we can generate it directly by the tool and don't have to copy past the contents when something changes.
go/tools/scion-pki/internal/v2/certs/verify.go, line 40 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
we could potential replace with
return v.verifyIssuer()if we are not strict not to return the cert
Done.
go/tools/scion-pki/internal/v2/certs/verify.go, line 46 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
I am not very fond return only error
func ValidateX() bool,err {} and separate the case 1. happy path - invalid X 2. error path - "file does not exists"
I don't see how this could work.
How do you pass on that the issuer certificate was not verifiable and not the as certificate?
go/tools/scion-pki/internal/v2/certs/verify.go, line 73 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Is that required to method?
I find easier when we have functions with input/outputs and not have constantly jump out to the receiver initialization (if possible)
what do you mean by "jump out to the receiver initialization"?
There was a problem hiding this comment.
Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
go/tools/scion-pki/internal/v2/certs/chain.go, line 233 at r1 (raw file):
Previously, Oncilla wrote…
I think here grouping makes sense because there is also issGen.createDirs.
But I have no strong opinion.
if the grouping makes sense then it might worth different package.
No strong opinion
go/tools/scion-pki/internal/v2/certs/cmd.go, line 106 at r2 (raw file):
Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if err := runHuman(args); err != nil {
return runHuman(args)? no?
go/tools/scion-pki/internal/v2/certs/verify.go, line 46 at r1 (raw file):
Previously, Oncilla wrote…
I don't see how this could work.
How do you pass on that the issuer certificate was not verifiable and not the as certificate?
We discussed that in person
go/tools/scion-pki/internal/v2/certs/verify.go, line 73 at r1 (raw file):
Previously, Oncilla wrote…
what do you mean by "jump out to the receiver initialization"?
same
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok)
go/tools/scion-pki/internal/v2/certs/cmd.go, line 106 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
return runHuman(args)? no?
Done.
oncilla
force-pushed
the
pub-spki-gen-as
branch
from
06085c3 to
15fa1ec
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
This PR adds the capability to generate certificate chains.
Generated chains are verified before writing to file system.
This change is