-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add password hashing #51
Conversation
install/install.go
Outdated
if err != nil { | ||
log.Fatal(err) | ||
} | ||
secretName = strings.Trim(secretName, "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secret name is specified in config map, so user might specify config map and provide different secret name here. Please use default secret name here or secret name from provided config map instead of asking user to retype it.
configMap, err := kubeclientset.CoreV1().ConfigMaps(i.Namespace).Get(i.InstallOptions.ConfigMap, metav1.GetOptions{}) | ||
errors.CheckError(err) | ||
|
||
secretNameOverride, ok := configMap.Data[util.RootCredentialsSecretNameKey] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for asking to make changes again :) . Even if configMap is not supplied then configMap with default name might exist and it might have non-default admin secret name. If user requested to create secret with password installer needs to read either default config map or config map with the provided name. If map does not exist, then default secret name might be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexmt to resolve this, I could see either just relying on the default secret name (ignoring the possibility of a default config map), or I could see us adding a default config map feature. I'm concerned that if we take the second route, it might actually go more smoothly if that's a follow up feature after some careful planning, because we have design decisions to make then (do we insist on there always being a default config map? when do we create it? for what is it consulted?). Interested in your thoughts!
LGTM |
To help us continue #29, allow specification of an admin username and password, as well as a namespace to store the secret, and hash passwords. These are now stored in running memory in a
ArgoCDSettings.LocalUsers
, which gives us flexibility to add more users later.In the process, add a password hasher very loosely inspired by the state of Django's password hashing several years ago, with a prioritized list of preferred hashing algorithms (only one, Bcrypt, is currently implemented, based on an implementation from Argo.
@alexmt this queries for the secret name when running
argocd install
. I realize that with./argocd install --config-map <config map name> --config-superuser
, we will have the secret name in the config map specified, but if we don't have the config map name, we won't—is it worth seeing if we can skip the namespace input when the config map is specified, but including it when a config map is omitted?@alexmt One other thing: it seems that
argocd-server
might start up with thedefault
namespace if a namespace is not specified. Should it default toargocd
, or is that namespace for the command-line tools?