Skip to content

Dev jwks auth - #31

Open
frobnitzem wants to merge 6 commits into
mainfrom
dev-jwks-auth
Open

Dev jwks auth#31
frobnitzem wants to merge 6 commits into
mainfrom
dev-jwks-auth

Conversation

@frobnitzem

Copy link
Copy Markdown
Collaborator

Adopts the OIDC portion of branch apiv2-persist-durable.

@frobnitzem

Copy link
Copy Markdown
Collaborator Author

This branch still needs two things before it's ready to merge:

  1. Testing verifying the SLAC JWT is properly validated.
  2. Addition of the auth.CurrentUser dependency to all routes.

Note that switching to this auth scheme and still using TLS will give "untrusted CA" errors unless a) clients add our CA, or b) we obtain a server cert. from SLAC.

@frobnitzem

Copy link
Copy Markdown
Collaborator Author

This PR is "working" but the CurrentUser-protected routes can't be tested without modifying the test client to inject a user token.

Some TODO-s remain for making this branch cleaner:

  • tests/test_config.py:# TODO: setup an ephermal RS256 token mint to create and validate test tokens
  • tests/test_server.py:client = TestClient(api) # TODO: bake my_token into authorization requests with this client
  • auth.py: Upgrade to an initialization pattern that allows reading a non-global config. file (for testing).

Comment thread config/lclstream_api.yaml
"--exclusive": ""

forwarder:
ip: "134.79.23.42"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe not good to put real IPs here?

raise HTTPException(status_code=404, detail="Transfer is not active.")

if entry.user != user:
raise HTTPException(status_code=404, detail="Transfer is not active.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i see 404 here to hide this, but maybe we could do "Transfer not found" instead?

Comment thread tests/test_jwks.py
from fastapi_jwks.models.types import JWTHeader

# FIXME: config. loading for jwks currently uses $VIRTUAL_ENV
# due to module-level definitions in auth.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do you mean we should avoid creating a global?

Comment thread tests/test_jwks.py
# assert payload.iss == _oidc.issuer_url

def test_config_aud():
""" Parsing the audiences field is apparently difficult for the lclstream-api config.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what do you mean

Comment thread tests/test_jwks.py
unverified_claims = jwt.decode(my_token, options={"verify_signature": False})

#kid = unverified_header.get("kid")
#alg = unverified_header.get("alg")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rm comments?

Comment thread pyproject.toml
"lclstream @ git+https://github.com/lclstream/lclstream.git",
"psik>=3.1.0",
"pydantic>=2.13.4",
"pydantic-settings>=2.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in this PR you are not using pydantic settings ight?

Comment thread .env.example

# --- App (LCLSTREAM_APP_*) -------------------------------------------------
# External path prefix the app is mounted under (FastAPI root_path). Must match
# the gateway's PathPrefix; override per env (e.g. /api/dev).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are we using this dotenv yet?

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

Successfully merging this pull request may close these issues.

2 participants