Two pull requests merged this week and the most security-critical piece of the whole
project went out for review on its own. This was the week the plan from week 7 started
visibly paying off.
Two merges
On 28 July, #7654 merged — the
LtiDeployment model. CircuitVerse can now store which LMS platforms it trusts: the
issuer, the client ID, the deployment ID, and the platform's endpoint URLs, unique per
issuer + client + deployment because that triple is how LTI 1.3 identifies a deployment.
Nothing reads the table yet. That is the point of building this way, and it is the thing
that needs explaining in every PR description.
The validator
The main work was #7715:
Lti::JwtValidator, a class with one public method.
Lti::JwtValidator.validate!(token, deployment:, nonce:)
Give it an id_token, the deployment it should belong to, and the nonce we issued. It
either returns the verified payload or raises. No controller, no session, no Rails
request object. That isolation was forced by the constraint that it had to be its own
pull request, and it is a much better boundary than I would have drawn otherwise.
What it has to refuse
The tests are mostly attacks:
A token signed by the wrong key. The baseline.
A token with alg: none. Early JWT libraries would accept an unsigned token if it
declared its algorithm as none. Any validator that reads the algorithm out of the
token and trusts it is exploitable.
An HS256 token where RS256 is expected. This is algorithm confusion, and it is the
subtle one. RS256 is asymmetric: the platform signs with a private key, we verify with
their public key, which is published. HS256 is symmetric: the same secret signs and
verifies. If a validator takes the algorithm from the token header and passes the
platform's public key as the key, an attacker can sign a token using that public key
as an HMAC secret — and the validator verifies it, because it is doing exactly what it
was told. The public key is public, so anyone can do this.
The defence is to never read the algorithm from the token. Pass an explicit allow-list:
encoded_token.verify_signature!(algorithm: "RS256", key: key)
Expired, wrong issuer, wrong audience, missing subject, replayed nonce, and the
multi-audience case: when aud is an array with more than one entry, the azp claim
must name our client ID.
Resolving the platform's key
The other half is fetching the right public key. A platform publishes a JWKS endpoint;
the token header carries a kid identifying which key signed it.
Two things made this more interesting than expected:
Key rotation. Platforms rotate keys. A cached key set will not contain a newly
rotated key. So a kid that is missing from the cache is treated as a signal that
rotation may have happened, and triggers a refetch rather than a wait for expiry.
Caching. Without it, every single launch makes an outbound HTTP request to the
platform before anyone can sign in. If the platform's JWKS endpoint is slow, every login
is slow; if it is down, every login is down. A short-lived cache keyed on the JWKS URL
fixes that, with the rotation behaviour above as the escape hatch.
What review found
The bot review flagged two failure modes I had not handled, and both were correct:
A blank jwks_url. A deployment configured with only a stored public key and no
JWKS URL would still make an HTTP request, because a blank URL parses as a relative path
rather than failing. So every launch for that deployment took a pointless network trip
before falling back. One guard clause.
Malformed stored key data. OpenSSL::PKey::RSA.new raises OpenSSL::PKey::RSAError
on bad PEM data, and I was only rescuing JWT::DecodeError. So a misconfigured
deployment record would propagate an OpenSSL exception out of a method whose entire
contract is "raises ValidationError". Callers written against that contract would not
catch it.
That second one is a good example of a bug that is invisible while you are writing the
code and obvious once named. My mental model was "this method validates tokens, so it
raises validation errors." The actual behaviour was "this method raises validation
errors unless the database row is malformed, in which case it raises something else
entirely."
I fixed both in fix(lti): harden JwtValidator key resolution per review.
The refactor request
Late in the week, a maintainer asked something more interesting than a bug report:
could you suggest refactoring this class using JWT::Token or JWT::EncodedToken
My implementation called JWT.decode twice — once with verification disabled to read
the header and pick a key by kid, then again with verification on. That works, and it
has a smell: there is a window where the code holds an unverified, parsed token, and
nothing but my own discipline stops me from reading a claim out of it.
JWT::EncodedToken closes that window at the library level. #payload refuses to
decode until both the signature and the claims have been verified:
def payload
raise JWT::DecodeError, '...' unless @signature_verified
raise JWT::DecodeError, '...' unless @claims_verified
decoded_payload
end
The header is still read unverified, which is unavoidable — you cannot pick a key
without knowing which key was used — but it is only ever used to select a candidate key,
and the ordering the class used to depend on by convention is now enforced by the
library.
That is a strictly better property: the difference between "the code is correct" and
"the code cannot easily be made incorrect by the next person to edit it."
What I am taking from the week
The isolation was the win. Because the validator was its own pull request, with no
controller and no session attached, both the review comments were about the validator
— its error contract, its failure modes — rather than about whether the launch flow
around it made sense.
Compare that to my POC, where the same class was buried in 2,000 lines and nobody looked
at it at all.
Next week: the first half of the handshake ships, and a scanner disagrees with me about
CSRF.
Top comments (0)