Skip to content
This repository was archived by the owner on Jul 24, 2023. It is now read-only.

Maybe check signatures first? #124

Closed
utkarsh2102 opened this issue Oct 10, 2019 · 1 comment · Fixed by #126
Closed

Maybe check signatures first? #124

utkarsh2102 opened this issue Oct 10, 2019 · 1 comment · Fixed by #126

Comments

@utkarsh2102
Copy link
Contributor

Hi @tobiashm,
Just to be extra secure, doesn't it make sense to check signatures first so that no third party could interfere?
Something like:

--- a/lib/openid/consumer/idres.rb
+++ b/lib/openid/consumer/idres.rb
@@ -70,9 +70,9 @@
       # 'endpoint', 'message', and 'signed_fields' contain the
       # verified information.
       def id_res
+        check_signature
         check_for_fields
         verify_return_to
-        check_signature
         check_nonce
         verify_discovery_results
       end

My take is that this method will raise ProtocolError in the very first place (which is a good thing!) unless the request is a valid id_res response. Once it has been verified, the methods endpoint, message, and signed_fields contain the verified information.
Thereby making everything secure from any third party interference.

I'll be happy to raise a PR if this makes sense to you?

@tobiashm
Copy link
Contributor

That sounds reasonable.
It's been a while since I've looked at the spec, so I can't remember if there's anything in there about the order.

A PR would be much appreciated 😃 — and it would be great if you could verify that we're still following the spec.

Perhaps the check_nonce should also be moved if that makes sense (just from the names, it looks like it could make sense to first perform all the checks, then verify URLs).

utkarsh2102 added a commit to utkarsh2102/ruby-openid that referenced this issue Oct 15, 2019
utkarsh2102 added a commit to utkarsh2102/ruby-openid that referenced this issue Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants