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

Support ECC Certificate Creation #3649

Closed
wants to merge 8 commits into from

Conversation

johnaheadley
Copy link
Contributor

This is part 1 of adding support for generating certificates using EC keys as requested in #2908

This is a work in progress.

@johnaheadley
Copy link
Contributor Author

A couple of things to explain with this PR:

  1. Currently only CA creation under System > Trust > Authorities supports selecting EC key type. System > Trust > Certificates will be a Part 2.

  2. I added the function system_trust_configure into system_camanager.php temporarily because I kept getting the error "Fatal error: Uncaught Error: Call to undefined function system_trust_configure()" ... If anyone can assist troubleshooting that, that would be great!

  3. There is no input validation on the new fields yet.

  4. The OpenVPN Wizard does not allow you to create a CA with EC key type yet.

@AdSchellevis AdSchellevis self-assigned this Aug 16, 2019
@johnaheadley
Copy link
Contributor Author

2 is fixed - it was an issue specific to my VM.
3 is added now following suit with the rest of the code, though not sure how to test.

@johnaheadley
Copy link
Contributor Author

Added Part 2 mentioned in item 1 of my original comment -> support for EC key type during certificate creation under System > Trust > Certificates. Includes input validation code as well.

Items outstanding:

  • Still need to add EC key type option to OpenVPN Wizard's CA and cert creation steps
  • Need to fix table row coloring. Just hiding the rows leads to an issue with the banded coloring of the table. See below:

Screen Shot 2019-08-25 at 12 23 38 PM

@johnaheadley
Copy link
Contributor Author

Alright, I fixed the color banding issue.

I want to do the OpenVPN Wizard changes on a different PR if that is ok. The OpenVPN Wizard is working fine, you just don't have the EC option.

That being said, @AdSchellevis please review when you get time.

@AdSchellevis
Copy link
Member

@johnaheadley I don't have a lot of time to review at the moment, but looking at the cert creating code (ca_create() and cert_create()) I think we could simplify the world a bit by removing the $keytype and $curve, renaming $keylen (maybe $keylen_curve) and detect its a curve based on $digest_alg or a non integer "keylen".

openssl_digest_algs and ec_curves look quite similar now, which adds quite some glue, which might not be absolutely necessary (since a cert can only be of one type at a time).

@johnaheadley
Copy link
Contributor Author

ok makes sense. I will work on removing $keytype and $curve and just using the original $keylen variable to hold both the key lengths and the curves.

@AdSchellevis
Copy link
Member

@johnaheadley any news on this?

@johnaheadley
Copy link
Contributor Author

@AdSchellevis sorry to disappear on you guys! I will work on the changes we discussed this weekend

@johnaheadley
Copy link
Contributor Author

@AdSchellevis added commits with the requested changes

AdSchellevis added a commit that referenced this pull request Nov 19, 2019
@AdSchellevis
Copy link
Member

@johnaheadley thanks, some small fixes, which I've pushed in a separate branch. The ca part doesn't appear to be working yet on my end, when creating a new ca, it fails with:

The following input errors were detected:

openssl library returns: error:0E06D06C:configuration file routines:NCONF_get_string:no value
openssl library returns: error:0E06D06C:configuration file routines:NCONF_get_string:no value

Can you check the new branch (https://github.com/opnsense/core/tree/ec-certs) and see if there's anything missing?

My changes are in 018149c

@johnaheadley
Copy link
Contributor Author

Hey @AdSchellevis, thanks for doing the style sweep! Your changes look great.

I found the issue with your updates that broke ca_create for EC keytypes - line 151 needs to be removed. private_key_bits should not be in the array by default since it can't be present ifprivate_key_type will be OPENSSL_KEYTYPE_EC. After this line is removed then CA creation for EC type will work.

The other cert creation functions, cert_create, csr_generate, and ca_inter_create don't need any changes.

@AdSchellevis
Copy link
Member

@johnaheadley thanks, I totally overlooked, I will fix that one and merge the set to master.

AdSchellevis added a commit that referenced this pull request Nov 20, 2019
AdSchellevis added a commit that referenced this pull request Nov 20, 2019
Support ECC Certificate Creation (#3649)
@AdSchellevis
Copy link
Member

closed in 02caac7

fichtner pushed a commit that referenced this pull request Dec 16, 2019
Support ECC Certificate Creation (#3649)

(cherry picked from commit 02caac7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants