-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding token validation #11889
base: main
Are you sure you want to change the base?
Adding token validation #11889
Conversation
def valid_auth_token?(token, issuer) | ||
begin | ||
pub_key = OpenSSL::X509::Certificate.new( | ||
config_data(issuer)[:key], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be preferable to re-use the certificate for the issuer that we already have as part of the partner configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly, but there were two things i was thinking about in this implementation:
- we haven't done user research to see if other partners would like to use their currently certificate
- we don't delete old certificates, so we'd have to cycle through existing certificates. while that's probably something we want ultimately, it didn't feel like a decision that had to be made at this moment!
end | ||
|
||
def payload_matches?(token_payload) | ||
hashed_payload = Digest::SHA256.hexdigest(poll_params.to_json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a way to simplify. Currently we send a payload in the body, and a JWT in a header to verify. Would it make sense to put everything in the JWT, ex:
{
issuer: 'MY_ISSUER',
parameters: {
maxEvents: 1,
acks: []
}
}
The current implementation reminds me a bit of the process Google Maps uses for signing requests.
I sort of lean towards choosing one approach amongst JWT-only or verify-by-parameter-signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoot, the thing that put me on this line of thinking is that as written, the implementation in part relies on the key order being maintained through:
- JSON string from request
- Parsed into Ruby hash
- Back into JSON string via
to_json
If I understand right, Ruby's implementation of JSON will maintain the key ordering, but strictly speaking, the JSON spec is explicit that they do not have an order:
An object is an unordered collection of zero or more name/value pairs, where a name is a string and a value is a string, number, boolean, null, object, or array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is an issue that also came to my mind. :( i was thinking about whether we could try both options, but that would require some gross string translation i think.
i am open to other suggestions, since it does seem likely this would be brittle at best.
i looked into signing the whole payload, but the JWT would be too big to fit in the authorization header. we could send that as a request
parameter ala the OIDC spec (like we talked about this morning?).
would that have any ramifications on the security of the request, since it's not via the authorization header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking on the RISC polling spec, there doesn't seem to be wiggle room to pass a request
parameter with the full payload signed. :( i think we may have to use a static shared secret for now, and consider implementing a token
endpoint at some point in the future
3a58052
to
b714dc7
Compare
end | ||
|
||
def valid_auth_token?(token, issuer) | ||
config_data(issuer)[:token] == token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider using ActiveSupport::SecurityUtils.secure_compare
. Even better would be to store a hashed version (which we did a half-hearted version of here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay what do you uh, think of 5ec84ca? the IdentityConfig
stuff feels pretty bad but i think necessary to accommodate multiple partners and multiple tokens
🎫 Ticket
Link to the relevant ticket:
Validate signed request
🛠 Summary of changes