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.

Sign up for GitHub

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

Jump to bottom

Expose JWKS via a feature-flag #9813

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Expose JWKS via a feature-flag #9813

merged 1 commit into from
Aug 31, 2020

Conversation

justinsb
Copy link
Member

When the PublicJWKS feature-flag is set, we expose the apiserver JWKS
document publicly (including enabling anonymous access). This is a
stepping stone to a more hardened configuration where we copy the JWKS
document to S3/GCS/etc.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 25, 2020
@justinsb justinsb mentioned this pull request Aug 25, 2020
Merged
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 25, 2020
@justinsb
Copy link
Member Author

(Splitting up #9352 into more reviewable chunks)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2020
rifelpet
rifelpet reviewed Aug 26, 2020
View reviewed changes
Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

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

overall this LGTM. We'll want to add validation when the feature flag is enabled to ensure:

  • k8s 1.19+
  • internet-accessible API ELB
  • not gossip

but we can do that in a followup PR.

pkg/model/components/discovery.go Outdated Show resolved Hide resolved
johngmyers
johngmyers requested changes Aug 26, 2020
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

The DiscoveryOptionsBuilder comments are the only ones of significant concern.

pkg/model/components/discovery.go Outdated Show resolved Hide resolved
pkg/model/components/discovery.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/bootstrapchannelbuilder.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go Outdated Show resolved Hide resolved
upup/pkg/fi/fitasks/keypair.go Outdated Show resolved Hide resolved
upup/pkg/fi/fitasks/keypair.go
@@ -44,6 +45,9 @@ type Keypair struct {
Type string `json:"type"`
// LegacyFormat is whether the keypair is stored in a legacy format.
LegacyFormat bool `json:"oldFormat"`

certificate *fi.TaskDependentResource
Copy link
Member

Choose a reason for hiding this comment

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

What is this for? It doesn't appear to be read by anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

It just felt more intuitive to store it, as we're also storing the fingerprint.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to be more YAGNI on these sorts of things. Anything that's going to need the cert will probably just pull it (and possibly the private key) out of the keystore. But I suppose it doesn't block.

upup/pkg/fi/fitasks/keypair.go
}

func (e *Keypair) setResources(cert *pki.Certificate) error {
e.ensureResources()
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to do stuff like this more lazily, not calculating the sha1 unless some previous ModelBuilder has called CertificateSHA1Fingerprint()

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair - as it is so cheap and to avoid the ordering requirement (must have called CertificateSHA1Fingerprint before setResources), is it OK to leave as is?

Copy link
Member

Choose a reason for hiding this comment

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

It's cheap enough to leave as-is.

upup/pkg/fi/resources.go Outdated Show resolved Hide resolved
@justinsb justinsb force-pushed the expose_jwks branch 2 times, most recently from f2474dc to 5e2d85b Compare August 29, 2020 13:52
johngmyers
johngmyers requested changes Aug 29, 2020
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

Just one typo

upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go Outdated Show resolved Hide resolved
upup/pkg/fi/fitasks/keypair.go
@@ -44,6 +45,9 @@ type Keypair struct {
Type string `json:"type"`
// LegacyFormat is whether the keypair is stored in a legacy format.
LegacyFormat bool `json:"oldFormat"`

certificate *fi.TaskDependentResource
Copy link
Member

Choose a reason for hiding this comment

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

I tend to be more YAGNI on these sorts of things. Anything that's going to need the cert will probably just pull it (and possibly the private key) out of the keystore. But I suppose it doesn't block.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2020
@justinsb @johngmyers
Expose JWKS via a feature-flag
786423f 
When the PublicJWKS feature-flag is set, we expose the apiserver JWKS
document publicly (including enabling anonymous access).  This is a
stepping stone to a more hardened configuration where we copy the JWKS
document to S3/GCS/etc.

Co-authored-by: John Gardiner Myers <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2020
@johngmyers
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2020
@johngmyers
Copy link
Member

/retest

@rifelpet
Copy link
Member

Fixing e2e tests here: kubernetes/test-infra#19056

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@rifelpet
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 56bab9f into kubernetes:master Aug 31, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rifelpet rifelpet rifelpet left review comments

@johngmyers johngmyers johngmyers requested changes

@gjtempleton gjtempleton Awaiting requested review from gjtempleton

@olemarkus olemarkus Awaiting requested review from olemarkus

Assignees

@johngmyers johngmyers

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/nodeup area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
5 participants
@justinsb @k8s-ci-robot @johngmyers @rifelpet @fejta-bot