Fix and test adding new issuers #260

beejones · 2024-12-11T17:09:01Z

Fixed the bug that a new issuer was not found.
Added auth tests
Test adding a new issuer
Test calling with a wrong issuer
Changed our test issuer to access properties to be put into the token

scripts/ccf/az-cleanroom-aci/up.sh

scripts/ccf/sandbox_local/up.sh

scripts/jwt_issuer/up.sh

DomAyre · 2024-12-12T09:00:23Z

scripts/jwt_issuer/up.sh

@@ -14,7 +14,8 @@ jwt-issuer-up() {

    sudo chown $USER:$USER -R $JWT_ISSUER_WORKSPACE

-    export JWT_ISSUER="http://localhost:3000/token"
+    export JWT_TOKEN_ISSUER_URL="http://localhost:3000/token"


Can you explain the difference between these two things? They're both URLs and I presume they both issue tokens so maybe we can think of a better naming scheme?

Also not super keen on JWT_TOKEN because the T in JWT stands for token.

So we have JSON Web Token Token Issuer

JWT_ISSUER is the name used in CCF for registering the JWT issuer.
JWT_TOKEN_ISSUER_URL is the actual url of the endpoint.

it is very common to talk about JWT tokens. Just do a search for "JWT token".

JWT_ISSUER is the name used in CCF for registering the JWT issuer. JWT_TOKEN_ISSUER_URL is the actual url of the endpoint.

Ah thanks for the explanation, that makes sense

it is very common to talk about JWT tokens. Just do a search for "JWT token".

Being common doesn't make it correct or less redundant, and I wouldn't claim it's more common that just JWT, jwt.io always just says JWT.

How about we use

JWT_ISSUER_URL which is the URL which issues JWTs

JWT_ISSUER_NAME which is the name of the thing which issues JWTs (for CCF)

We could make it clearly a name by making it not look like a URL?

I believe CCF requires this to be a url. Need to check this.

If it needs to be a URL, could it just be the actual URL so we only need one variable instead of two?

indeed.
{
bool validate_issuer(
const std::string& iss,
const std::optionalstd::string& tid,
std::string constraint)
{
LOG_DEBUG_FMT(
"Verify token.iss {} and token.tid {} against published key issuer {}",
iss,
tid,
constraint);

const auto issuer_url = ::http::parse_url_full(constraint);

If it needs to be a URL, could it just be the actual URL so we only need one variable instead of two?

See #260 (comment)

I understand it's a test JWT issuer, but why can't both values be http://localhost:3000/token? In which case the JWT issuer up script only needs to emit one value not two? Isn't it also kind of wrong to have a pretend URL for the issuer which doesn't match it's actual URL?

I've tested this locally with a couple of small changes to the issuer and it works fine

test/system-test/test_auth.py

DomAyre · 2024-12-12T09:09:08Z

scripts/kms/jwt_issuer_trust.sh

@@ -8,6 +8,24 @@ jwt-issuer-trust() {

  REPO_ROOT="$(realpath "$(dirname "$(realpath "${BASH_SOURCE[0]}")")/../..")"

+  # Check for new issuer


For my own understanding, the scheme is:

If you call this without --iss it will use JWT_TOKEN_ISSUER_URL which will have been set if you bring up a local test JWT issuer

If you have some external JWT issuer you can just specify it with this --iss flag?

If my understanding is correct I really like this scheme!

For an external one we need to set JWT_TOKEN_ISSUER_URL to the token endpoint.
--iss will the name for the iss property in the token. CCF uses this to register the issuer.

So is it equivalent to setting JWT_ISSUER? In which case why are we mixing methods? I think we either allow fully specifying which JWT issuer we're trusting via command line params or we just require the env variables to be changed?

I added a test adding a new issuer implemented by our test token issuer.
For this case I needed to specify the issuer JWT_ISSUER and keep the url to the test token url.

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?

Now we've decided to unify JWT_ISSUER this goes away

beejones · 2024-12-13T10:04:25Z

tests:
https://github.com/microsoft/privacy-sandbox-dev/actions/runs/12313566584

scripts/jwt_issuer/up.sh

DomAyre · 2024-12-13T10:33:22Z

tests: https://github.com/microsoft/privacy-sandbox-dev/actions/runs/12313566584

@beejones I think the privacy-sandbox-dev CI is currently broken, I'm looking into it now and I'll update when it's fixed

DomAyre · 2024-12-17T08:53:50Z

Now privacy-sandbox-dev CI is fixed, here is a run for this PR
https://github.com/microsoft/privacy-sandbox-dev/actions/runs/12369513461

DomAyre · 2024-12-17T10:07:44Z

Merged the fixes in main to this PR, re-running privacy-sandbox-dev CI
https://github.com/microsoft/privacy-sandbox-dev/actions/runs/12370752558

…ft/azure-privacy-sandbox-kms into beejones/fix-jwt-issuers

…errors

DomAyre · 2025-01-09T09:36:50Z

test/utils/jwt/README.md

+
+## Request
+```
+curl -X POST ${AadEndpoint:-http://localhost:3000/token} \


This needs updating since #266

beejones · 2025-01-09T10:26:53Z

scripts/jwt_issuer/up.sh

@@ -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"
+    export JWT_ISSUER="http://Demo-jwt-issuer"


only use JWT_ISSUER

DomAyre · 2025-01-09T10:27:13Z

scripts/jwt_issuer/down.sh

@@ -9,3 +9,4 @@ docker compose -p ${UNIQUE_ID:-default} -f services/docker-compose.yml down jwt-

 unset JWT_ISSUER_WORKSPACE
 unset JWT_ISSUER
+unset JWT_TOKEN_ISSUER_URL


We will update this too when we combine JWT_ISSUER into one value

DomAyre · 2025-01-09T10:27:55Z

scripts/jwt_issuer/up.sh

@@ -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"


We set it twice

beejones · 2025-01-09T10:32:58Z

scripts/kms/endpoints/auth.sh

+# Licensed under the MIT license.
+
+set -ex
+


DomAyre · 2025-01-09T10:47:35Z

scripts/kms/js_app_set.sh

@@ -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


We can revisit this when all other changes are addressed, this shouldn't be necessary and it might hide other issues

DomAyre · 2025-01-09T10:51:13Z

test/system-test/conftest.py

@@ -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",


We can match the default in up.sh (/defualt/)

beejones added 2 commits December 11, 2024 16:22

fix add issuer and new tests

3659f05

fixing auth tests

e2539f4