validate jwts #118
validate jwts #118
Conversation
|
To document for the future, skipping JWT verification when TLS is in use is discussed in the OpenID spec
While I'm hesitant to argue with a spec document, I would do so for a few reasons:
Nevertheless, I cannot definitively say that JWT verification is absolutely necessary, so I won't hold up this PR |
- add JWK set to config - use jwcrypto for JWT decode, since it does all the things for us - refactor config heirarchy + tests
tests/unit/conftest.py
Outdated
There was a problem hiding this comment.
No objection per se, but is this file necessary?
There was a problem hiding this comment.
it's not necessary - it's a standard file for pytest that I was using for some fixtures while I worked through this, but they all got refactored away. I can remove this to tidy up though
cf_auth_proxy/config.py
Outdated
| self.SESSION_REFRESH_EACH_REQUEST = False | ||
| self.PERMANENT_SESSION_LIFETIME = self.env_parser.int("SESSION_LIFETIME") |
There was a problem hiding this comment.
What are these values for? Are they related to the JWT changes?
There was a problem hiding this comment.
These are unrelated to the change - they just moved unchanged during the config restructure.
these control how flask tracks sessions.
session_refresh_each_request makes flask include a set cookie header on every response. I don't recall why we set it that way initially.
permanent_session_lifetime controls how long into the future to set session cookie's expiration
There was a problem hiding this comment.
It looks like SESSION_REFRESH_EACH_REQUEST defaults to True but now will be set to False? From what you described, False seems like what we want, but just want to make sure the behavior will be what we expect
There was a problem hiding this comment.
actually, looks like this is a change I introduced accidentally here. I think I set the SESSION_REFRESH_EACH_REQUEST on the local config when fighting some other issue, then assumed that because we set it True there, it must be False elsewhere.
I am not really sure whether we want this behavior or not. Setting the cookie every time seems like it could save a little annoyance if we change the session structure or something, but it also seems like it could be a meaningful amount of overhead given the number of small background requests opensearch is likely to make
There was a problem hiding this comment.
Can we just preserve the current behavior for now? Just want to limit this PR to only the intended changes. Happy to change the behavior later as necessary
There was a problem hiding this comment.
after some more thinking, I'm leaning towards leaving SESSION_REFRESH_EACH_REQUEST as the flask default.
I think either behavior will work, and the tradeoffs seem minimal at this point, so from a maintainability perspective I think it's better to have a smaller config set to think about is better, and having fewer differences between unit testing vs local testing vs production is better
Changes proposed in this pull request:
Things to check
INFOand debugging statements are written withlog.debugor similar, then they won't be written to the otput, which can prevent unintentional leaks of sensitive data.Security considerations
jwcrypto does most of the token validation we need right out of the box, so this is big step forward in security.