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

authorize: do not redirect if invalid client cert #4344

Merged
merged 2 commits into from Jul 10, 2023

Conversation

kenjenkins
Copy link
Contributor

@kenjenkins kenjenkins commented Jul 6, 2023

Summary

If an authorization policy requires a client certificate, but an incoming request does not include a valid certificate, we should serve a deny error page right away, regardless of whether the user is authenticated via the identity provider or not. Do not redirect to the identity provider login page in this case.

Update the existing integration tests accordingly.

Related issues

User Explanation

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

If an authorization policy requires a client certificate, but an
incoming request does not include a valid certificate, we should serve a
deny error page right away, regardless of whether the user is
authenticated via the identity provider or not. Do not redirect to the
identity provider login page in this case.

Update the existing integration tests accordingly.
@coveralls
Copy link

coveralls commented Jul 6, 2023

Coverage Status

coverage: 63.432% (-0.02%) from 63.455% when pulling f5e1875 on kenjenkins/client-cert-deny into 74e6486 on main.

@kenjenkins kenjenkins marked this pull request as ready for review July 7, 2023 23:35
@kenjenkins kenjenkins requested a review from a team as a code owner July 7, 2023 23:35
@kenjenkins kenjenkins requested a review from wasaga July 7, 2023 23:35
@kenjenkins
Copy link
Contributor Author

I'm not sure if this is the best way to do this. I wonder if it's feasible to express this logic directly in the generated Rego policy (i.e. rather than interpreting specific allow/deny reasons returned by the Rego policy, introduce a way for the Rego policy to directly return a result like "needs sign in" or "needs WebAuthn enrollment").

@kenjenkins kenjenkins merged commit 5459e69 into main Jul 10, 2023
10 checks passed
@kenjenkins kenjenkins deleted the kenjenkins/client-cert-deny branch July 10, 2023 23:39
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.

Do not redirect to IdP login when client certificate is required but not present/valid
3 participants