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

KIALI-2397 if a secret is not yet created, wait for one #883

Merged
merged 2 commits into from Mar 5, 2019

Conversation

@jmazzitelli
Copy link
Contributor

commented Feb 28, 2019

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

This is not ready to merge for two reasons:

  1. This assumes Kiali is installed in the same namespace as Istio - is this an OK assumption? This is assumed here: https://github.com/kiali/kiali/pull/883/files#diff-c19e5972b564f8f7ebd30c662b1974cbR137
if s, err := client.GetSecret(config.Get().IstioNamespace, "kiali"); err == nil {
  1. More importantly than 1 above, this code requires Kiali to be able to access/read secrets in its namespace. Right now, we do not give Kiali that role permission. How can we allow Kiali access to read its secret without giving it permissions to read too much. In my testing, I added "secrets" to Kiali's cluster role - but this is overkill - we do not want to give Kiali that broad of access. We only need to be able to read secrets from its own namespace, and even that might be too broad - we only need to be able to read the kiali secret found in its own namespace.
@jmazzitelli jmazzitelli requested review from lucasponce and mwringe Feb 28, 2019
@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@mwringe @lucasponce see my comment above - particularly point (2) regarding permissions. Any ideas how best to do this?

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2019

Jenkins CI: kiali-core-pr-e2e-test #621
🔴 PRT Failed, Please Contact QE

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

How can we allow Kiali access to read its secret without giving it permissions to read too much. In my testing, I added "secrets" to Kiali's cluster role - but this is overkill - we do not want to give Kiali that broad of access. We only need to be able to read secrets from its own namespace, and even that might be too broad - we only need to be able to read the kiali secret found in its own namespace.

But what happens in scenarios where a user can't read namespace where kiali is deployed ?

I wonder how this will work with @mwringe changes ?

Do we need to level of permissions here ?

I mean, a technical role (i.e. kiali that can always access to istio-system namespace), and a user role which is propagated into the k8s queries to apply RBAC.

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Do we need to level of permissions here ?

That's my question :) What kind of permission do we need to give Kiali (outside of the logged-in user's permissions)?

We have those clusterroles we keep adding to - and like I say, I could add "secrets" to our clusterrole - but that is overkill. We only need permission to read one specific secret in the namespace we are deployed inside of.

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

I wonder how this will work with @mwringe changes ?

This code would not use that stuff at all - this code in this PR is run immediately at kiali startup and has nothing to do with user requests. We'd be connecting to the k8a API using the service account the Kiali pod is running under - and thus the permissions granted to the code in this PR are the permissions granted to that service account.

@mwringe

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

We might need something like an additional role here that is only applied to the service account in the namespace that kiali is deployed into. We don't want to grant kiali access to secrets outside of the namespace its deployed in.

Listening for secret changes is interesting, but I would have thought they would have loaded the secret in a volume and monitored changes to the file (which means you don't need any special permissions)

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Listening for secret changes is interesting, but I would have thought they would have loaded the secret in a volume and monitored changes to the file (which means you don't need any special permissions)

That's another interesting way to implement this feature. Lemme look closer at how we could do that. Today we bind the secret to environment variables and those make their way into our config object internally. But we could make these "special" and read from a file, rather than env variables. If we do that, we could wait for the file to exist rather than wait for a secret to appear in the namespace.

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

Jenkins CI: kiali-core-pr-e2e-test #627

  • ✔️ run-kiali-e2e-tests #[1340]
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

Jenkins CI: kiali-core-pr-e2e-test #628

  • ✔️ run-kiali-e2e-tests #[1341]
@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

I believe this is now ready. This no longer needs permissions to read secrets - it will now rely on the secret getting mounted on the file system. The kiali server will now just wait for the secret to show up on the file system.

NOTE: a recent change previously broke the feature that the login screen will show the error about the missing secret - a commit in this PR fixes that breakage and brings that feature back.

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

Jenkins CI: kiali-core-pr-e2e-test #629

  • ✔️ run-kiali-e2e-tests #[1342]
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

Jenkins CI: kiali-core-pr-e2e-test #630

  • ✔️ run-kiali-e2e-tests #[1343]
@jmazzitelli jmazzitelli force-pushed the jmazzitelli:KIALI-2397-wait-for-secret branch from c5d641d to c20679a Mar 4, 2019
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

Jenkins CI: kiali-core-pr-e2e-test #642

  • ✔️ run-kiali-e2e-tests #[1364]
@mwringe

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Why did we change from using 'password' to using 'passphrase'? Doesn't this mean anyone who is already using existing secrets will have to change them? Or do all of our installers overwrite the secret each time?

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Why did we change from using 'password' to using 'passphrase'?

The names of the secret key labels did not change in this PR - the secret has had a key "passphrase" since November 2018 - see v0.10 and v0.10.

Doesn't this mean anyone who is already using existing secrets will have to change them?

No, secrets remain the same because, as stated above, the secret key name has been "passphrase" since v0.10 (November 2018) release. Nothing changes with existing secrets.

The reason we changed this back in v0.10 was upstream Istio asked that we change it from password to passphrase to conform to more standard security nomenclature. See this as one place where upstream Istio asked that we conform to that nomenclature.

What did change in this PR is the config name of the password in server/credentials configmap. This was to maintain consistency with the Istio nomenclature and the above recommendation to use passphrase. Otherwise, we have a case where secret uses "passphrase" and our optional configmap entry is called "password" - which is confusing and inconsistent. Where this will most likely affect someone is if they are using anonymous logins - when they add a configmap setting server/credentials with "allow_anonymous=true" they need to use "passphrase" consisting of an empty value.

@mwringe

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@gbaufake @mattmahoneyrh Can one of you please take a quick look at this?

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

don't review this. Some recent PR merge caused (again) the missing-secret-error-message to be broke. I'm going to resolve these conflicts and try and fix it again.

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

anonymous access is also now broke again :/

jmazzitelli added 2 commits Feb 28, 2019
…ame/passphrase credentials, wait for a secret to be created
…the user in the log in screen when a secret is missing. This puts that feature back.

# Conflicts:
#	handlers/authentication.go
@jmazzitelli jmazzitelli force-pushed the jmazzitelli:KIALI-2397-wait-for-secret branch from c20679a to c239775 Mar 5, 2019
@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

OK, I need this approved and merged before someone breaks this code again :)

@jmazzitelli jmazzitelli requested a review from israel-hdez Mar 5, 2019
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

Jenkins CI: kiali-core-pr-e2e-test #647

  • ✔️ run-kiali-e2e-tests #[1372]
Copy link
Member

left a comment

Changes to the authentication file look OK.
I'm just wondering why we should have two ways to allow anonymous access.

config/config.go Show resolved Hide resolved
return fmt.Errorf("A username must be provided if a password is set")
}

if c.Username != "" && c.Token != "" {
return fmt.Errorf("Username/password cannot be specified if a token is specified also. Only Username/Password or Token can be set but not both")
return fmt.Errorf("Username/passphrase cannot be specified if a token is specified also. Only Username/Passphrase or Token can be set but not both")

This comment has been minimized.

Copy link
@israel-hdez

israel-hdez Mar 5, 2019

Member

Is token authentication used only in tests?
I don't see how token auth is being supported in normal scenarios.

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Mar 5, 2019

Author Contributor

That token stuff is a remnant of the original config / authentication stuff. It was in place in case we wanted to support Bearer token logins. There was bearer token look ups recently, but I think that code was removed recently (I forget by who and in what PR). For now, I think this token stuff is not used in the actual code but is supported for future use. The tests do test for valid config surrounding the token.

} else {
RespondWithCode(w, http.StatusUnauthorized)
return false
}

This comment has been minimized.

Copy link
@israel-hdez

israel-hdez Mar 5, 2019

Member

Ok, this invalidates my previous comment about AllowAnonymous not being referenced.
But, then, we have two ways of allowing anonymous access:

  • By setting the SERVER_ALLOW_ANONYMOUS_ACCESS env var in the deployment
  • And by doing AUTH_STRATEGY=anonymous deploy/openshift/deploy-kiali-to-openshift.sh

For me, it doesn't make sense to have two ways to allow anonymous access. We should get rid of one of them -- This was changed in the OAuth work, so I though we were preferring the AUTH_STRATEGY way.

Besides that, the check for the presence of the secret looks OK.

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Mar 5, 2019

Author Contributor

I don't know why the strategy/anonymous work was added. the anonymous access was already there in the login stuff before the strategy work existed. We could eliminate one or the other in another JIRA/PR. This only returns the functionality back to the way it was.

JIRA added: https://issues.jboss.org/browse/KIALI-2503

kiali.go Show resolved Hide resolved
@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I'm just wondering why we should have two ways to allow anonymous access.

As mentioned earlier, the original anonymous access was done through the login stuff - this was in place before the oauth/strategy stuff was even implemented. But it seems when the strategy stuff was implemented, it also included an anonymous mode. We can create another JIRA to address this in another PR.. ."pick one method of anonymous use and eliminate the other one"

@jmazzitelli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

JIRA to pick an anonymous mode implementation and remove the other: https://issues.jboss.org/browse/KIALI-2503

Copy link
Member

left a comment

OK, I'm fine with the changes and to choose an anonymous mode later.

@jmazzitelli jmazzitelli merged commit 9b2e0a9 into kiali:master Mar 5, 2019
3 checks passed
3 checks passed
Jenkins-CI Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - tests/e2e/requirements.txt (theute) No manifest changes detected
@jmazzitelli jmazzitelli deleted the jmazzitelli:KIALI-2397-wait-for-secret branch Mar 5, 2019
israel-hdez added a commit to israel-hdez/swscore that referenced this pull request May 16, 2019
Reported in kiali#1059 (comment).

Support for credentials in config.yaml was removed in PR kiali#883. So this looks safe to remove, since credentials in config.yaml are no longer honored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.