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

Applying v2alpha1 APIRule blocks in-cluster communication to the target #1632

Open
6 tasks
barchw opened this issue Jan 22, 2025 · 10 comments
Open
6 tasks

Applying v2alpha1 APIRule blocks in-cluster communication to the target #1632

barchw opened this issue Jan 22, 2025 · 10 comments
Assignees
Milestone

Comments

@barchw
Copy link
Contributor

barchw commented Jan 22, 2025

Description

Applying v2alpha1 APIRule blocks in-cluster communication to the target, as an AP (AuthorizationPolicy) (action type: ALLOW) is applied in all cases. As logic behind AP blocks all traffic except for a match to the ALLOW AuthorizationPolicy, the internal traffic is blocked unless it is allowed explicitly by a different AP of type ALLOW.

Expected result
Application of v2alpha1 APIRule to a workload should not block in-cluster traffic targeting the workload.

Actual result
Any traffic other than incoming from istio-ingressgateway and matching the APIRule restrictions is blocked.

Steps to reproduce

  1. APIGateway module installed
kubectl apply -f https://github.com/kyma-project/istio/releases/latest/download/istio-manager.yaml
kubectl apply -f https://github.com/kyma-project/istio/releases/latest/download/istio-default-cr.yaml

kubectl apply -f https://github.com/kyma-project/api-gateway/releases/latest/download/api-gateway-manager.yaml
kubectl apply -f https://github.com/kyma-project/api-gateway/releases/latest/download/apigateway-default-cr.yaml
  1. A target workload present (and Istio injected)
export NAMESPACE_NAME=sidecar-enabled
kubectl create ns ${NAMESPACE_NAME}
kubectl label namespace $NAMESPACE_NAME istio-injection=enabled --overwrite
kubectl -n ${NAMESPACE_NAME} apply -f https://raw.githubusercontent.com/istio/istio/master/samples/httpbin/httpbin.yaml
  1. An APIRule created, targeting the workload, handler is irrelevant (can be e.g. noAuth)
apiVersion: gateway.kyma-project.io/v2alpha1
kind: APIRule
metadata:
  name: "httpbin"
  namespace: sidecar-enabled
spec:
  gateway: "kyma-system/kyma-gateway"
  hosts:
    - "httpbin"
  service:
    name: httpbin
    port: 8000
  rules:
    - path: /anything/{**}
      methods: ["GET"]
      noAuth: true
  1. Internal traffic to the httpbin pod will be blocked

PR

DoD [Developer & Reviewer]

  • Provide unit and integration tests.
  • Provide documentation.
  • Verify if the solution works for both open-source Kyma and SAP BTP, Kyma runtime.
  • If you changed the resource limits, explain why it was needed.
  • Verify that your contributions don't decrease code coverage. If they do, explain why this is the case.
  • Add release notes.
@strekm strekm added this to the 2.10.3 milestone Jan 22, 2025
@mluk-sap
Copy link
Contributor

mluk-sap commented Jan 23, 2025

We discussed it during the refinement:

  • let's add AP that allows internal traffic except Ingress controller, like:
apiVersion: security.istio.io/v1
kind: AuthorizationPolicy
metadata:
  name: allow-internal
  namespace: sidecar-enabled
spec:
  selector:
    matchLabels:
      app: httpbin
  action: ALLOW
  rules:
  - from:
    - source:
        notPrincipals: ["cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account"]

@I551317
Copy link

I551317 commented Jan 27, 2025

We discussed it during the refinement:

  • let's add AP that allows internal traffic except Ingress controller, like:
apiVersion: security.istio.io/v1
kind: AuthorizationPolicy
metadata:
  name: allow-internal
  namespace: sidecar-enabled
spec:
  selector:
    matchLabels:
      app: httpbin
  action: ALLOW
  rules:
  - from:
    - source:
        notPrincipals: ["cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account"]

Out of my curiosity. If you are about to extend current implementation by adding a new AP with "notPrincipal" set, wouldn't it be simpler and cleaner just to remove the prior AuthorizationPolicy (with ALLOW for that Principal)? Wouldn't that be the same as no having AP at all? Are there any pros of such an approach that I'm missing?

@barchw
Copy link
Contributor Author

barchw commented Jan 27, 2025

@I551317 This unfortunately would not have the same outcome for all supported APIRule cases.
If there would be no AP created for the noAuth policy, creating any other AP that would target the same workload (for example to expose some particular path using JWT strategy) would end up blocking the traffic incoming to the path exposed with noAuth.

For example imagine the following configuration if there would be no AP created for noAuth:

  rules:
    - path: /headers
      methods: ["GET"]
      jwt:      
        authentications:
        - issuer: https://example.com
          jwksUri: https://example.com
    - path: /anything/{**}
      methods: ["GET"]
      noAuth: true

In this case, APIRule would create an AP for JWT exposed /headers path, that would block access to the noAuth /anything/{**} path as a side-effect (keeping in mind, that the policies are evaluated per workload, not path). This is because if any ALLOW type AP exists, the request needs to match at least one.

Istio AP docs:
3. If there are no ALLOW policies for the workload, allow the request.
4. If any of the ALLOW policies match the request, allow the request.
5. Deny the request.

The case where only a noAuth path exists is a special, as it technically does not require an AP when there is nothing else targeting this workload. However, for the sake of a consistent behaviour across different scenarios (like the above JWT scenario) we presume it is better to explicitly create an AP for the noAuth path every time.

@I551317
Copy link

I551317 commented Jan 27, 2025

Thanks Bartosz for the explanation. Indeed, that's what I missed - there are other scenarios than 'noAuth: true' that may still need AP and interfere with that solution. Clear to me.

@barchw barchw self-assigned this Jan 29, 2025
@strekm strekm modified the milestones: 2.10.3, 2.10.4 Jan 30, 2025
@barchw barchw removed their assignment Feb 5, 2025
@mluk-sap
Copy link
Contributor

mluk-sap commented Feb 7, 2025

Okay, I tested the PR with the following script:

#/bin/bash
set -e
set -o pipefail
set -x

k3d cluster delete kyma
k3d cluster create kyma --k3s-arg '--tls-san=host.docker.internal@server:*' --agents 2 --k3s-arg '--disable=traefik@server:0' -p 80:80@loadbalancer -p 443:443@loadbalancer 

export IMG=api-gateway-manager:$(date +%s)
make build
make docker-build 
k3d image import -c kyma "$IMG"

make install-istio
make deploy

export NAMESPACE=workload
kubectl create ns $NAMESPACE || true
kubectl label namespace $NAMESPACE istio-injection=enabled --overwrite

export DOMAIN_TO_EXPOSE_WORKLOADS=local.kyma.dev
export SERVICE_NAME=httpbin
export GATEWAY=$NAMESPACE/httpbin-gateway

echo "deploying apigateway"
cat <<EOF | kubectl -n $NAMESPACE apply -f -
apiVersion: operator.kyma-project.io/v1alpha1
kind: APIGateway
metadata:
  name: default
spec:
  enableKymaGateway: true
EOF
echo "apigateway deployed"

echo "waiting for apigateway being ready"
kubectl wait apigateway default --for=jsonpath='{.status.state}'=Ready --timeout=120s
echo "apigateway is ready"

echo "deploying app"
cat <<EOF | kubectl -n $NAMESPACE apply -f -
apiVersion: v1
kind: ServiceAccount
metadata:
  name: $SERVICE_NAME
---
apiVersion: v1
kind: Service
metadata:
  name: $SERVICE_NAME
  labels:
    app: httpbin
    service: httpbin
spec:
  ports:
  - name: http
    port: 8000
    targetPort: 80
  selector:
    app: httpbin
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: $SERVICE_NAME
spec:
  replicas: 1
  selector:
    matchLabels:
      app: httpbin
      version: v1
  template:
    metadata:
      labels:
        app: httpbin
        version: v1
    spec:
      serviceAccountName: $SERVICE_NAME
      containers:
      - image: docker.io/kennethreitz/httpbin
        imagePullPolicy: IfNotPresent
        name: httpbin
        ports:
        - containerPort: 80
