-
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
Fix and test adding new issuers #260
base: main
Are you sure you want to change the base?
Changes from all commits
3659f05
e2539f4
37618bc
a2359f2
92020eb
9497639
9c2cf3e
fa65601
ab9f92b
d039514
824775c
d3a2843
e7cd184
4252e2f
685aad3
a8192dc
3a79912
9c572f3
08e69e9
d93bd4c
c78a9c6
e70301a
68b9668
34b7d0b
d932624
d74e9b3
6bc2264
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,38 @@ | ||
{ | ||
"actions": [ | ||
{ | ||
"name": "set_jwt_issuer", | ||
"args": { | ||
"issuer": "http://Demo-jwt-issuer", | ||
"key_filter": "all", | ||
"jwks": { | ||
"keys": [ | ||
${JWK} | ||
] | ||
} | ||
"actions": [ | ||
{ | ||
"name": "set_jwt_issuer", | ||
"args": { | ||
"issuer": "${JWT_ISSUER}", | ||
"key_filter": "all", | ||
"jwks": { | ||
"keys": [ | ||
${JWK} | ||
] | ||
} | ||
}, | ||
{ | ||
"name": "set_jwt_validation_policy", | ||
"args": { | ||
"issuer": "http://Demo-jwt-issuer", | ||
"validation_policy": { | ||
"iss": "http://Demo-jwt-issuer", | ||
"sub": "c0d8e9a7-6b8e-4e1f-9e4a-3b2c1d0f5a6b", | ||
"name": "Cool caller" | ||
} | ||
} | ||
}, | ||
{ | ||
"name": "set_jwt_validation_policy", | ||
"args": { | ||
"issuer": "${JWT_ISSUER}", | ||
"validation_policy": { | ||
"iss": "${JWT_ISSUER}", | ||
"sub": "c0d8e9a7-6b8e-4e1f-9e4a-3b2c1d0f5a6b", | ||
"name": "Cool caller" | ||
} | ||
}, | ||
{ | ||
"name": "set_jwt_public_signing_keys", | ||
"args": { | ||
"issuer": "http://Demo-jwt-issuer", | ||
"jwks": { | ||
"keys": [ | ||
${JWK} | ||
] | ||
} | ||
} | ||
}, | ||
{ | ||
"name": "set_jwt_public_signing_keys", | ||
"args": { | ||
"issuer": "${JWT_ISSUER}", | ||
"jwks": { | ||
"keys": [ | ||
${JWK} | ||
] | ||
} | ||
} | ||
] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,11 +9,13 @@ jwt-issuer-up() { | |
REPO_ROOT="$(realpath "$(dirname "$(realpath "${BASH_SOURCE[0]}")")/../..")" | ||
export JWT_ISSUER_WORKSPACE=${JWT_ISSUER_WORKSPACE:-$REPO_ROOT/jwt_issuers_workspace/${UNIQUE_ID:-default}/} | ||
mkdir -p $JWT_ISSUER_WORKSPACE | ||
export JWT_TOKEN_ISSUER_URL="http://localhost:3000/token" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We set it twice |
||
export JWT_ISSUER="http://Demo-jwt-issuer" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only use JWT_ISSUER |
||
|
||
docker compose -p ${UNIQUE_ID:-default} -f services/docker-compose.yml up jwt-issuer --wait "$@" | ||
sudo chown $USER:$USER -R $JWT_ISSUER_WORKSPACE | ||
|
||
export JWT_ISSUER="$(cat $JWT_ISSUER_WORKSPACE/jwt_issuer_address)/token" | ||
export JWT_TOKEN_ISSUER_URL="$(cat $JWT_ISSUER_WORKSPACE/jwt_issuer_address)/token" | ||
|
||
set +e | ||
} | ||
|
@@ -22,5 +24,7 @@ jwt-issuer-up "$@" | |
|
||
jq -n '{ | ||
JWT_ISSUER_WORKSPACE: env.JWT_ISSUER_WORKSPACE, | ||
JWT_TOKEN_ISSUER_URL: env.JWT_TOKEN_ISSUER_URL, | ||
JWT_ISSUER: env.JWT_ISSUER, | ||
DomAyre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
}' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
#!/bin/bash | ||
|
||
# Copyright (c) Microsoft Corporation. | ||
# Licensed under the MIT license. | ||
|
||
set -ex | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove +x |
||
auth() { | ||
params=() | ||
auth="jwt" | ||
jwtprops="" | ||
|
||
# Parse command-line arguments | ||
while [[ $# -gt 0 ]]; do | ||
case "$1" in | ||
--auth) | ||
auth="$2" | ||
shift 2 | ||
;; | ||
--jwtprops) | ||
jwtprops="$2" | ||
shift 2 | ||
;; | ||
*) | ||
echo "Unknown parameter: $1" | ||
exit 1 | ||
;; | ||
esac | ||
done | ||
|
||
# Extract iss from jwtprops if present | ||
if [[ "$jwtprops" == *"iss="* ]]; then | ||
export JWT_ISSUER=$(echo "$jwtprops" | grep -oP '(?<=iss=)[^&]*') | ||
fi | ||
|
||
auth_arg=() | ||
if [[ "$auth" == "member_cert" ]]; then | ||
auth_arg=(--cert $KMS_MEMBER_CERT_PATH --key $KMS_MEMBER_PRIVK_PATH) | ||
elif [[ "$auth" == "jwt" ]]; then | ||
auth_arg=(-H "Authorization: Bearer $(curl -X POST $JWT_TOKEN_ISSUER_URL$jwtprops | jq -r '.access_token')") | ||
fi | ||
|
||
curl $KMS_URL/app/auth \ | ||
--cacert $KMS_SERVICE_CERT_PATH \ | ||
"${auth_arg[@]}" \ | ||
-H "Content-Type: application/json" \ | ||
-w '\n%{http_code}\n' | ||
} | ||
|
||
auth "$@" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,10 @@ js-app-set() { | |
echo "$line" >> "$temp_file" | ||
fi | ||
done < $REPO_ROOT/governance/proposals/set_js_app.json | ||
|
||
# Ensure the target directory exists | ||
mkdir -p $WORKSPACE/proposals | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can revisit this when all other changes are addressed, this shouldn't be necessary and it might hide other issues |
||
|
||
mv "$temp_file" $WORKSPACE/proposals/set_js_app.json | ||
|
||
# Submit the proposal | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,24 @@ jwt-issuer-trust() { | |
|
||
REPO_ROOT="$(realpath "$(dirname "$(realpath "${BASH_SOURCE[0]}")")/../..")" | ||
|
||
# Check for new issuer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my own understanding, the scheme is:
If my understanding is correct I really like this scheme! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For an external one we need to set JWT_TOKEN_ISSUER_URL to the token endpoint. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So is it equivalent to setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test adding a new issuer implemented by our test token issuer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that addresses my point about mixed methods, for the test you could set both env variables for the test, or you could provider command line params for both and specify them for the test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now we've decided to unify JWT_ISSUER this goes away |
||
while [[ $# -gt 0 ]]; do | ||
case "$1" in | ||
--iss) | ||
JWT_ISSUER="$2" | ||
shift 2 | ||
;; | ||
--iss=*) | ||
JWT_ISSUER="${1#*=}" | ||
shift 1 | ||
;; | ||
*) | ||
echo "Unknown parameter: $1" | ||
exit 1 | ||
;; | ||
esac | ||
done | ||
|
||
# Populate the JWK of the token issuer | ||
PRIVATE_PEM="$JWT_ISSUER_WORKSPACE/private.pem" | ||
CERT_PEM="$JWT_ISSUER_WORKSPACE/cert.pem" | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ def setup_jwt_issuer(): | |
["./scripts/jwt_issuer/up.sh", "--build"], | ||
env={ | ||
**os.environ, | ||
"JWT_ISSUER_WORKSPACE": f"{REPO_ROOT}/jwt_issuer_workspace", | ||
"JWT_ISSUER_WORKSPACE": f"{REPO_ROOT}/jwt_issuers_workspace", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can match the default in up.sh (/defualt/) |
||
}, | ||
) | ||
yield | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
from endpoints import auth | ||
from utils import apply_kms_constitution, trust_jwt_issuer | ||
|
||
|
||
def test_auth_member_cert(setup_kms): | ||
status_code, auth_json = auth(auth="member_cert") | ||
assert status_code == 200 | ||
assert auth_json["auth"]["policy"] == "member_cert" | ||
|
||
|
||
def test_auth_jwt(setup_kms, setup_jwt_issuer): | ||
apply_kms_constitution() | ||
trust_jwt_issuer() | ||
status_code, auth_json = auth(auth="jwt") | ||
assert status_code == 200 | ||
assert auth_json["auth"]["policy"] == "jwt" | ||
|
||
|
||
def test_auth_jwt_new_issuer(setup_kms, setup_jwt_issuer): | ||
issuer = "https://new-issuer" | ||
apply_kms_constitution() | ||
trust_jwt_issuer(iss=issuer) | ||
status_code, auth_json = auth(auth="jwt", jwtprops=f"?iss={issuer}") | ||
assert status_code == 200 | ||
|
||
|
||
def test_auth_jwt_wrong_iss(setup_kms, setup_jwt_issuer): | ||
apply_kms_constitution() | ||
trust_jwt_issuer() | ||
status_code, auth_json = auth(auth="jwt", jwtprops="?iss=test") | ||
assert status_code == 401 | ||
|
||
|
||
if __name__ == "__main__": | ||
import pytest | ||
pytest.main([__file__, "-s"]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# Token issuer | ||
|
||
## Request | ||
``` | ||
curl -X POST ${AadEndpoint:-http://localhost:3000/token} \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs updating since #266 |
||
-H "Content-Type: application/x-www-form-urlencoded" \ | ||
-d "client_id=${ClientApplicationId:-}&client_secret=${ClientSecret:-}&scope=${ApiIdentifierUri:-}/.default&grant_type=client_credentials" \ | ||
-w "\n" | ||
``` |
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.
We will update this too when we combine JWT_ISSUER into one value