Add secret for kubeadmin pre-idp user #771
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
pkg/asset/manifests/tectonic.go
Outdated
| @@ -54,6 +62,13 @@ func (t *Tectonic) Generate(dependencies asset.Parents) error { | |||
| worker := &machines.Worker{} | |||
| master := &machines.Master{} | |||
| dependencies.Get(installConfig, clusterk8sio, worker, master) | |||
| kubeadminPassword, kubeadminPasswordHash, err := generateRandomPasswordHash() | |||
There was a problem hiding this comment.
Why are we generating this instead of asking the user if they want to supply a password?
There was a problem hiding this comment.
this (really more this: openshift/origin#21580) creates an initial pre-identity-provider-setup user that can access registry, prometheus, web-console, grafana, while at the same time not creating a bunch of roles/identities/objects that have to be cleaned up once an admin does set up an idp. @enj definitely does not want to let a user choose this password, it has to be secure.
There was a problem hiding this comment.
@enj definitely does not want to let a user choose this password, it has to be secure.
14 random chars is less seceure than my default ;). And I'd really like to not take responsibility for this :p. Can it be a day-2 operation?
There was a problem hiding this comment.
We want to try to prevent the user from setting a password like "password" initially, configuring their IDP, and then ignore the cleanup step (which is to delete the generated secret) leaving an easily guessed admin account. If we force a good password then forgetting to clean up is less dangerous.
cmd/openshift-install/create.go
Outdated
| logrus.Infof("Install complete! Run 'export KUBECONFIG=%s' to manage your cluster.", kubeconfig) | ||
| logrus.Info("After exporting your kubeconfig, run 'oc -h' for a list of OpenShift client commands.") | ||
| // TODO: Direct users to web-console | ||
| // TODO: Get kubeadmin password, log here |
There was a problem hiding this comment.
we already give the user admin kubeconfig. I rather tell people how to get/create the password.
There was a problem hiding this comment.
I rather tell people how to get/create the password.
How about we point folks at their admin kubeconfig (like we do now) and then drop a link to a "common day-2 operations" landing page? Where would something like that live? Master branch of https://github.com/openshift/openshift-docs ? Some sort of pre-release branch? Some other repo?
There was a problem hiding this comment.
without this: openshift/origin#21580 in order to have a user that can login to the console/prometheus/etc, you'd have to set up an identity provider. This kubeadmin user is expected to be removed upon setting up your idp.
There was a problem hiding this comment.
We need to be able to do oauth flows for the console, etc. and that can't be done with just a kubeconfig.
There was a problem hiding this comment.
We need to be able to do oauth flows for the console, etc. and that can't be done with just a kubeconfig.
But this PR is just pushing in a new secret, right? That's not installer-specific. At any time after the control plane comes up, you can use the admin kubeconfig and oc to push in the kubeadmin-password secret. You could wrap it up in an oc subcommand if constructing the password, hashing it, and putting it inside a secret were a concern. A benefit of that approach is that it would be opt-in, folks who are going to start setting up a fully-fledged identity provider immediately after the cluster comes up could skip it and have nothing around to clean up.
There was a problem hiding this comment.
@mrogers950
If you want the admin user to have both admin kubeconfig and admin ui creds by default that is understandable.
We need to be able to do oauth flows for the console, etc. and that can't be done with just a kubeconfig.
But I think you can most definitely do all the steps to do the oauth flows for the console with that admin kubeconfig.
pkg/asset/manifests/tectonic.go
Outdated
| @@ -54,6 +62,13 @@ func (t *Tectonic) Generate(dependencies asset.Parents) error { | |||
| worker := &machines.Worker{} | |||
| master := &machines.Master{} | |||
| dependencies.Get(installConfig, clusterk8sio, worker, master) | |||
| kubeadminPassword, kubeadminPasswordHash, err := generateRandomPasswordHash() | |||
There was a problem hiding this comment.
It doesn't seem quite right to me that the kube admin password is generated as part of the tectonic manifests asset. Should it be a separate asset that the tectonic manifests asset depends upon?
There was a problem hiding this comment.
this is a secret that is required by the oauth server to authenticate a pre-identity-provider-setup user with an oauth token. The password could be generated elsewhere but the secret should be created during the tectonic service, i think. I'm going to move the generatePassword function to generate a new asset, KubeadminPassword and write to ${CLUSTER_DIR}/kubeadmin-password
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
2 times, most recently
from
5a496d3
to
8a13740
Compare
openshift-ci-robot
added
size/XL
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
from
1637a1f
to
8d3b8c3
Compare
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
2 times, most recently
from
277a7c9
to
b7a38a2
Compare
|
/test e2e-aws |
sallyom
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
@wking @abhinavdahiya @staebler let me explain the requirements that have been outlined by @smarterclayton and @derekwaynecarr:
This should make it clear that this is not a day two activity - it is expected to always be there. No configuration from the user is required to make it happen (IDP or otherwise). It is opt-out, not opt-in. We also know that this must be done via an OAuth flow as that is the only thing the web console / Prometheus / etc support. The requirements from @openshift/sig-auth are:
We have a host of other requirements that I have taken care of inside the openshift OAuth server. |
pkg/asset/installconfig/password.go
Outdated
| // generateRandomPasswordHash generates a hash of a random ASCII 14 char string with at least | ||
| // one digit and one special character. | ||
| func generateRandomPasswordHash() (string, []byte, error) { | ||
| rand.New(rand.NewSource(time.Now().UnixNano())) |
There was a problem hiding this comment.
We should be using crypto/rand for secure random generation. See like https://github.com/openshift/origin/blob/f070bf845040df1f4e8301eeb8f8f037a7030818/pkg/oauthserver/server/crypto/random.go#L16
pkg/asset/installconfig/password.go
Outdated
| // generateRandomPasswordHash generates a hash of a random ASCII 14 char string with at least | ||
| // one digit and one special character. | ||
| func generateRandomPasswordHash() (string, []byte, error) { | ||
| rand.New(rand.NewSource(time.Now().UnixNano())) |
pkg/asset/installconfig/password.go
Outdated
| rand.New(rand.NewSource(time.Now().UnixNano())) | ||
| const ( | ||
| lowercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" | ||
| uppercase = "abcdefghijklmnopqrstuvwxyz" |
There was a problem hiding this comment.
Let us avoid ambiguous characters such as 0 and O - even with the reduced entropy we still have ~100 bits.
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
7 times, most recently
from
b06096f
to
e9d5852
Compare
| Email: emailAddress.EmailAddress, | ||
| Password: password.Password, | ||
| SSHKey: sshPublicKey.Key, | ||
| Password: password.Password, |
There was a problem hiding this comment.
From what I can tell, the entire install config in stored in the kube-system/cluster-config-v1 config map. We do not want the password or the hash stored there.
There was a problem hiding this comment.
ok,.. generating the pw in pkg/asset/password/password.go, and the pw will be stored in the install dir at ${INSTALL_DIR}/kubeadmin-password similar to how aws metadata is written to metadata.json
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
from
cb3b810
to
8f3dce1
Compare
|
@sallyom: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
from
8f3dce1
to
7bd885c
Compare
|
@abhinavdahiya I've addressed your comments. Also, I've moved the asset file to |
@sallyom what is motivation regarding |
sallyom
force-pushed
the
add-oauthadmin-preidp-password-secret
branch
from
7bd885c
to
fcd8f73
Compare
@abhinavdahiya |
would it possible to link to that discussion? @enj |
so it looks like the name was
i'll let @wking give the |
|
@wking I see your point from #771 (comment) |
So is the no- That's from: question := &survey.Question{
Prompt: &survey.Confirm{
Message: "Add a basic identity provider",
Help: "If selected, this will enable the kubeadmin user used by the bootstrap authenticator.",
Default: true,
},
} |
|
Or maybe no prompt and add a boolean to the |
|
So the defaults should definitely align to what we want to achieve. For the next 4 months, there will be no production clusters installed with this. There will be innumerable kick the tires clusters, followed by a phase where seeding clusters starts to become real, followed by GA. In the next four months or so we need to convince everyone who tries it that we can deliver the whole enchilada. I agree disablement will be useful in the future (service delivery will want something like it). But since disablement is so easy post install I would want to focus on keeping our input options limited. Doing this by default doesn’t meaningfully reduce the security profile of the cluster any more than the default admin kubeconfig does, and this is in many ways a superior flow for showing the coherent story we are being tasked to deliver. We can absolutely refine over the next month or two. |
|
the installer collects two pieces of information today that have no purpose (user and pw). the request is that we can demonstrate a frictionless initial experience for upcoming presentation at KubeCon. If we can accept this change and litigate after if the secret should be injected as a post-install step, let’s litigate that after KubeCon. |
|
the console has already introduced support to alert a user logged in with this identity to configure a proper idp. |
Yup.
I will send an email with details once I get a chance to collect thought. As an aside, I think it would be cool to compile the installer into a web asm module so you could run it from the browser 😄
The web console will have UI to help you add IDPs and remove the secret.
Yes.
It is basically "whatever provisioned the cluster" since it cannot be done on the cluster.
If you are new to kube/openshift, something like the web console is far easier to get started with. And to do that, you need to be able to login as some user.
Anything that provisions a cluster and wants to have a nice experience in the console would be expected to create this secret. The installer just happens to be the provisioner we care about.
While we certainly make it obvious to people that they should remove it, it poses no security risk to the cluster. Nothing is going to brute force that password.
I believe you are looking at this incorrectly. This user is no different than the |
This what the admin kubeconfig allows you to assert. It doesn't have to be a single binary.
Wouldn't these folks mostly want to use openshift.io or some other hosted offering? Or your asm module? ;)
It's also just exposure. What if the backing code has bugs? What if someone finds a way to push this one secret and escalate themselves to admin status?
I was thinking "install
That is terrifying. Is there an issue tracking it? Anyhow, I think opt-out would be an easy add, but yeah, we can land this now and revisit later. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ericavonb, sallyom, wking 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 |
openshift-ci-robot
added
the
approved
This (really more this: openshift/origin#21580)configures an initial pre-identity-provider-setup user that can access registry, prometheus, web-console, grafana, while at the same time does not create a role/identity/etc that will have to be cleaned up once an admin does set up an idp. This PR generates a bcrypted randomly generated password rather than any password a user supplies.
@enj has more info about the kubeadmin initial user.
The password/hash is a new asset,
KubeadminPasswordwith a file${CLUSTER_DIR}/kubeadmin-password(similar to metadata.json generation). Also, I removed the unused Admin.Email. This unblocks access to ui-token based components, while we can have cross-team discussion to iterate upon the best place to create/store/log this secret is.@enj @mrogers950 what else has to happen to enable this initial user? I see openshift/origin#21580 merged.