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

Option to add staticPasswords from environment variables #1601

Merged
merged 7 commits into from
Jan 30, 2020

Conversation

krishnadurai
Copy link
Contributor

@krishnadurai krishnadurai commented Dec 14, 2019

This PR introduces the ability to set StaticPasswords with field 'hashFromEnv'.

Partially implements requirements from #1099.

Relates to kubeflow/kfctl#130

/cc @ericchiang @rithujohn191 @sagikazarmark @bonifaido

@krishnadurai
Copy link
Contributor Author

@ericchiang @rithujohn191 @sagikazarmark @bonifaido could any one of you PTAL?

@krishnadurai
Copy link
Contributor Author

Circling back in here again to see if we could please get this reviewed once.

@ericchiang @rithujohn191 @sagikazarmark @bonifaido

@ericchiang
Copy link
Contributor

I am no longer a dex maintainer. Please stop cc'ing me :)

@sagikazarmark
Copy link
Member

@krishnadurai we've received your PR, but as the holidays are coming, it probably won't be reviewed until next year.

@sagikazarmark sagikazarmark self-requested a review December 20, 2019 03:09
@krishnadurai
Copy link
Contributor Author

@sagikazarmark thanks for replying. Sorry for being impatient about it. We could wait until the holidays end. :-)

@bonifaido bonifaido self-requested a review December 20, 2019 08:00
Copy link

@jenting jenting left a comment

Choose a reason for hiding this comment

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

LGTM. But it needs to resolve the conflict file.

@krishnadurai
Copy link
Contributor Author

@sagikazarmark this is ready for a look.

@krishnadurai
Copy link
Contributor Author

@sagikazarmark Could we please move this PR forward?

@sagikazarmark
Copy link
Member

@krishnadurai I've looked at your PR, it looks OK implementation-wise, but I'm not sure I would ever use this feature from an operational standpoint.

Managing users in a static file already presents a lot of limitations and issues (eg. security) and I understand this is an attempt improve that last bit, but I'm not 100% it's actually an improvement.

Can you tell me a bit more about how you imagine using this feature? Or in what cases would you consider recommending it?

Thanks!

@k0da
Copy link

k0da commented Jan 16, 2020

I'd love to have this feature, so far we awaited for such feature to move secrets into k8s secretes, k8s secrets are perfectly managed by kustomize and vault plugin.

Of course, better option would be an operator, but I'm not aware of any existing yet.

@krishnadurai
Copy link
Contributor Author

@sagikazarmark We use Dex in Kubeflow. One of our setups is a basic authentication - based setup. We are keen on using staticPasswords field for setting up basic auth with Dex for Kubeflow.
The challenge with the configuration file, as you've mentioned, is that the credentials are exposed. In Kubeflow we have a 'KfDef' based yaml definition, as shown in this file.

Lines
https://github.com/kubeflow/manifests/blob/9aae81d93b78b3fe802527b084530672f39e2b1a/kfdef/kfctl_istio_dex.yaml#L112-L113

Show that this committed configuration has secrets.

We are looking to accept environment variables through our deployment plugin (kubeflow/kfctl) which would set this environment variable on the Dex controller pod programmatically.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

@krishnadurai well, I'm still not convinced if this is the best solution, but it's a solution, it's already done and looks OK to me.

@sagikazarmark sagikazarmark added this to the v2.22.0 milestone Jan 20, 2020
@krishnadurai
Copy link
Contributor Author

@sagikazarmark thanks for considering it @sagikazarmark!

@krishnadurai
Copy link
Contributor Author

@bonifaido It looks like this requires your review as well. Could you please take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants