Add support for token auth using tokens fetched from arbitrary external tools - #74
Add support for token auth using tokens fetched from arbitrary external tools#74cnweaver wants to merge 5 commits into
Conversation
…al tools. Perform slightly more careful parsing of URLs.
jnation3406
left a comment
jnation3406
left a comment
There was a problem hiding this comment.
Looks good. I don't understand what its doing, but this seems like something internal for you so you know the required format of the params and token_command without documentation.
| if len(sections) != 3: | ||
| raise RuntimeError("Token callback output does not appear to be a valid JWT") | ||
| try: | ||
| # The JWT spec mandates that base 64 padding be omitted, but |
There was a problem hiding this comment.
I wonder if this would be handled automatically with a jwt library, like pyjwt. Maybe you don't want to include extra libraries though.
There was a problem hiding this comment.
We could, yes (with this it looks like); I figured that a new dependency was a lot of overhead for something that requires such minimal parsing, since we aren't concerned with any of the cryptographic verification.
| sections[1] += b"==" | ||
| elif m == 3: | ||
| sections[1] += b"=" | ||
| elif m == 1: |
There was a problem hiding this comment.
do you not need an else for the case of m == 0 potentially?
There was a problem hiding this comment.
If m==0 the amount of padding needed is zero by definition.
| exp_value = claims["exp"] | ||
| if not isinstance(exp_value, int) and not isinstance(exp_value, float): | ||
| raise RuntimeError("Token expiration value is not a number") | ||
| if isinstance(exp_value, int): |
There was a problem hiding this comment.
I think its safe to set exp_value = float(exp_value) without checking if its int.
There was a problem hiding this comment.
The spec requires it to be a number, so I figured I might as well implement that check to flag invalid inputs. Otherwise, a malformed token could cause a ValueError from float(), which would require a similar amount of error handling.
There was a problem hiding this comment.
Yes, I'm talking about removing line 137, not line 135-6
There was a problem hiding this comment.
I see, yes, that check is redundant to what float() already does internally.
2 participants
This enables using token auth with non-OIDC token issuing system systems, such as LIGO's vault/openbao-based system.