-
Notifications
You must be signed in to change notification settings - Fork 14
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
generate kuadrant authconfig #37
Conversation
bfe9238
to
e93c618
Compare
return authConfig, nil | ||
} | ||
|
||
func generateKuadrantAuthConfigMetadata(doc *openapi3.T) ([]*authorinov1beta1.Metadata, error) { |
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.
what are the empty methods for wondering what the metadata concept is?
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.
placeholders if we ever need to generate them. We can remove them and assign the value nil
in the struct
// - toystore_oidc: [] | ||
// | ||
|
||
// scopes not being used now |
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.
do you see a use case for scopes in the future? Are you thinking it might make up part of authorization? Or would we leave that entirely up to the user to implement?
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.
It is actually part of the spec, so the command should generate required config to validate the scopes. The authorino implementation would be in the authorization phase validating that the user doing the request has granted those scopes.
I think we can leave this for another PR. Definitely is a missing feature from the command
switch secScheme.Type { | ||
case "apiKey": | ||
AuthConfigIdentityFromApiKeyScheme(identity, secScheme) | ||
case "openIdConnect": |
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.
I may have missed it on the call earlier. Are these the only two schemas in open API?
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.
No, there are more:
For OpenAPI 3.0.X it is "apiKey", "http", , "oauth2", "openIdConnect"
.
OpenAPI 3.1 added "mutualTLS"
For the first implementation, apiKey and oidc. Others can be implemented either as authorizationpolicies or with authorino (authorino does not implement, yet, mtls)
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.
I think oauth2
could be supported combining Envoy's OAuth2 filter and either Authorino's OIDC or OAuth2 Token Introspection.
http
can be modelled on top of Authorino API key authn. See this user guide for ref.
And, yes, mutualTLS
soon to be supported in Authorino, but not yet.
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.
This looks really good and simple to me!
pkg/authorino/auth_config.go
Outdated
return []authorinov1beta1.JSONPattern{ | ||
{ | ||
JSONPatternExpression: authorinov1beta1.JSONPatternExpression{ | ||
Selector: "context.request.http.path", |
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.
A trick might be needed here when the OAS securitySchemes.{name}.in == "query"
.
As query string params are part of path (as supplied by Envoy), we may need to strip them out or the condition won't match. To do so, you can use a string modifier:
context.request.http.path.@extract{"sep":"/"}
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.
ok, that is unexpected. Good catch.
I will fix that
@eguzki tried out the example and looks good. One thing is it created a status block in the authconfig is that expected? |
// Fixed label selector for now | ||
apikey := authorinov1beta1.Identity_APIKey{ | ||
LabelSelectors: map[string]string{ | ||
"authorino.kuadrant.io/managed-by": "authorino", |
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.
How can users be more restrictive here?
The way it is right now, virtually any API key secret stored in a cluster can be used to authenticate to any API protected with Authorino deployed by Kuadrant in this cluster.
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.
I will add some more to be more restrictive
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.
Added a new label app = {title of the openapi satinized}
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.
Anyway, the CLI can have some default values.. as a user I would expect to override them if I needed.
For another PR (good exercise to somebody wanting to learn) add CLI optional parameters to override default labels.
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.
Probably a question for Authorino. but should it not be limited to looking in the same namespace as the authconfig? Seems odd I would protect my API with secret in a different namespace?
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.
That depends on the deployment mode of authorino. As kuadrant deployment, it will be cluster scoped controller, so secrets on any namespace are valid as long as they have the required labels.
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.
It seems odd to me though, that I can create an authconfig in namespace A with a secret label selector "app":"myapp" and someone working in a different namespace can create a secret in their namespace with the same selector and then get access to my API? They don't need to know the API Key or contents of the secret they just need to be able to guess the label selector. This is not related to the PR though so lets discuss separately
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.
Codecov Report
@@ Coverage Diff @@
## generate-istiointegration #37 +/- ##
=============================================================
- Coverage 30.39% 29.60% -0.79%
=============================================================
Files 19 21 +2
Lines 1020 1118 +98
=============================================================
+ Hits 310 331 +21
- Misses 659 733 +74
- Partials 51 54 +3
Continue to review full report at Codecov.
|
Because the It should no do any harm to create the object with the There is a way to have the CLI remove the status field. After serialization, unserialize with unstructured types (maps and lists), then, remove the status with a |
Thanks for the reviews. This PR can be merged after #36 |
Co-authored-by: Guilherme Cassolato <[email protected]>
Co-authored-by: Guilherme Cassolato <[email protected]>
Co-authored-by: Guilherme Cassolato <[email protected]>
81f7664
to
7d6edc4
Compare
New command to generate AuthConfig objects from your OpenAPI spec.
Running example:
Using the following OpenAPI spec that contains three operations:
The generated AuthConfig would be: