Skip to content

Conversation

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Jan 11, 2021

No description provided.

@tvdijen tvdijen force-pushed the signedelts branch 5 times, most recently from 07f62c1 to 0fb165d Compare January 11, 2021 20:01
@codecov
Copy link

codecov bot commented Jan 11, 2021

Codecov Report

Merging #2 (32ac524) into master (538c92c) will increase coverage by 7.09%.
The diff coverage is 80.95%.

@@             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     

@tvdijen tvdijen force-pushed the signedelts branch 4 times, most recently from 5dae87b to 8aa1aa7 Compare January 11, 2021 21:27
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from a15ece1 to 4f7b889 Compare January 15, 2021 20:02
@tvdijen tvdijen force-pushed the signedelts branch 2 times, most recently from a71fed0 to 86bea89 Compare January 15, 2021 21:34
@tvdijen tvdijen force-pushed the signedelts branch 2 times, most recently from f470290 to bb9c926 Compare January 16, 2021 14:43
* @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;
Copy link
Member

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.

public function getValidatingCertificates(): array
{
$ret = [];
foreach ($this->signature->getCertificates() as $cert) {
Copy link
Member

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?

  1. Require validate() before calling getValidatingCertificates(). If we call the latter first, raise an exception.
  2. 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.
  3. When calling getValidatingCertificates() after validate(), 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.

tvdijen and others added 14 commits July 23, 2021 21:32
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>
@tvdijen tvdijen merged commit dc430d7 into master Jul 24, 2021
@tvdijen tvdijen deleted the signedelts branch July 24, 2021 17:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2023
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 this pull request may close these issues.

3 participants