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

Fix ECC PKey name to OID #80

Closed

Conversation

vdukhovni
Copy link

The encodePK function in Data/X509/PublicKey.hs used to only support secp384r1. It now supports all the curves listed in curvesOIDTable. Also took the opportunity to convert the table to a Bimap and drop some curves that are rather obsolete/insecure from that mapping. Further pruning may be wise.

@vdukhovni vdukhovni mentioned this pull request Dec 1, 2016
@vincenthz
Copy link
Collaborator

the first bit is fine, but please don't add dependency and also don't make policy choice in certificate.

  • bimap is not necessary and pull a lots of unwanted dependency (template-haskell, exceptions), and is probably of questionable gain here (2 tree maps) for iterating over a 20 elements list; very unlikely to be faster and leaner.
  • don't drop insecure curves from certificate. certificate should have minimal policy, and should be about decoding / encoding certificate. I definitely agree that no one should use those curves, but to be able to encode/decode certificate using those curves is important.

@vdukhovni
Copy link
Author

I've reverted the addition of Bimap and (at least for now) restored the removed weak curves.

I should say, on the OpenSSL team I've often argued for essentially your position (that even weak obsolete code-points should be retained in libcrypto to maintain backwards-compatibility, giving users access to previously created data). In this case however, I really don't think that these curves were ever used (especially via Haskell code) for processing any real data.

Still, what's really missing perhaps is code to assign a security floor for certificate verification in TLS. Perhaps the curve list tuples needs a third element that assigns the curves an an approximate bit strength (0 for most of them), so that the TLS layer can conditionally impose a floor on that value...

@vdukhovni vdukhovni force-pushed the fix-ecc-pkey-name-to-oid branch 2 times, most recently from dfe610a to dea14b8 Compare December 1, 2016 17:29
@vincenthz
Copy link
Collaborator

vincenthz commented Dec 2, 2016

I agree that they were very unlikely used but unless they really turn into a massive burden, then I don't think we should just remove it

FYI, at the tls level I want to add a default sensible security floor with a system to override some per-host security floors with system level and user level configuration files. However I can't add any default floor without having the overriding mechanism in place for users that will undoubtly need it.

@vdukhovni
Copy link
Author

vdukhovni commented Dec 3, 2016

@vincenthz FYI, at the tls level I want to add a default sensible security floor ...

Yes, we are on the same wavelength, and indeed the floor would have to be user configurable, with sane defaults. For example, opportunistic TLS in SMTP needs a very permissive floor (probably no restrictions at all), while HTTPS to some sensitive site may want to ensure a strong enough leaf public key and strong signatures (public key and hash algorithm) up the certificate chain (except any self-signature of a self-signed trust-anchor).

In OpenSSL (not always the best model of excellence, but not always wrong either) version 1.1.0 there is a new security level feature which roughly corresponds to NIST SP800-57 bit strengths. I'd be curious to know what you think of that sort of approach: https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_set_security_level.html

In the mean-time I hope that this PR is good to go. Please let me know if there are any further changes you'd like to see. Perhaps this one first, and then the TLS one that adds the missing bits for for SHA384, ... and shows promise for better TLS12 interoperability, including ECDSA.

@vdukhovni
Copy link
Author

This appears to have been merged. Thanks.

@vdukhovni vdukhovni closed this Dec 3, 2016
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.

2 participants