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

enhancement: Add token based auth as alternative to basic-auth for http requests. #192

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
9 changes: 8 additions & 1 deletion .github/workflows/build_and_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ jobs:

wait_for_pod_ready "sidecar"
wait_for_pod_ready "sidecar-5xx"
wait_for_pod_ready "sidecar-api_key"
wait_for_pod_ready "dummy-server-pod"

- name: Install Configmaps and Secrets
Expand All @@ -80,6 +81,7 @@ jobs:
- name: Retrieve pod logs
run: |
kubectl logs sidecar > /tmp/sidecar.log
kubectl logs sidecar > /tmp/sidecar-api_key.log
kubectl logs sidecar-5xx > /tmp/sidecar-5xx.log
kubectl logs dummy-server-pod > /tmp/dummy-server.log
- name: Upload artifacts
Expand Down Expand Up @@ -109,6 +111,9 @@ jobs:
kubectl cp sidecar-5xx:/tmp-5xx/relative/relative.txt /tmp/5xx/relative.txt
kubectl cp sidecar-5xx:/tmp-5xx/500.txt /tmp/5xx/500.txt

echo "Downloading resource files from sidecar-api_key..."
kubectl cp sidecar-api_key:/tmp/api_key.txt /tmp/api-key/api_key.txt || true

- name: Verify files
run: |
echo "Verifying file content from sidecar and sidecar-5xx ..."
Expand All @@ -126,4 +131,6 @@ jobs:
echo -n "This absolutely exists" | diff - /tmp/5xx/absolute.txt &&
echo -n "This relatively exists" | diff - /tmp/5xx/relative.txt &&
echo -n "500" | diff - /tmp/5xx/500.txt &&
ls /tmp/5xx/script_result
ls /tmp/5xx/script_result &&
[ -f /tmp/api-key/api_key.txt ] && echo "api_key used for aouthentication"

9 changes: 9 additions & 0 deletions test/resources/resources.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,12 @@ metadata:
findme: "yup"
data:
500.txt.url: "http://dummy-server/500"
---
apiVersion: v1
kind: ConfigMap
metadata:
name: url-configmap-api_key
labels:
findme: "sure"
data:
api_key.txt.url: "http://dummy-server/200/api_key"
45 changes: 45 additions & 0 deletions test/resources/sidecar.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,51 @@ spec:
defaultMode: 0777
---
apiVersion: v1
kind: Secret
metadata:
name: token-auth-sample-secret
type: Opaque
data:
REQ_TOKEN_KEY: private_token
REQ_TOKEN_VALUE: c3VwZXItZHVwZXItc2VjcmV0
---
apiVersion: v1
kind: Pod
metadata:
name: sidecar-api_key
namespace: default
spec:
serviceAccountName: sample-acc
containers:
- name: sidecar
image: kiwigrid/k8s-sidecar:testing
volumeMounts:
- name: shared-volume
mountPath: /tmp/
- name: script-volume
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need the script here.

Copy link
Author

Choose a reason for hiding this comment

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

good point. removed the script

mountPath: /opt/script.sh
subPath: script.sh
envFrom:
- secretRef:
name: token-auth-sample-secret
env:
- name: LABEL
value: "findme"
- name: FOLDER
value: /tmp/
- name: RESOURCE
value: both
- name: SCRIPT
Copy link
Member

Choose a reason for hiding this comment

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

and here

Copy link
Author

Choose a reason for hiding this comment

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

good point. removed the script

Copy link
Author

Choose a reason for hiding this comment

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

also switched resource to configmap

value: "/opt/script.sh"
volumes:
- name: shared-volume
emptyDir: {}
- name: script-volume
Copy link
Member

Choose a reason for hiding this comment

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

and here

Copy link
Author

Choose a reason for hiding this comment

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

good point. removed the script

configMap:
name: script-configmap
defaultMode: 0777
---
apiVersion: v1
kind: Pod
metadata:
name: dummy-server-pod
Expand Down
21 changes: 19 additions & 2 deletions test/server/server.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
from fastapi import FastAPI
import uvicorn
#from http.client import HTTPException, HTTPResponse
from fastapi import FastAPI, Security, Depends, HTTPException
from fastapi.security.api_key import APIKeyQuery, APIKey
#import uvicorn
jekkel marked this conversation as resolved.
Show resolved Hide resolved

API_KEY_NAME="private_token"
API_KEY="super-duper-secret"
api_key_query = APIKeyQuery(name=API_KEY_NAME, auto_error=True)

app = FastAPI()

def get_api_key (api_key_query: str = Security(api_key_query)):
if api_key_query == API_KEY:
return api_key_query
else:
raise HTTPException(403)

@app.get("/", status_code=200)
def read_root():
Expand All @@ -27,3 +38,9 @@ async def read_item():
@app.post("/503", status_code=503)
async def read_item():
return 503


@app.get("/200/api_key")
Copy link
Member

Choose a reason for hiding this comment

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

could you add another endpoint with a different API key so we can add a negative test case as well? That would be awesome!

Copy link
Author

Choose a reason for hiding this comment

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

added a new container with wrong credentials/secret and used the same endpoint

def read_root(api_key: APIKey = Depends(get_api_key)):
return 200