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

Chunk 12 of CMP contribution to OpenSSL: CLI-based test #11998

Closed
wants to merge 25 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented May 30, 2020

Finally the last chunk of the CMP contribution,
adding numerous tests run via the cmp app.

These tests address all previous contribution chunks:

For part of the test cases the CMP client side is sufficient,
while many tests naturally also require a CMP server.
Therefore we made sure that our contribution contains
a simple CMP server that is sufficient for testing purposes.

The test bed is sufficiently flexible to support addressing also externals servers.
Throughout our development we have been testing against various EJBCA instances,
the Demo CA of the Insta Certifier, and Siemens-internal prototype RA implementations.

While updating and consolidating the many test cases that we have compiled earlier
I came across various issues mostly with the CMP code contributed so far but also
with the internal app library. So this PR also contains commits implementing fixes for
those as far as needed for successfully running all the CMP tests added in this PR.

The high number of lines added by the last commit is mostly due to large test data files.

Checklist
  • documentation is added or updated
  • tests are added or updated

@DDvO
Copy link
Contributor Author

DDvO commented May 30, 2020

Can we have the Perl module Proc::Background installed on the build systems?
I even tried system ("perl -MCPAN -e \"install 'Proc::Background'\"") from within the tests
but no luck.
This module would offer a very nice system-independent way of launching the CMP test server in the background and shutting it down at the end of 81-test_cmp_cli.t.

So far I'm using the system("cmd &") pattern but of course this does not work on Windows.
Meanwhile I've disabled the tests for Windows and made sure they launch fine also on Mac OS.

@DDvO
Copy link
Contributor Author

DDvO commented May 31, 2020

The Travis s390x build keeps timing out after 50 minutes; everything else works fine.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 9, 2020

Again my question:
what is the preferred way of launching a test server to run in parallel to a client and shutting it down at the end of the test?
There would be a nice likely system-independent Perl module for this purpose: Proc::Background, but at least so far it is not available on the build systems.

So I had to restrict these sort of tests to Unix-like systems, where the shell ampersand operator & for running commands in the background, the lsof command, and the kill command are available.

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the preferred way of launching a test server to run in parallel to a client and shutting it down at the end of the test?

We do this in util/perl/TLSProxy.pm where we start instances of s_server and s_client and get them to talk to each other. You might like to take a look how it is done there.

apps/lib/opt.c Show resolved Hide resolved
crypto/cmp/cmp_server.c Outdated Show resolved Hide resolved
doc/internal/man3/ossl_cmp_msg_check_update.pod Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man3/OSSL_CMP_SRV_CTX_new.pod Outdated Show resolved Hide resolved
test/recipes/81-test_cmp_cli.t Outdated Show resolved Hide resolved
test/recipes/81-test_cmp_cli.t Outdated Show resolved Hide resolved
DDvO added 22 commits June 10, 2020 21:57
…k_update()

Bugfix: allow using extraCerts contained in msg already while checking signature
Improve function name, simplify its return value, and update its documentation
Also adds ossl_cmp_hdr_get_protection_nid() simplifying cmp_vfy.c
as checking expected_sender and adding caPubs is not part of msg validation.
Also constify a couple of internal and public functions related to cmp_vfy.c
Certificate Management Protocol (CMP, RFC 4210) extension to OpenSSL
Also includes CRMF (RFC 4211) and HTTP transfer (RFC 6712).
Adds the CMP and CRMF API to libcrypto and the "cmp" app to the CLI.
Adds extensive documentation and tests.
@DDvO
Copy link
Contributor Author

DDvO commented Jun 10, 2020

Thanks @mattcaswell for your yesterday's review comments!
I've handled all of them, rebased the PR, and squashed the commits where it made sense.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 10, 2020

what is the preferred way of launching a test server to run in parallel to a client and shutting it down at the end of the test?

We do this in util/perl/TLSProxy.pm where we start instances of s_server and s_client and get them to talk to each other. You might like to take a look how it is done there.

Thanks @mattcaswell for this hint.
I've meanwhile had a look at util/perl/TLSProxy/Proxy.pm
and its use in test/recipes/70-test_*.t.
This appears pretty cumbersome for the basic use needed in 81-test_cmp_cli.t, and it would be hard to extract a simple mechanism for just launching any process in the background, checking if it is running, and shutting it down later.

With the Perl module Proc::Background it would be very nice and straightforward:

use Proc::Background;

# launch the server:
my $proc = Proc::Background->new({'die_upon_destroy' => 1}, "$cmd $args");
die "Cannot start server" unless $proc;

# interact with the server

# shut down the server:
$proc = undef;

As mentioned above, since unfortunately Proc::Background is not available (by default at least),
I've resorted to using instead the shell ampersand operator & for running commands in the background, the lsof command for obtaining its pid, and the kill command.
Due to this implementation I had to restrict the server-based to tests to Unix-like systems.

Would this be acceptable for the time being,
as long as apparently no general and simple-to-use solution is available?

openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
…g numbers

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
…ate_msg()

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
…k_update()

Bugfix: allow using extraCerts contained in msg already while checking signature
Improve function name, simplify its return value, and update its documentation

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Also adds ossl_cmp_hdr_get_protection_nid() simplifying cmp_vfy.c

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
…ted sender

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
as checking expected_sender and adding caPubs is not part of msg validation.
Also constify a couple of internal and public functions related to cmp_vfy.c

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
… request template

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2020
Certificate Management Protocol (CMP, RFC 4210) extension to OpenSSL
Also includes CRMF (RFC 4211) and HTTP transfer (RFC 6712).
Adds the CMP and CRMF API to libcrypto and the "cmp" app to the CLI.
Adds extensive documentation and tests.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11998)
@DDvO
Copy link
Contributor Author

DDvO commented Jun 13, 2020

Merged - thanks @mattcaswell!

@DDvO DDvO closed this Jun 13, 2020
@romen
Copy link
Member

romen commented Jun 13, 2020

@DDvO I am seeing this error compiling with ubsan on:

../../home/tuveri/repos/ossl_committer/workbench/crypto/cmp/cmp_msg.c:259:45: runtime error: signed integer overflow: 86400 * 36500 cannot be represented in type 'int'                                                                 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../home/tuveri/repos/ossl_committer/workbench/crypto/cmp/cmp_msg.c:259:45 in                                                                                                 
../../../../../../../../../tmpram/ossl_build/util/wrap.pl ../../../../../../../../../tmpram/ossl_build/apps/openssl cmp -config ../Mock/test.cnf -section 'Mock enrollment' -proxy '' -cmd ir -newkey new.key -newkeypass 'pass:' -days 3
6500 -certout test.cert.pem -out_trusted root.crt => 1                                                                                                                                                                                  
    not ok 23 - days 36500                                                                                                                                                                                                              
                                                                                                                                                                                                                                        
    #   Failed test 'days 36500'                                                                                                                                                                                                        
    #   at ../../home/tuveri/repos/ossl_committer/workbench/test/recipes/81-test_cmp_cli.t line 171.                                                                                                                                    
cmp: Can't parse "-certout" as a number                                                                                                                                                                                                 
OPENSSL_FUNC:../../home/tuveri/repos/ossl_committer/workbench/apps/cmp.c:2848:CMP error: use -help for summary of 'cmp' options                                                                                                         
# OPENSSL_FUNC:../../home/tuveri/repos/ossl_committer/workbench/apps/cmp.c:2892:CMP info: using OpenSSL configuration file '../Mock/test.cnf'                                                                                           
# OPENSSL_FUNC:../../home/tuveri/repos/ossl_committer/workbench/apps/cmp.c:2498:CMP warning: argument of -proxy option is empty string, resetting option                                                                                
../../../../../../../../../tmpram/ossl_build/util/wrap.pl ../../../../../../../../../tmpram/ossl_build/apps/openssl cmp -config ../Mock/test.cnf -section 'Mock enrollment' -proxy '' -cmd ir -newkey new.key -newkeypass 'pass:' -days -
certout test.cert.pem -out_trusted root.crt => 1                                                                                                                                                                                        
    ok 24 - days missing arg

@DDvO
Copy link
Contributor Author

DDvO commented Jun 17, 2020

@romen, I've been able to reproduce this, having configured with enable-uban.
This will be fixed by #12175.

Among others, I've improved the range checking. A too large -days option value now would give

make test VFO=1 TESTS="test_cmp_cli" 
make depend && make _tests
make[1]: Entering directory '/home/david/Siemens/proj/PKI/CMP/CMPforOpenSSL/cmpossl_incremental12'
make[1]: Leaving directory '/home/david/Siemens/proj/PKI/CMP/CMPforOpenSSL/cmpossl_incremental12'
make[1]: Entering directory '/home/david/Siemens/proj/PKI/CMP/CMPforOpenSSL/cmpossl_incremental12'
( SRCTOP=. \
  BLDTOP=. \
  PERL="/usr/bin/perl" \
  EXE_EXT= \
  /usr/bin/perl ./test/run_tests.pl test_cmp_cli )

81-test_cmp_cli.t .. 6/? OPENSSL_FUNC:apps/cmp.c:1886:CMP error: -days argument is too large: resulting epoch value exceeds INT_MAX
OPENSSL_FUNC:apps/cmp.c:3055:CMP error: cannot set up CMP context
# OPENSSL_FUNC:apps/cmp.c:2895:CMP info: using OpenSSL configuration file '../Mock/test.cnf'
# OPENSSL_FUNC:apps/cmp.c:2500:CMP warning: argument of -proxy option is empty string, resetting option
# OPENSSL_FUNC:apps/cmp.c:2112:CMP info: will contact http://127.0.0.1:1700/pkix/
# OSSL_CMP_CTX_set_option:crypto/cmp/cmp_ctx.c:945:CMP error: value too large
../../../../util/wrap.pl ../../../../apps/openssl cmp -config ../Mock/test.cnf -section 'Mock enrollment' -proxy '' -cmd ir -newkey new.key -newkeypass 'pass:' -days 36500 -certout test.cert.pem -out_trusted root.crt => 1
    not ok 23 - days 36500
    # Looks like you failed 1 test of 93.
not ok 7 - CMP app CLI Mock enrollment
81-test_cmp_cli.t .. 7/? # 

#   Failed test 'CMP app CLI Mock enrollment
# '
#   at /home/david/Siemens/proj/PKI/CMP/CMPforOpenSSL/cmpossl_incremental12/util/perl/OpenSSL/Test.pm line 1302.
# Looks like you failed 1 test of 7.
81-test_cmp_cli.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/7 subtests 

Test Summary Report
-------------------
81-test_cmp_cli.t (Wstat: 256 Tests: 7 Failed: 1)
  Failed test:  7
  Non-zero exit status: 1
Files=1, Tests=7, 65 wallclock secs ( 0.17 usr  0.01 sys + 20.67 cusr  5.28 csys = 26.13 CPU)
Result: FAIL

@mouse07410
Copy link
Contributor

@DDvO please see #12145. #12175 (at least, the way it is now) did not seem to help. :-(

@DDvO DDvO mentioned this pull request Jun 18, 2020
@DDvO
Copy link
Contributor Author

DDvO commented Jun 18, 2020

@DDvO please see #12145. #12175 (at least, the way it is now) did not seem to help. :-(

I've just fixed this in a new commit in #12175.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants