-
Notifications
You must be signed in to change notification settings - Fork 3
Rewrite signed elements structure #2
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
Conversation
07f62c1 to
0fb165d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
============================================
+ Coverage 76.35% 83.45% +7.09%
+ Complexity 638 631 -7
============================================
Files 54 60 +6
Lines 1751 1819 +68
============================================
+ Hits 1337 1518 +181
+ Misses 414 301 -113 |
5dae87b to
8aa1aa7
Compare
a15ece1 to
4f7b889
Compare
a71fed0 to
86bea89
Compare
f470290 to
bb9c926
Compare
src/XML/SignedElementInterface.php
Outdated
| * @param \SimpleSAML\XMLSecurity\XMLSecurityKey $key The key we should check against. | ||
| * @return bool True if successful, false if we don't have a signature that can be verified. | ||
| */ | ||
| public function validate(XMLSecurityKey $key): bool; |
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 think, in order to encourage good practices, and also to avoid any possible implementation errors (e.g. we have a signed element, and we just call getElement() to get the unsigned element without validating the signature first), the validate() method should return the actual element whose signature has been verified.
That would mean that when the signature validation fails, we should throw an exception (which is a good way to ensure the failure is noticeable, I believe), instead of returning false. It would also prevent us from having any processing security issue. We'll need support from xmlseclibs or our fork though to get this done, since verify should call fromXML() on the target class (the signable element), passing the actual, canonicalised an transformed DOM (not the original), and as far as I recall, that's not possible with xmlseclibs.
src/XML/SignedElementTrait.php
Outdated
| public function getValidatingCertificates(): array | ||
| { | ||
| $ret = []; | ||
| foreach ($this->signature->getCertificates() as $cert) { |
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 think this can be quite expensive. Would this be an alternative approach?
- Require
validate()before callinggetValidatingCertificates(). If we call the latter first, raise an exception. - During
validate(), if the signature validation succeeds with the given key, check if it corresponds to any of the certificates stored internally. Build a list of validating certificates with all where the passed key corresponds to them. - When calling
getValidatingCertificates()aftervalidate(), return the list we just built. If none of the certificates in the signature correspond to the given key, or no certificates where found, we just return an empty list.
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
These new documents can be used in tests to verify different features. Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
We got rid of the initial extra toSignedXML() method. If we want to sign an element, we should just call sign() on it, then get the result with toXML(). This means it will be toXML() the method that actually does the signing, while sign() will simply prepare everything for it and signal that the element should be signed when calling toXML(). Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
This commit adds support for verifying signatures (and renames the API method to verify(), which is the correct term we should us in this context, instead of validate()). It also refactors the way we sign, and adds support for canonicalization algorithms with comments, which were in practice not supported before. Signed-off-by: Jaime Pérez <jaimepc@gmail.com>
No description provided.