Repository navigation
Conversation
… may be based on internal k8s routing to the service
Integration TestsFailed to Prepare CI environment Please find the Github Action logs here |
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
Coverage Report for CI Build 37331405262Coverage decreased (-0.01%) to 74.658%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions194 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
…s to handle a user_id without needing to do full JWT validation (since Fence does that already)
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
Integration TestsFailed to Prepare CI environment Please find the Github Action logs here |
Integration TestsFailed to Prepare CI environment Please find the Github Action logs here |
| alembic = ">=1.7.7" | ||
| authlib = ">=1.6.10,<=1.6.12" | ||
| authutils = ">=8.0.0" | ||
| authutils = {git = "https://github.com/uc-cdis/authutils.git", rev = "feat/dpop"} |
There was a problem hiding this comment.
Reminder to pin it to a release before merge.
| # | ||
| # You can use openssl to generate this (but remember you MUST use the SAME secret across services) | ||
| # `openssl rand -base64 64` | ||
| DPOP_SHARED_SECRET: null |
There was a problem hiding this comment.
Should there be any user-facing documentation added for the dPop setup steps, perhaps letting users know where they can add this secret as an environment variable in gen3-helm (fence/values.yaml#L346)? Or do you think this is outside the scope of fence?
| if not header.lower().startswith("bearer") and not header.lower().startswith( | ||
| "dpop" | ||
| ): |
There was a problem hiding this comment.
Can combine both the header options into a single one.
| if not header.lower().startswith("bearer") and not header.lower().startswith( | |
| "dpop" | |
| ): | |
| if not header.lower().startswith(("bearer", "dpop")): |
Or better, to avoid cases like "dpopXyZ <token>", we could do
| if not header.lower().startswith("bearer") and not header.lower().startswith( | |
| "dpop" | |
| ): | |
| if header.lower().split(" ", 1)[0] not in ("bearer", "dpop") |
There was a problem hiding this comment.
and .partition(" ") instead of .split(" ", 1) 😄
or if not header.lower().startswith(("bearer ", "dpop ")):
so many options, so little code!
|
|
||
|
|
||
| def create_user_access_token(keypair, api_key, expires_in, task_token_type): | ||
| def create_user_access_token(keypair, api_key, expires_in, task_token_type, cnf=None): |
There was a problem hiding this comment.
Not related to this PR, but tests/scripting/test_fence-create.py has methods named test_user_access_token_... that don't actually test this method. It might be worth renaming those tests to avoid future confusion.
Feel free to defer this if you think it’s out of scope or self-explanatory, since the file name already indicates that those tests are for fence-create.py.
| now = int(time.time()) | ||
| with flask.current_app.db.session as session: | ||
| # A proof older than this is rejected on `iat` alone, so its id stops mattering. | ||
| session.query(DPoPProofJTI).filter(DPoPProofJTI.exp < now).delete() |
There was a problem hiding this comment.
We could add an index on the exp field here since we query the table based on expiry for every check.
There was a problem hiding this comment.
agreed, good call. working on adding it
| logger.error(f"Unknown error validating DPoP request: {exc}") | ||
| raise UserError("Error validating DPoP request") | ||
|
|
||
| # minting the token is what validates the api key and resolves the user it |
There was a problem hiding this comment.
It seems a little out of the ordinary that we perform the entire DPoP proof validation and respond with a nonce on the first attempt before validating the API key here. This means a user with an invalid API key can still attempt to obtain a task token, and they would see the nonce error first rather than an expired/invalid API key error.
There was a problem hiding this comment.
Since create_user_access_token is only called once in the whole repo, can we refactor it so that api.py calls validate_jwt directly, before validate_dpop_proof, and then passes the resulting claims into create_user_access_token? That way an invalid API key is rejected before the DPoP proof is validated and its jti is written to the database, and we don't validate the key twice
There was a problem hiding this comment.
good catch, agreed. will work on cleaning this up so the validation occurs once before nonce repsonse
| with flask.current_app.db.session as session: | ||
| # A proof older than this is rejected on `iat` alone, so its id stops mattering. | ||
| session.query(DPoPProofJTI).filter(DPoPProofJTI.exp < now).delete() | ||
| session.add(DPoPProofJTI(jti=jti, exp=now + DPOP_PROOF_MAX_TTL)) |
There was a problem hiding this comment.
Should we also incorporate authutils.dpop.DPOP_PROOF_CLOCK_SKEW_LEEWAY while calculating exp?
Since iat = now() + skew_leeway is considered valid, so, shouldn't the jti be in the database until then?
There was a problem hiding this comment.
yep, you're right, this is a real gap. will adjust
| sa.Column("exp", sa.BigInteger(), nullable=False), | ||
| sa.PrimaryKeyConstraint("jti"), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Consider adding an index over exp since it is used to query and delete during every jti_seen callback
| "auth_request", | ||
| wraps=flask.current_app.arborist.auth_request, | ||
| ) as auth_request: | ||
| assert can_user_get_task_token("FOO", 200, "test-user") is True |
There was a problem hiding this comment.
| assert can_user_get_task_token("FOO", 200, "test-user") is True | |
| assert can_user_get_task_token("FOO", 200, "test-user") |
Unless the addition was intentional to be stricter. But I couldn't see any need for it.
Co-authored-by: Sai Shanmukha Narumanchi <nss10@outlook.com>
paulineribeyre
left a comment
paulineribeyre
left a comment
There was a problem hiding this comment.
lgtm (i did not review the tests)
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
4 participants
New Features
DPOP_ENABLEDis true,POST /credentials/api/access_tokenrequires a valid DPoP proof (with nonce) for task token requests and issues a key-bound access token with acnf.jktclaim.DPoP-Nonceheader so the client can retry.DPOP_SHARED_SECRET, which must be identical across all Gen3 services that participate in DPoP.DPOP_SHARED_SECRETcan be supplied via environment variableBreaking Changes
Bug Fixes
validate_jwtno longer performs JWKS key discovery against an unverifiediss: the issuer is checked against the configured allowlist first, and requests with no configured issuers are rejected instead of triggering an outbound request.Improvements
/credentials/api/access_token(enabled/disabled, mi retry) and for the issuer allowlist check invalidate_jwt.Dependency updates
authutilsto get DPoP supportDeployment changes
DPOP_ENABLED: trueand setDPOP_SHARED_SECRET(e.g.openssl rand -base64 64) to the same value in every service doing DPoP validation. Fence will refuse to start if DPoP is enabled without the secret.