EOF
echo "app deployed"

echo "waiting for pods"
kubectl wait pods -n "$NAMESPACE" -l app=httpbin  --for=condition=ready
echo "pods ready"

cat <<EOF | kubectl apply -f -
---
apiVersion: networking.istio.io/v1alpha3
kind: Gateway
metadata:
  name: httpbin-gateway
  namespace: $NAMESPACE
spec:
  selector:
    istio: ingressgateway
  servers:
    - port:
        number: 80
        name: http
        protocol: HTTP
      hosts:
        - "*.$DOMAIN_TO_EXPOSE_WORKLOADS"
EOF

echo "applying apirule v1"
cat <<EOF | kubectl apply -f -
apiVersion: gateway.kyma-project.io/v1beta1
kind: APIRule
metadata:
  name: httpbin
  namespace: $NAMESPACE
spec:
  host: httpbin.$DOMAIN_TO_EXPOSE_WORKLOADS
  service:
    name: $SERVICE_NAME
    namespace: $NAMESPACE
    port: 8000
  gateway: $GATEWAY
  rules:
    - path: /.*
      methods: ["GET"]
      accessStrategies:
        - handler: no_auth
    - path: /post
      methods: ["POST"]
      accessStrategies:
        - handler: no_auth
EOF

echo "waiting for apirule status OK"
kubectl wait -n "$NAMESPACE" apirule httpbin --for=jsonpath='{.status.APIRuleStatus.code}' --timeout=60s
echo "apirule status OK"

echo "applying apirule v2"
cat <<EOF | kubectl apply -f -
apiVersion: gateway.kyma-project.io/v2alpha1
kind: APIRule
metadata:
  name: httpbin
  namespace: $NAMESPACE
spec:
  hosts:
  - httpbin.$DOMAIN_TO_EXPOSE_WORKLOADS
  service:
    name: $SERVICE_NAME
    namespace: $NAMESPACE
    port: 8000
  gateway: $GATEWAY
  rules:
    - path: /*
      methods: ["GET"]
      noAuth: true
    - path: /post
      methods: ["POST"]
      noAuth: true
EOF

echo "waiting for apirule status OK"
kubectl wait -n "$NAMESPACE" apirule httpbin --for=jsonpath='{.status.APIRuleStatus.code}' --timeout=60s
echo "apirule status OK"

echo "dumping resources"
kubectl get pod -A
kubectl get apirules -A
kubectl get apirules.v2alpha1.gateway.kyma-project.io -A -o yaml
kubectl get authorizationpolicies.security.istio.io -A
kubectl get authorizationpolicies.security.istio.io -A -o yaml

echo "checking external communication"
until curl http://httpbin.${DOMAIN_TO_EXPOSE_WORKLOADS}/ip --fail; do sleep 5; done

echo "checking internal communication"
kubectl delete ns test || true
kubectl create ns test
kubectl label namespace test istio-injection=enabled --overwrite
kubectl run --namespace test --image=curlimages/curl check-internal-curl -- /bin/sleep 3600
kubectl wait --for=condition=ready pod -n test check-internal-curl
kubectl exec --namespace test -ti check-internal-curl -- curl --verbose --fail --location http://httpbin.$NAMESPACE.svc.cluster.local:8000/ip

The internal communication works fine:

++ kubectl exec --namespace test -ti check-internal-curl -- curl --verbose --fail --location http://httpbin.workload.svc.cluster.local:8000/ip
* Host httpbin.workload.svc.cluster.local:8000 was resolved.
* IPv6: (none)
* IPv4: 10.43.108.176
*   Trying 10.43.108.176:8000...
* Connected to httpbin.workload.svc.cluster.local (10.43.108.176) port 8000
* using HTTP/1.x
> GET /ip HTTP/1.1
> Host: httpbin.workload.svc.cluster.local:8000
> User-Agent: curl/8.12.0
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 200 OK
< server: envoy
< date: Fri, 07 Feb 2025 12:02:11 GMT
< content-type: application/json
< content-length: 28
< access-control-allow-origin: *
< access-control-allow-credentials: true
< x-envoy-upstream-service-time: 7
<
{
  "origin": "127.0.0.6"
}

There is an additional authorization policy created:

- apiVersion: security.istio.io/v1
  kind: AuthorizationPolicy
  metadata:
    creationTimestamp: "2025-02-07T12:02:03Z"
    generateName: httpbin-
    generation: 1
    labels:
      apirule.gateway.kyma-project.io/v1beta1: httpbin.workload
      gateway.kyma-project.io/hash: workload.47fnidc80q3pr.0
      gateway.kyma-project.io/index: "1"
    name: httpbin-l2pnw
    namespace: workload
    resourceVersion: "1846"
    uid: 41b87d67-d7f9-47dc-8273-7b1c0d4ec69e
  spec:
    rules:
    - from:
      - source:
          notPrincipals:
          - cluster.local/ns/istio-system/sa/istio-ingressgateway-service-account
    selector:
      matchLabels:
        app: httpbin

@ingentismg
Copy link

I am not overly familiar with the AuthorizationPolicies, but the suggested allow-internal policy would mean that deployments that secure their application with an own-namespace-only + istio principal AP would suddenly allow all cluster traffic again?

This would then require a change to a deny other namespaces, unless it is the istio-principal policy?

@strekm
Copy link
Contributor

strekm commented Feb 13, 2025

@ingentismg this implementation match functionality from APIRule v1beta1 that was based on ORY Oathkeeper. Oathkeeper did not configure / restrict internal traffic at all. Based on feedback we received we decided to implement it the same way.

APIRule never supported configuration to secure internal traffic. In the future we might want to include that feature on our roadmap.

@ingentismg
Copy link

Hi @strekm ,

sorry if I am misunderstanding, but assume the following context:

An application/pod has /public and /internal HTTP routes.
/public is made publicly available via an APIRule, without any authentication.
The setup also adds an AuthorizationPolicy, that allows the istio principal access to /public and only the current namespace access to /internal.

With v1beta1 only the istio-principal can access /public and only the namespace of the pod can access /internal.
With v2 however, from the examples I have seen here, any non-istio principal can access both /public and /internal, from any namespace. My AuthorizationPolicy is basically useless.

The APIRule is actively making my app less secure. If this is the easiest way to solve the issue for most people, that is fine. But my problem is that this change in behavior does not seem to be documented. That is if I am actually correct and not completely missing or misunderstanding something.

@pbochynski
Copy link
Contributor

As authorization policies are evaluated in the way that any ALLOW policy matching the request will open the access to the workload I would strongly recommend not adding it by default. In Kyma we follow secure by default principle and adding such a policy as allow-internal is against that principle. Especially, since users cannot make it more restrictive by adding their policies. If there is a demand to open in-cluster access it should be optional and should be enabled by user in the API rule spec. This would be useful for development, but I expect that all production systems would rather go with restricted access to their workload and you should just document how to enable access from a given namespace or service account.

@strekm
Copy link
Contributor

strekm commented Feb 14, 2025

based on comments from @ingentismg and @pbochynski we decided to revert this behavior. It will be rolled back with 2.10.5 release and rolled out to managed kyma as soon as possible. Additionally this behavior change will be documented as breaking change compared to APIRule v1beta1.
Finally we will implement additional configuration on APIRule to configure internal traffic: internalTraffic: ALLOW | DENY with default value set to DENY.

mluk-sap added a commit to mluk-sap/api-gateway that referenced this issue Feb 14, 2025
…itional Authorization Policy with action ALLOW that is hard to override
mluk-sap added a commit to mluk-sap/api-gateway that referenced this issue Feb 14, 2025
…itional Authorization Policy with action ALLOW that is hard to override
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

No branches or pull requests

7 participants