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

keyconf: Load keys from PEM #3177

Merged
merged 3 commits into from
Oct 2, 2019
Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Sep 23, 2019

This PR introduces a PEM encoding for the keys used in the control
plane PKI.

The PEM file contains key metadata and the key for easier key
management.


This change is Reviewable

@oncilla oncilla added c/tooling SCION network tools c/CPPKI SCION Control-plane PKI labels Sep 23, 2019
@oncilla oncilla added this to the Q3S3 milestone Sep 23, 2019
@oncilla oncilla requested a review from scrye September 23, 2019 10:46
@oncilla oncilla requested review from karampok and removed request for scrye September 30, 2019 07:25
Copy link
Contributor

@karampok karampok 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 6 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @oncilla)


go/lib/keyconf/doc.go, line 20 at r1 (raw file):

// attached metadata. The PEM files have special file names based on the type,
// usage, and version of the key. See the encoding and filename expample for
// more information.

typo in expample

Also do we need documentation what if the Conf struct?


go/lib/keyconf/doc_test.go, line 15 at r1 (raw file):

// limitations under the License.

package keyconf

Is there a reason why the example goes to _test file name?


go/lib/keyconf/key.go, line 49 at r1 (raw file):

	hdrUsage     = "usage"
	hdrVersion   = "version"
	hdrIA        = "ia"

[nit] sort them


go/lib/keyconf/key.go, line 63 at r1 (raw file):

	TRCVotingOnlineKey  Usage = "trc-voting-online"
	TRCVotingOfflineKey Usage = "trc-voting-offline"
	TRCIssuingKey       Usage = "trc-issuing"

[nit] sort them


go/lib/keyconf/key.go, line 74 at r1 (raw file):

	usages := []Usage{ASSigningKey, ASDecryptionKey, ASRevocationKey, IssCertSigningKey,
		IssRevocationKey, TRCVotingOnlineKey, TRCVotingOfflineKey, TRCIssuingKey}
	for _, usage := range usages {

Can that be replaced with a map somehow?

v,ok:=m[text]
if !ok { 
   return Unsupported
}
return v

Also is Exported on purpose?


go/lib/keyconf/key.go, line 76 at r1 (raw file):

	for _, usage := range usages {
		if usage == s {
			*u = usage

I personally do not like to have a method, which replaces the receiver value. Maybe a NewUsage function?


go/lib/keyconf/key.go, line 107 at r1 (raw file):

// Key contains the key with additional metada.
//
// On disk, the key is encoded in PEM with a file name specific to the type,

Maybe worth giving an example of this pattern in the comment
as-decrypt-v%d.key


go/lib/keyconf/key.go, line 111 at r1 (raw file):

// to avoid collisions.
type Key struct {
	Type      Type

Do we need to export all fields of Key?


go/lib/keyconf/key.go, line 117 at r1 (raw file):

	Version   scrypto.KeyVersion
	IA        addr.IA
	Key       []byte

Key is the same name as the struct, rename to Bytes (or maybe keep the pem.Block as embedded struct)


go/lib/keyconf/key.go, line 121 at r1 (raw file):

// KeyFromPEM parses the PEM block.
func KeyFromPEM(block pem.Block) (Key, error) {

If I am not mistaken this is not being used yet? Probably it is just part of your big thing


go/lib/keyconf/key.go, line 123 at r1 (raw file):

func KeyFromPEM(block pem.Block) (Key, error) {
	k := Key{}
	if err := k.Type.UnmarshalText([]byte(block.Type)); err != nil {

Without asking for changes, an alternative approach would be

k:=key{}
if v,ok=getType( block.Type); ok{ k.Type=v}
if v,ok=getUsage( block.Usage); ok{ k.Usage=v}

if err:=validateKey(k); err!=nil{
 return nil, err
}

return key, err

go/lib/keyconf/key.go, line 156 at r1 (raw file):

// PEM encodes the key with metadata into a PEM block.
func (k Key) PEM() pem.Block {

I am not sure when/who will is use this function, but returning an error might be useful

In that case I would propose

if err:=valid(key); err!=nil{ return err}
return k.PEM()
}

go/lib/keyconf/key.go, line 197 at r1 (raw file):

// PublicKeyFile returns the file name for the public key with the provided
// intended usage and version.
func PublicKeyFile(usage Usage, ia addr.IA, version scrypto.KeyVersion) string {

Unless there is a reason, I would just embed the one line return to the File() function. I just see 12 extra lines of code


go/lib/keyconf/key_test.go, line 109 at r1 (raw file):

			k, err := keyconf.KeyFromPEM(block)
			test.ErrAssertion(t, err)
			if err != nil {

Why do we need the if return if we use the require.Error.
What would be different if we use the assert.Error?


go/lib/keyconf/key_test.go, line 126 at r1 (raw file):

}

func TestKeyPem(t *testing.T) {

why this is cannot be a entry in the above test?
The answer is probably not because you want to unit-test the PEM function

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: 2 of 6 files reviewed, 15 unresolved discussions (waiting on @karampok)


go/lib/keyconf/doc.go, line 20 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

typo in expample

Also do we need documentation what if the Conf struct?

Conf is bad and should feel bad :)
It will be gone ~soon-ish.

Done.


go/lib/keyconf/doc_test.go, line 15 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Is there a reason why the example goes to _test file name?

This ensures that the example is not compiled into the binary.
It is actually only needed for documentation purposes.


go/lib/keyconf/key.go, line 49 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

[nit] sort them

Done.


go/lib/keyconf/key.go, line 63 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

[nit] sort them

Done.


go/lib/keyconf/key.go, line 74 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Can that be replaced with a map somehow?

v,ok:=m[text]
if !ok { 
   return Unsupported
}
return v

Also is Exported on purpose?

Done.


go/lib/keyconf/key.go, line 76 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I personally do not like to have a method, which replaces the receiver value. Maybe a NewUsage function?

It is a common pattern and used by a lot of parsing libraries such as toml and json. https://golang.org/pkg/encoding/#TextUnmarshaler
This method will actually be used when parsing toml files in an upcoming PR


go/lib/keyconf/key.go, line 107 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Maybe worth giving an example of this pattern in the comment
as-decrypt-v%d.key

Refer to the example instead to avoid possible inconsistencies.
Godoc will put the example right below the documentation of the struct.


go/lib/keyconf/key.go, line 111 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Do we need to export all fields of Key?

Yes, it functions as a container.


go/lib/keyconf/key.go, line 117 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Key is the same name as the struct, rename to Bytes (or maybe keep the pem.Block as embedded struct)

Done.


go/lib/keyconf/key.go, line 121 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

If I am not mistaken this is not being used yet? Probably it is just part of your big thing

It's not used yet. It will be very soon though.


go/lib/keyconf/key.go, line 123 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Without asking for changes, an alternative approach would be

k:=key{}
if v,ok=getType( block.Type); ok{ k.Type=v}
if v,ok=getUsage( block.Usage); ok{ k.Usage=v}

if err:=validateKey(k); err!=nil{
 return nil, err
}

return key, err

Ack, but we will need to have UnmarshalText in the future anyway for toml and json unmarshalling.


go/lib/keyconf/key.go, line 156 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I am not sure when/who will is use this function, but returning an error might be useful

In that case I would propose

if err:=valid(key); err!=nil{ return err}
return k.PEM()
}

Turning this into a PEM should never fail.


go/lib/keyconf/key.go, line 197 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Unless there is a reason, I would just embed the one line return to the File() function. I just see 12 extra lines of code

This will be needed when parsing files.
I don't want to create a bogus key struct just to get the filename.


go/lib/keyconf/key_test.go, line 109 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Why do we need the if return if we use the require.Error.
What would be different if we use the assert.Error?

when err != nil, the pem block will be empty, thus all checks below would fail. Thus the if clause.

We use require.Error instead of assert.Error, because there is no point of outputting 8 times that the 0 value was found.


go/lib/keyconf/key_test.go, line 126 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

why this is cannot be a entry in the above test?
The answer is probably not because you want to unit-test the PEM function

Done.

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

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


go/lib/keyconf/key.go, line 123 at r1 (raw file):

Previously, Oncilla wrote…

Ack, but we will need to have UnmarshalText in the future anyway for toml and json unmarshalling.

Ack, is there an issue/doc that describes this future need?


go/lib/keyconf/key.go, line 156 at r1 (raw file):

Previously, Oncilla wrote…

Turning this into a PEM should never fail.

but it will panic until someone will do some fuzzing ;) (and someone is you)


go/lib/keyconf/key.go, line 197 at r1 (raw file):

Previously, Oncilla wrote…

This will be needed when parsing files.
I don't want to create a bogus key struct just to get the filename.

An alternative approach would be to make the first string argument of print a const (like a template). I think I have seen such pattern, but i cannot really find. I think it was
somewhere in https://golang.org/src/time/format.go, or in a regexp package.

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, 3 unresolved discussions (waiting on @karampok)


go/lib/keyconf/key.go, line 123 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Ack, is there an issue/doc that describes this future need?

Not really, there are only very high level issues on this.
It will be used by the new scion-pki tool.


go/lib/keyconf/key.go, line 156 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

but it will panic until someone will do some fuzzing ;) (and someone is you)

What will panic?


go/lib/keyconf/key.go, line 197 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

An alternative approach would be to make the first string argument of print a const (like a template). I think I have seen such pattern, but i cannot really find. I think it was
somewhere in https://golang.org/src/time/format.go, or in a regexp package.

I don't follow.

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)


go/lib/keyconf/key.go, line 156 at r1 (raw file):

Previously, Oncilla wrote…

What will panic?

I do not know, maybe somehow a value in key can give something that panics.
For example a nil value on IA and then call the String() on the k.IA.


go/lib/keyconf/key.go, line 197 at r1 (raw file):

Previously, Oncilla wrote…

I don't follow.

A naive approach of what I mean is

const fn = "%s-v%d-my-pattern"

func main() {

	fmt.Printf(fn, "x", 1)

}

how the time format on the time pkg is implemented is an other (more complicated) approach

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 @karampok)


go/lib/keyconf/key.go, line 197 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

A naive approach of what I mean is

const fn = "%s-v%d-my-pattern"

func main() {

	fmt.Printf(fn, "x", 1)

}

how the time format on the time pkg is implemented is an other (more complicated) approach

Ah, you are suggesting just defining the format string constant and let the caller fill in the values themself?
I prefer the type safe way of doing this.

Copy link
Contributor

@karampok karampok 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

This PR introduces a PEM encoding for the keys used in the control
plane PKI.

The PEM file contains key metadata and the key for easier key
management.
@oncilla oncilla merged commit b9a3206 into scionproto:master Oct 2, 2019
@oncilla oncilla deleted the pub-key-format branch October 2, 2019 06:25
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.

None yet

2 participants