Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPKI: Generate keys #3004

Merged
merged 4 commits into from
Aug 22, 2019
Merged

SPKI: Generate keys #3004

merged 4 commits into from
Aug 22, 2019

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Aug 16, 2019

Add key generation capability for v2 to scion-pki tool.


This change is Reviewable

@oncilla oncilla added c/tooling SCION network tools c/CPPKI SCION Control-plane PKI labels Aug 16, 2019
@oncilla oncilla added this to the Q3S2 milestone Aug 16, 2019
@oncilla oncilla requested a review from lukedirtwalker August 16, 2019 12:18
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

		return err
	}
	if !a.Issuer.isZero() {

would it make sense to integrate the isZero check directly into validate? basically zero always validates. Would make the code here slightly more streamlined.


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

	Long: `
'keys' can be used to generate all the necessary keys used in the SCION control plane PKI as well
as the AS master key.

do we still have an AS master key? didn't the terminology change?


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

	}
	// Check if out directory exists and if not create it.
	_, err = os.Stat(a.outDir)

Wait checking this for every key gen is a bit weird. This should be in the top level gen method.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@scrye scrye self-requested a review August 20, 2019 15:35
Add key generation capability for v2 to scion-pki tool.
Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @scrye)


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

Previously, lukedirtwalker (Lukas Vogel) wrote…

would it make sense to integrate the isZero check directly into validate? basically zero always validates. Would make the code here slightly more streamlined.

Done.


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

Previously, lukedirtwalker (Lukas Vogel) wrote…

do we still have an AS master key? didn't the terminology change?

Yes. It is a symmetric key to derive the HF mac key for example.
It is not strictly part of the PKI, but somehow v1 supports it, so I left support in.


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

Previously, lukedirtwalker (Lukas Vogel) wrote…

Wait checking this for every key gen is a bit weird. This should be in the top level gen method.

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @scrye)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @scrye)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @scrye)

@oncilla oncilla merged commit 48cfa5b into scionproto:master Aug 22, 2019
@oncilla oncilla deleted the pub-spki-keygen branch August 22, 2019 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/CPPKI SCION Control-plane PKI c/tooling SCION network tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants