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

Deprecate and remove "implicit" oauth flow #4705

Closed
nrfox opened this issue Feb 3, 2022 · 5 comments · Fixed by kiali/kiali.io#612 or #5679
Closed

Deprecate and remove "implicit" oauth flow #4705

nrfox opened this issue Feb 3, 2022 · 5 comments · Fixed by kiali/kiali.io#612 or #5679
Assignees
Labels
backlog Triaged Issue added to backlog enhancement This is the preferred way to describe new end-to-end features. maintenance Development tasks, like refactoring, performance improvement, any non-user facing changes.

Comments

@nrfox
Copy link
Contributor

nrfox commented Feb 3, 2022

Kiali currently supports both "implicit" and "authorization code" oauth flows. There are security benefits to using the authorization code flow vs. the implicit flow. In addition, moving to the auth code flow would allow Kiali to take advantage of the widely used go-oidc lib.

Pros:

  • Improved security
  • Less to maintain
  • Less to test

Cons:

  • Would break providers who only support the "implicit" flow. (I'm not sure these exist)
  • May require additional configuration for existing open id users
@nrfox nrfox added enhancement This is the preferred way to describe new end-to-end features. maintenance Development tasks, like refactoring, performance improvement, any non-user facing changes. labels Feb 3, 2022
@israel-hdez
Copy link
Member

Perhaps, we should first deprecate it with a warning in the login form. And some time later, remove it.
Just to give people a chance to do proper configs, if any.

@nrfox
Copy link
Contributor Author

nrfox commented Feb 4, 2022

Perhaps, we should first deprecate it with a warning in the login form. And some time later, remove it.
Just to give people a chance to do proper configs, if any.

That sounds like a good plan. We could even put the implicit flow behind a feature flag that is disabled by default after giving a warning on the login form. Either way, would be good to wait X versions before completely removing since some folks won't upgrade sequentially but jump to the latest version from their current version.

@jmazzitelli
Copy link
Collaborator

This might also break the molecule tests. So just a reminder that we should ensure we still have those molecule tests that ensure we at least test the basics of our auth (we have openshift, openid, token, and header login tests)

@jshaughn jshaughn added the backlog Triaged Issue added to backlog label Dec 9, 2022
@jshaughn
Copy link
Collaborator

jshaughn commented Dec 9, 2022

This should be deprecated as of v1.60, we need to update the release notes at this time. Then we should deprecate as soon as we've given folks enough time to respond to the deprecation.

@israel-hdez
Copy link
Member

Oops. I'll re-open. I mistakenly wrote "Fixes" in a comment in one of the related PRs.
However, deprecation notes are in place, but removal is not yet done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Triaged Issue added to backlog enhancement This is the preferred way to describe new end-to-end features. maintenance Development tasks, like refactoring, performance improvement, any non-user facing changes.
Projects
4 participants