tls: Generate TLS assets through dependency graph #145

yifan-gu · 2018-08-17T19:18:51Z

Created CertKey type that implements the asset interface.
Pulling the tls asset generation logic from /installer/pkg/config-generator/tls.go

wking · 2018-08-17T22:47:20Z

pkg/asset/tls/certkey.go

I'd rather use separate assets for the key and the cert. I guess most folks who want to bring their own keys will also bring their own certs, but having separate assets would support us cert'ing folks who bring only their own keys. A potentially more-useful reason for splitting certs from keys is that we could recycle keys and only rebuild certs if something feeding the cert changed (e.g. the DNS names because the user decided to change that config and rebuild their assets).

I don't think we support user provided key/certs?
cc @crawford @abhinavdahiya @aaronlevy @smarterclayton

In Tectonic we were going to target just allowing a user to show up with a CA (or assets to generate intermediate CA), but that was it (they wouldn't get to provide component level certificates).

I don't know if that's acceptable from the Openshift product side. IMO letting user provide component level certs is just asking for them to mess it up (and rotation may not work as expected). But I've heard various things across the spectrum in various meetings, from:

We generate everything (offline root + in-cluster CA). No option to customize

We generate everything, but a user can provide is a CSR endpoint that we can use to ask for an in-cluster CA (and we'd plug endpoint into installer).

User can provide cluster-CA or root CA used to generating cluster-CA (a-la Tectonic plans).

User can provide all component certs.

@smarterclayton / @eparis -- any specific constraints here we should be working from? My position is going to be the less options we provide (and the more we control) the better - but not always realistic.

wking · 2018-08-17T22:49:00Z

pkg/asset/tls/certkey.go

I don't see you setting this false anywhere:

$ git grep AppendParent origin/pr/145 origin/pr/145:pkg/asset/tls/certkey.go: AppendParent bool // Whether append the parent CA in the cert. origin/pr/145:pkg/asset/tls/certkey.go: key, crt, err = generateCert(caKey, caCert, cfg, c.AppendParent) origin/pr/145:pkg/asset/tls/variables.go: AppendParent: true, origin/pr/145:pkg/asset/tls/variables.go: AppendParent: true, origin/pr/145:pkg/asset/tls/variables.go: AppendParent: true, origin/pr/145:pkg/asset/tls/variables.go: AppendParent: true,

Can we drop it?

For CAs I didn't set it so it's default to false, I can set it to false explicitly.

For CAs I didn't set it...

Ah, that explains my grep. Can we just use !IsCA then, or do we need to toggle this independently sometimes?

only server certs need to append the signing CA; client certs don't need append.

Also, IsCA vs IsRoot ?

https://github.com/openshift/library-go/blob/master/pkg/crypto/crypto.go

only server certs need to append the signing CA; client certs don't need append.

Can we distinguish that using ExtKeyUsageClientAuth?

Also, IsCA vs IsRoot ?

Won't root just be "IsCA set, but ParentCA unset"?

The IsCA boolean is required by the x509.Certificate type.

Can we distinguish that using ExtKeyUsageClientAuth?

Seems we can't, the apiserver-proxy cert still has ExtKeyUsageClientAuth but it doesn't have the AppendParent

BTW, I split the root into a separate type as it shares less code with others.

wking · 2018-08-17T22:50:32Z

pkg/asset/tls/variables.go

This seems to be unused.

it's used by the kubelet cert

wking · 2018-08-17T22:51:46Z

pkg/asset/tls/certkey.go

If everyone is using 10-years for this, we can drop this knob to simplify the structure.

If everyone is using 10-years for this...

They aren't, so this property is fine as it stands.

wking · 2018-08-17T22:53:34Z

pkg/asset/tls/variables.go

How important is it to vary OrganizationalUnit? I'd expect that CommonName is sufficient context and we can just hard-code []string{"openshift"} for this in your cert-Asset method (instead of having it configurable via a cert-Asset-struct property).

Hmm, there's also Organization field, I am not sure what's the best way of dealing with all those, so maybe just leave as is for now, and we can refactor later?

wking · 2018-08-17T22:54:35Z

pkg/asset/tls/variables.go

I expect we can generate these filenames from the subject common name, in which case they don't have to be part of our CertKey config. For an example in my mockup, see here (my Assets have names, so getting the asset filename is a generic Asset method, not a TLS-specific method).

good point.

@wking After a second look, seems like we have some tls assets whose subject name is not a fixed value (e.g. tnc, ingress), so might just keep as is unless you feel strong about it.

yifan-gu · 2018-08-22T22:52:25Z

pkg/asset/installconfig/installconfig.go

I think many packages will need to perform this. so either move it to a common helpfer func (ideally it can just take the asset.State and iterate over the contents) or make the Store to do it? But I am aware that some targets won't need to write stuff into disk (e.g. the targetable assets like Cluster)

But I am aware that some targets won't need to write stuff into disk (e.g. the targetable assets like Cluster)

I think we should just write all the assets to disk. It should help with reloading from disk for regeneration (or for generating the next asset group). And even if an asset will never be needed again, disk space isn't that expensive ;)

@wking I know, by targetable assets I mean things like Cluster, Manifests which are not the actual assets, they just represent a group of assets, so they don't have any content to write.

... by targetable assets I mean things like Cluster, Manifests which are not the actual assets, they just represent a group of assets, so they don't have any content to write.

I'd rather not create pseudo-assets for grouping.

that's already part of the dependency graph, it's meant to be the entrypoint for different stages of the installer, so that users can insert their customization.
The installer's subcommands (openshift-install installconfig, openshift-install ignition) also correspond to these targets.

abhinavdahiya · 2018-08-28T17:47:15Z

@wking PTAL (all green)

wking · 2018-08-28T22:52:07Z

pkg/asset/tls/certkey_test.go

It looks like you're ignoring an error here. Can you replace this with:

installConfigState, err := installConfig.Generate(nil) if err != nil { t.Fatal(err) }

yeah cuz it's fake install config, but I added anyway.

wking · 2018-08-29T17:36:16Z

Ok, I've got just the three nits (which I'm fine punting on or handling in this PR), and the golint errors (which I think we should fix before merging). Once we get the golint issues fixed I'll /lgtm.

Move helper functions from 'installer/pkg/tls.go' and 'installer/pkg/utils.go'

yifan-gu · 2018-08-29T19:37:12Z

/lint

openshift-ci-robot

@yifan-gu: 2 unresolved warnings and 4 new warnings.

Details

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2018-08-29T19:37:21Z

pkg/asset/tls/certkey.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

Golint comments: should have a package comment, unless it's in another file for this package.

I think this is just a limitation of the golint plugin, because you have package docs in a sibling file:

$ git grep '// Package tls ' 0a7e6f4 -- pkg/asset/tls 0a7e6f4:pkg/asset/tls/doc.go:// Package tls defines and generates the tls assets based on its dependencies.

And the golint Prow job is happy, so I don't think we need to worry about these.

openshift-ci-robot · 2018-08-29T19:37:22Z

pkg/asset/tls/keypair.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-08-29T19:37:22Z

pkg/asset/tls/helper.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-08-29T19:37:22Z

pkg/asset/tls/helper.go

Golint comments: exported const ValidityTenYears should have comment (or a comment on this block) or be unexported. More info.

yifan-gu · 2018-08-29T19:39:49Z

/lint

openshift-ci-robot

@yifan-gu: 5 unresolved warnings and 0 new warnings.

Details

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

The Generate* functions should not write files to disk in the function. It should be done by the caller.

wking · 2018-08-29T19:57:29Z

The go-vet errors are:

# github.com/openshift/installer/pkg/asset/tls
pkg/asset/tls/certkey_test.go:77:20: undefined: validityTenYears
pkg/asset/tls/certkey_test.go:94:21: undefined: validityTenYears
pkg/asset/tls/certkey_test.go:116:21: undefined: validityTenYears
# github.com/openshift/installer/installer/pkg/config-generator
installer/pkg/config-generator/generator_test.go:134:13: undefined: validityTenYears
installer/pkg/config-generator/generator_test.go:154:19: undefined: validityTenYears

You probably need to convert to the new public versions there too.

wking · 2018-08-29T19:59:57Z

pkg/asset/tls/certkey.go

nit: add a trailing period. From here:

Notice this comment is a complete sentence that begins with the name of the element it describes.

wking · 2018-08-29T20:01:16Z

pkg/asset/tls/stock.go

nit: "key/cert" -> "key".

It is generating both public and private key pair, why just saying "key"?

It is generating both public and private key pair, why just saying "key"?

Making a "key/cert" -> "key" replacement will leave us with "...generates the service-account key pair.". I'm fine if you want to call out public/private as well, but I don't want to claim there's a cert involved.

wking · 2018-08-29T20:02:04Z

pkg/asset/tls/stock.go

nit: "key/cert" -> "key".

Add more helper functions to compute some tls info from installconfig.

CertKey implements the asset.Asset interface and generates private key and certs that are signed by the parent CA.

KeyPair implements the asset.Asset interface and create an RSA public/private key pair.

RootCA implements asset.Asset interface and generate a pair of root ca cert/key.

All tls assets are either of CertKey type or KeyPair type, they all implements the asset.Asset interface with different configuration.

wking · 2018-08-29T20:18:07Z

/lgtm

/wking crosses his fingers for the e2e and smoke tests ;)

openshift-ci-robot · 2018-08-29T20:18:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking, yifan-gu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [wking,yifan-gu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yifan-gu force-pushed the generate_tls branch 2 times, most recently from 3db2959 to 7d3a374 Compare August 17, 2018 21:49

yifan-gu mentioned this pull request Aug 17, 2018

Create asset graph engine #120

Merged

yifan-gu force-pushed the generate_tls branch 2 times, most recently from 32cabef to 7773def Compare August 17, 2018 22:22

wking reviewed Aug 17, 2018

View reviewed changes

yifan-gu force-pushed the generate_tls branch 3 times, most recently from 7e5d785 to e0a7bb5 Compare August 22, 2018 01:18

yifan-gu mentioned this pull request Aug 22, 2018

asset: Add asset generation for kubeconfigs #160

Merged

yifan-gu force-pushed the generate_tls branch from e0a7bb5 to f4dd71f Compare August 22, 2018 19:13

yifan-gu commented Aug 22, 2018

View reviewed changes

yifan-gu force-pushed the generate_tls branch 6 times, most recently from 19e5240 to 6e736c8 Compare August 23, 2018 22:47

yifan-gu changed the title ~~WIP: Generate TLS assets~~ tls: Generate TLS assets through dependency graph Aug 23, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2018

yifan-gu force-pushed the generate_tls branch 3 times, most recently from 81629de to 4fc301f Compare August 28, 2018 01:16

abhinavdahiya approved these changes Aug 28, 2018

View reviewed changes

yifan-gu force-pushed the generate_tls branch from 4fc301f to 4e5cb88 Compare August 28, 2018 22:47

wking reviewed Aug 28, 2018

View reviewed changes

tls: Copy helper functions.

2bed07c

Move helper functions from 'installer/pkg/tls.go' and 'installer/pkg/utils.go'

yifan-gu force-pushed the generate_tls branch from ed03ce8 to 3ce285d Compare August 29, 2018 19:36

openshift-ci-robot reviewed Aug 29, 2018

View reviewed changes

yifan-gu force-pushed the generate_tls branch from 3ce285d to b3d9eda Compare August 29, 2018 19:38

openshift-ci-robot reviewed Aug 29, 2018

View reviewed changes

tls: Refactor the helper functions.

1776cc5

The Generate* functions should not write files to disk in the function. It should be done by the caller.

yifan-gu force-pushed the generate_tls branch from b3d9eda to 0a7e6f4 Compare August 29, 2018 19:45

wking reviewed Aug 29, 2018

View reviewed changes

pkg/asset/tls/stock.go Outdated

Copy link

Member

wking Aug 29, 2018

Choose a reason for hiding this comment

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

nit: "key/cert" -> "key".

yifan-gu force-pushed the generate_tls branch 2 times, most recently from a8c7460 to d7428e8 Compare August 29, 2018 20:09

yifan-gu and others added 6 commits August 29, 2018 13:13

tls: Add more helper functions

5c08a15

Add more helper functions to compute some tls info from installconfig.

tls: Add certkey type that implements the asset.Asset interface

a3475a2

CertKey implements the asset.Asset interface and generates private key and certs that are signed by the parent CA.

tls: Add KeyPair type that implements the asset.Asset interface

70d86e7

KeyPair implements the asset.Asset interface and create an RSA public/private key pair.

tls: Add RootCA type to implement the asset.Asset interface.

fc9671e

RootCA implements asset.Asset interface and generate a pair of root ca cert/key.

tls: establish stock for all tls assets.

533c601

All tls assets are either of CertKey type or KeyPair type, they all implements the asset.Asset interface with different configuration.

*: Fix golint.

cf8cd4c

yifan-gu force-pushed the generate_tls branch from d7428e8 to cf8cd4c Compare August 29, 2018 20:14

openshift-ci-robot assigned wking Aug 29, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2018

openshift-merge-robot merged commit ae7fe33 into openshift:master Aug 29, 2018

yifan-gu deleted the generate_tls branch August 29, 2018 21:03

tls: Generate TLS assets through dependency graph #145

tls: Generate TLS assets through dependency graph #145

Uh oh!

Conversation

yifan-gu commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifan-gu Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 29, 2018

Uh oh!

yifan-gu commented Aug 29, 2018

Uh oh!

openshift-ci-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifan-gu commented Aug 17, 2018 •

edited

Loading

yifan-gu Aug 21, 2018 •

edited

Loading