Autpolicy doc fixes#76
Conversation
| @@ -41,21 +45,17 @@ components: | |||
| openIdConnectUrl: https://example.com/.well-known/openid-configuration | |||
There was a problem hiding this comment.
This particular example is also broken and we should not be sharing it without at least a similar warning to the user as this one proposed further below.
Explanation:
Copying a well-known OpenId Connect Discovery URL to the AuthPolicy's jwt.issuerUrl field will not work.
That's why kuadrantctl actually expects in the openIdConnectUrl field of the OAS an issuer URL identifier instead. This is a URL without the /.well-known/openid-configuration part, which is automatically appended to the value obtained from jwt.issuerUrl.
Users who follow the OAS docs though may inadvertently use the full Discovery URL. We do not want that.
There was a problem hiding this comment.
This is a good catch! Please look at my next commit a9db0d0
I removed the mention of the /.well-known/openid-configuration path so it should be less confusing.
I am just wondering if there is a reason why AuthPolicy expects issuer instead of openid-configuration path?
The response JSON from openid-configuration contains value of issuer so theoretically it could be used as well. Also if the current practice is to use openid-configuration path in OAS yaml-s as you mentioned.
There was a problem hiding this comment.
I am just wondering if there is a reason why AuthPolicy expects
issuerinstead ofopenid-configurationpath?
It's because of the OIDC library used by Authorino. In its simplest form, it expects an issuer URL and not the (more specific) Discovery endpoint. Otherwise, Authorino itself would have to perform the request to fetch the OIDC config, parse its response, handle possible exceptions, and then start using the library that, among other things, let us validate signed JWTs.
I would advocate for more flexibility in Authorino's JWT verification method, which surfaces into the Kuadrant AuthPolicy. Along with issuerUrl, I'd love to add discoveryUrl, jwksURL, and possibly others. It's just that currently this is not the state of things... yet.
R-Lawton
left a comment
R-Lawton
left a comment
There was a problem hiding this comment.
havent ran through the tutorial yet but from a high level found a few things
| ``` | ||
| kuadrantctl generate kuadrant authpolicy --oas ./petstore-openapi.yaml | yq -P | ||
| ```bash | ||
| kuadrantctl generate kuadrant authpolicy --oas example.yaml |
There was a problem hiding this comment.
i dont really get this change, why change the name to be example and why use remove the pretty print?
There was a problem hiding this comment.
The yp command has been removed from multiple guides as is now not needed. The pretty print is done as default.
I changed the name of the yaml file so it does not clash with the same name as the yaml file generated as part of User guide that follows later. I am opened to better file name suggestion though.
|
|
||
| ``` | ||
| kuadrantctl generate kuadrant authpolicy --oas ./petstore-openapi.yaml | yq -P | ||
| ```bash |
| </details> | ||
|
|
||
| > Replace `${KEYCLOAK_PUBLIC_DOMAIN}` with your SSO instance domain | ||
| > Replace `${KEYCLOAK_ISSUER}` with your SSO instance issuer endpoint for your `petstore` realm. |
There was a problem hiding this comment.
What do you think about moving this to be above the resource that uses it so they know to set it before they create the resource it falls more in line with other docs we have
|
|
||
| > Replace `${KEYCLOAK_PUBLIC_DOMAIN}` with your SSO instance domain | ||
|
|
||
| > Replace `${KEYCLOAK_TOKEN_ENDPOINT}` with your SSO instance token endpoint for your `petstore` realm. |
There was a problem hiding this comment.
Same like the other var moving to be above where its needed so people can set it then run the bash command
5 participants
Contains fixes:
yqremovalINGRESS_IPis needed for curl examples