Skip to content
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

Add RSA MGF1 signature types #200

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

liamdennehy
Copy link

@liamdennehy liamdennehy commented Sep 9, 2019

I have no idea what I am doing! Someone with crypto expertise please check my work! 😄

It seems OAEP and MGF1 refer to the same (or are simply included) underlying functions, so simply extending the XMLSecurityKey class with the new definitions in RFC6931#Section-2.3.10 does the job, but maybe it's not that simple.

Tested to correctly verify against sample in #199, unable to test generation, so no units included either.

liamdennehy added a commit to liamdennehy/eidas-certificate-parse that referenced this pull request Sep 9, 2019
Relies on PR robrichards/xmlseclibs#200
so explicitly depending on commit in the meantime
@peter279k
Copy link

The Travis CI build is failed because the default dist has been changed.

According to the Travis CI blog post, the default dist change into xenial, not trusty.

To let php-5,4 to php-5.6 versions be supported correctly, defining the matrix setting and let these versions on that. Then setting the dist is trusty.

@liamdennehy
Copy link
Author

Fixed the trusty issue, but this requires more work:

$ if [[ $EXECUTE_COVERAGE != 'true' ]]; then phpunit tests; fi
PHPUnit 7.0.2 by Sebastian Bergmann and contributors.

This version of PHPUnit is supported on PHP 7.1 and PHP 7.2.
You are using PHP 7.0.27 (/home/travis/.phpenv/versions/7.0.27/bin/php).
The command "if [[ $EXECUTE_COVERAGE != 'true' ]]; then phpunit tests; fi" exited with 1.

No idea why the version of phpunit that is included in a travis vm is incompatible with the version of php in the same vm.

@peter279k
Copy link

peter279k commented Sep 16, 2019

Hi @liamdennehy, thanks for your concern.

The PHPUnit version problem is about internal Travis CI environment.

Some useful discussion is here.

To avoid this internal Travis problem, I suggest we can consider using the vendor/bin/phpunit version directly rather than using complicated condition to dispatch different works for phpunit at this moment.

@liamdennehy
Copy link
Author

Last three commits have been addressing that, 4857aeb provides for composer install when required and fe4c990 includes better script readability. Build times have unfortunately increased, but still tolerable.

https://travis-ci.org/robrichards/xmlseclibs/builds/585488348 is now a pleasant shade of green...

@liamdennehy
Copy link
Author

Note This PR still has no tests, and especially has only been made to validate, but not produce the required signature in my own exemplar.

@liamdennehy
Copy link
Author

rather than using complicated condition to dispatch different works for phpunit

I don't think the new scripts complicate things too much, though this is the first dependency in the project's travis build so I only require when actually necessary. I can propose a move to composer's phpunit exclusively, though I don't know how that intersects with the coverage tests in 7.0.27, and is outside the scope of this PR's purpose.

@peter279k
Copy link

rather than using complicated condition to dispatch different works for phpunit

I don't think the new scripts complicate things too much, though this is the first dependency in the project's travis build so I only require when actually necessary. I can propose a move to composer's phpunit exclusively, though I don't know how that intersects with the coverage tests in 7.0.27, and is outside the scope of this PR's purpose.

Yes. I think it's about another PR and issue.

@7118path
Copy link

I could not get this to work correctly on an external created signature, therefore used this code as base for another pullrequest #222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants