Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

make security requirements optional #56

Merged
merged 2 commits into from
Oct 14, 2021
+91 −72
Merged

make security requirements optional #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87f5b4f
make security requirements optional
eguzki Oct 7, 2021
e54d4eb
fix apiproduct status field for optional resources
eguzki Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions apis/networking/v1beta1/apiproduct_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,11 @@ type APIProductSpec struct {
// platform, short-names can also be used instead of a FQDN (i.e. has no
// dots in the name). In such a scenario, the FQDN of the host would be
// derived based on the underlying platform.
Hosts []string `json:"hosts"`
SecurityScheme []*SecurityScheme `json:"securityScheme"`
Hosts []string `json:"hosts"`

// Configure authentication mechanisms
// +optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed for controller-gen to make it optional? I usually rely only on omitempty and it does the trick, but I'd love to learn about any other extra advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have experienced that too. That's confusing as the kubebulder doc says it should be the '+optional marker.

To make it explicit and easy to read, I always add it for optional fields

Copy link
Contributor

@guicassolato guicassolato Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps +kubebuilder:validation:Optional then, so there's no doubt it's for kubebuilder?

SecurityScheme []SecurityScheme `json:"securityScheme,omitempty"`

// The list of kuadrant API to be protected
// +kubebuilder:validation:MinItems=1
Expand Down Expand Up @@ -238,6 +241,10 @@ func (a *APIProduct) IsPreAuthRateLimitEnabled() bool {
a.GlobalRateLimit() != nil
}

func (a *APIProduct) HasSecurity() bool {
return len(a.Spec.SecurityScheme) > 0
}

func (a *APIProduct) HasAPIKeyAuth() bool {
for _, securityScheme := range a.Spec.SecurityScheme {
if securityScheme.APIKeyAuth != nil {
Expand Down
8 changes: 2 additions & 6 deletions apis/networking/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion config/crd/bases/networking.kuadrant.io_apiproducts.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ spec:
type: object
type: object
securityScheme:
description: Configure authentication mechanisms
items:
properties:
apiKeyAuth:
Expand Down Expand Up @@ -161,7 +162,6 @@ spec:
required:
- APIs
- hosts
- securityScheme
type: object
status:
description: APIProductStatus defines the observed state of APIProduct
Expand Down
2 changes: 1 addition & 1 deletion controllers/apiproduct_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func apiProduct(ns string) *networkingv1beta1.APIProduct {
Prefix: &dogsPrefix,
},
},
SecurityScheme: []*networkingv1beta1.SecurityScheme{
SecurityScheme: []networkingv1beta1.SecurityScheme{
{
Name: "MyAPIKey",
APIKeyAuth: &networkingv1beta1.APIKeyAuth{
Expand Down
27 changes: 17 additions & 10 deletions pkg/authproviders/authorino/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ func buildAuthConfig(apip *networkingv1beta1.APIProduct) *authorino.AuthConfig {
},
}

if !apip.HasSecurity() {
common.TagObjectToDelete(authConfig)
return authConfig
}

for _, securityScheme := range apip.Spec.SecurityScheme {
identity := authorino.Identity{
Name: securityScheme.Name,
Expand Down Expand Up @@ -153,17 +158,19 @@ func (a *Provider) Status(ctx context.Context, apip *networkingv1beta1.APIProduc
// If any object is missing/not-created, Status returns false.
authConfig := buildAuthConfig(apip)

existingAuthConfig := &authorino.AuthConfig{}
err := a.Client().Get(ctx, client.ObjectKeyFromObject(authConfig), existingAuthConfig)
if err != nil && errors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
if !common.IsObjectTaggedToDelete(authConfig) {
existingAuthConfig := &authorino.AuthConfig{}
err := a.Client().Get(ctx, client.ObjectKeyFromObject(authConfig), existingAuthConfig)
if err != nil && errors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
}
// TODO(jmprusi): No status in authorino serviceConfig objects, yet.
//if !existingServiceConfig.Status.Ready {
// return false, nil
//}
}
// TODO(jmprusi): No status in authorino serviceConfig objects, yet.
//if !existingServiceConfig.Status.Ready {
// return false, nil
//}

return true, nil
}
Expand Down
66 changes: 36 additions & 30 deletions pkg/ingressproviders/istioprovider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (is *IstioProvider) Reconcile(ctx context.Context, apip *networkingv1beta1.
return ctrl.Result{}, err
}

authPolicy, err := is.getAuthorizationPolicy(existingVS)
authPolicy, err := is.getAuthorizationPolicy(apip, existingVS)
if err != nil {
return ctrl.Result{}, err
}
Expand All @@ -121,17 +121,7 @@ func (is *IstioProvider) ReconcileIstioAuthorizationPolicy(ctx context.Context,
return is.ReconcileResource(ctx, &istioSecurity.AuthorizationPolicy{}, desired, mutatefn)
}

func (is *IstioProvider) getAuthorizationPolicy(virtualService *istio.VirtualService) (*istioSecurity.AuthorizationPolicy, error) {
policyHosts := []string{}

// Now we need to generate the list of hosts that will match the authorization policy,
// to be sure, we will match the "$host" and "$host:*".
for _, host := range virtualService.Spec.Hosts {
//TODO(jmprusi): avoid duplicates
hostSplitted := strings.Split(host, ":")
policyHosts = append(policyHosts, hostSplitted[0]+":*")
policyHosts = append(policyHosts, hostSplitted[0])
}
func (is *IstioProvider) getAuthorizationPolicy(apip *networkingv1beta1.APIProduct, virtualService *istio.VirtualService) (*istioSecurity.AuthorizationPolicy, error) {
authPolicy := &istioSecurity.AuthorizationPolicy{
TypeMeta: metav1.TypeMeta{
Kind: "AuthorizationPolicy",
Expand All @@ -141,26 +131,42 @@ func (is *IstioProvider) getAuthorizationPolicy(virtualService *istio.VirtualSer
Name: virtualService.GetName(),
Namespace: common.KuadrantNamespace,
},
Spec: v1beta12.AuthorizationPolicy{
Selector: &v1beta13.WorkloadSelector{
MatchLabels: map[string]string{
"app": "istio-ingressgateway",
},
}

if !apip.HasSecurity() {
common.TagObjectToDelete(authPolicy)
return authPolicy, nil
}

// Now we need to generate the list of hosts that will match the authorization policy,
// to be sure, we will match the "$host" and "$host:*".
policyHosts := []string{}
for _, host := range virtualService.Spec.Hosts {
//TODO(jmprusi): avoid duplicates
hostSplitted := strings.Split(host, ":")
policyHosts = append(policyHosts, hostSplitted[0]+":*")
policyHosts = append(policyHosts, hostSplitted[0])
}

authPolicy.Spec = v1beta12.AuthorizationPolicy{
Selector: &v1beta13.WorkloadSelector{
MatchLabels: map[string]string{
"app": "istio-ingressgateway",
},
Rules: []*v1beta12.Rule{
{
To: []*v1beta12.Rule_To{{
Operation: &v1beta12.Operation{
Hosts: policyHosts,
},
}},
},
},
Rules: []*v1beta12.Rule{
{
To: []*v1beta12.Rule_To{{
Operation: &v1beta12.Operation{
Hosts: policyHosts,
},
}},
},
Action: v1beta12.AuthorizationPolicy_CUSTOM,
ActionDetail: &v1beta12.AuthorizationPolicy_Provider{
Provider: &v1beta12.AuthorizationPolicy_ExtensionProvider{
Name: common.KuadrantAuthorizationProvider,
},
},
Action: v1beta12.AuthorizationPolicy_CUSTOM,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope here, but there's a request for discussing the conventions across components at Kuadrant/authorino#164

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

istio constant :)

ActionDetail: &v1beta12.AuthorizationPolicy_Provider{
Provider: &v1beta12.AuthorizationPolicy_ExtensionProvider{
Name: common.KuadrantAuthorizationProvider,
},
},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/ingressproviders/istioprovider/rate_limit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func postAuthEnvoyFilter(apip *networkingv1beta1.APIProduct) *istionetworkingv1a
Patches: make([]*istioapiv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch, 0),
}

if apip.AuthRateLimit() == nil {
if apip.AuthRateLimit() == nil || !apip.HasSecurity() {
envoyFilter := factory.EnvoyFilter()
common.TagObjectToDelete(envoyFilter)
return envoyFilter
Expand Down
45 changes: 24 additions & 21 deletions pkg/ratelimitproviders/limitador/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,37 +110,40 @@ func (p *Provider) Delete(ctx context.Context, apip *networkingv1beta1.APIProduc
func (p *Provider) Status(ctx context.Context, apip *networkingv1beta1.APIProduct) (bool, error) {
log := p.Logger().WithValues("apiproduct", client.ObjectKeyFromObject(apip))
log.V(1).Info("Status")
if apip.Spec.RateLimit == nil {
return true, nil
}

// Right now, we just try to get all the objects that should have been created, and check their status.
// If any object is missing/not-created, Status returns false.
desiredGlobalRateLimit := p.globalRateLimit(apip)
existing := &limitadorv1alpha1.RateLimit{}
err := p.GetResource(ctx, client.ObjectKeyFromObject(desiredGlobalRateLimit), existing)
if err != nil && apierrors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
if !common.IsObjectTaggedToDelete(desiredGlobalRateLimit) {
existing := &limitadorv1alpha1.RateLimit{}
err := p.GetResource(ctx, client.ObjectKeyFromObject(desiredGlobalRateLimit), existing)
if err != nil && apierrors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
}
}

desiredperRemoteIPRateLimit := p.perRemoteIPRateLimit(apip)
existing = &limitadorv1alpha1.RateLimit{}
err = p.GetResource(ctx, client.ObjectKeyFromObject(desiredperRemoteIPRateLimit), existing)
if err != nil && apierrors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
if !common.IsObjectTaggedToDelete(desiredperRemoteIPRateLimit) {
existing := &limitadorv1alpha1.RateLimit{}
err := p.GetResource(ctx, client.ObjectKeyFromObject(desiredperRemoteIPRateLimit), existing)
if err != nil && apierrors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
}
}

authenticatedAuthRateLimit := p.authenticatedRateLimit(apip)
existing = &limitadorv1alpha1.RateLimit{}
err = p.GetResource(ctx, client.ObjectKeyFromObject(authenticatedAuthRateLimit), existing)
if err != nil && apierrors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
if !common.IsObjectTaggedToDelete(authenticatedAuthRateLimit) {
existing := &limitadorv1alpha1.RateLimit{}
err := p.GetResource(ctx, client.ObjectKeyFromObject(authenticatedAuthRateLimit), existing)
if err != nil && apierrors.IsNotFound(err) {
return false, nil
} else if err != nil {
return false, err
}
}

return true, nil
Expand Down