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

pkcs11-tool: various improvements: extensions, bug fixes, and cleanup #747

Closed
wants to merge 5 commits into from

Conversation

@DDvO
Copy link
Contributor

DDvO commented Apr 27, 2016

This is a collection of improvements I recently implemented on pkcs11-tool and would like to share for the benefit of other users:

  1. added capability to (auto-detect and) read certificate files in PEM format
  2. added capability to read RSA and EC public keys from module/card
  3. major bugfix on writing EC public key objects, introducing parse_ec_pkey()
  4. bugfix on new session created by test_kpgen_certwrite()
  5. two bugfixes w.r.t. leading zeros in hex_to_bin() input and output
  6. made --keypairgen and --test-ec more permissive on minor card problems
  7. added hints on missing options for --test-ec and --moz-cert
  8. added cleanup of key pairs and cert written by --test-ec and --moz-cert
  9. improved --test to skip non-RSA keys in a clean way
  10. added hint on invalid command line options
  11. improved diagnostic output: use stderr for error messages, avoid duplicate newlines, etc.
  12. added CKM_ECDSA_SHA224 ... CKM_ECDSA_SHA512
  13. simplified do_read_key()
  14. re-factored implicit parameter of RSA_GET_BN() macro
  15. several minor simplifications of write_object()

I am aware that this sums up to quite a number of items, where some 170 lines have been changed and about 230 lines have been added. Yet it would be pretty time consuming to break this up into many per-topic pull requests, while this overhead would not add anything in terms of productivity.

All changes, and I hope also all additions, should be pretty straightforward to understand
So please have a look and take over whatever you consider useful. I am available for answering any questions and comments.

@frankmorgner

This comment has been minimized.

Copy link
Member

frankmorgner commented May 3, 2016

your code does not compile, see the CI links above

… (corrections w.r.t. #ifdef ENABLE_OPENSSL)
@DDvO

This comment has been minimized.

Copy link
Contributor Author

DDvO commented May 3, 2016

So far I had thought the partial failures of the automated builds were due to unrelated architecture issues. Yet I just found that the code did not compile in case ENABLE_OPENSSL is not defined.
Corrected.

@frankmorgner

This comment has been minimized.

Copy link
Member

frankmorgner commented May 3, 2016

please split your commits into smaller ones with changes that logically belong together and fix your commit message with a summary of those changes. Review and rollback in case of problem is much easier in small chunks!

@frankmorgner

This comment has been minimized.

Copy link
Member

frankmorgner commented May 3, 2016

DDvO added 2 commits May 4, 2016
… (workaround for variable scope bug in gcc)
… (correction w.r.t. defined(OPENSSL_NO_EC))
@DDvO

This comment has been minimized.

Copy link
Contributor Author

DDvO commented May 4, 2016

Thanks @frankmorgner for your hint on a point where the compilation went wrong.
At that position the compiler confused a local variable named 'out' with another local variable that I introduced in an inner scope and that happened to have the same name. This must be due to a compiler bug; as a workaround I renamed the newly introduced variable.

After two more iterations, where I corrected two further problems with conditional compilation (this time related to the definedness of OPENSSL_NO_EC) I am now pretty sure that the code is free of static errors.

The Travis Cl build confirms this, while the AppVeyor build still has some trouble related to nmake and linker errors like LNK2001: unresolved external symbol ___iob_func. Yet these also happen for parts of the build unrelated to pkcs11-tool.c and thus I suppose that the AppVeyor configuration should be fixed.
Update: meanwhile a green tick is shown indicating that all checks have passed, although the internal AppVeyor issues remain, which can be seen when opening the details.

@frankmorgner

This comment has been minimized.

Copy link
Member

frankmorgner commented May 4, 2016

CI is good now, but I fear you'll have to split up the commits...

@frankmorgner

This comment has been minimized.

Copy link
Member

frankmorgner commented Jun 25, 2016

@DDvO did you make any progress? without the splitup we can't review your changes

@DDvO

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Jun 28, 2016

Thanks for asking. I cannot spend much further time on it, but will try to do soon.

BTW, my motivation to contribute to this project dropped a lot after my frustrating #737 experience.

viktorTarasov added a commit that referenced this pull request Jun 29, 2016
========================================
rebased by VTA -- commits are forged to one,
excluding the following chunk
(reason -- if not explicitely indicated, the mechanism has to be found out using the mechanism flags):

@@ -1713,8 +1713,9 @@ static int gen_keypair(CK_SLOT_ID slot, CK_SESSION_HANDLE session,
                        int ii;

                        if (!opt_mechanism_used)
+                               opt_mechanism = CKM_EC_KEY_PAIR_GEN;
                                if (!find_mechanism(slot, CKF_GENERATE_KEY_PAIR, mtypes, mtypes_num, &opt_mechanism))
-                                       util_fatal("Generate EC key mechanism not supported\n");
+                                       util_warn("Generate EC key mechanism not listed as supported");

                        for (ii=0; ec_curve_infos[ii].name; ii++)   {
                                if (!strcmp(ec_curve_infos[ii].name, type + 3))

will close PR #747
@viktorTarasov

This comment has been minimized.

Copy link
Member

viktorTarasov commented Jun 30, 2016

essential part applied in 4441efa,
thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.