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 OpenSSL support for transport encryption; #68

Closed
wants to merge 1 commit into from

Conversation

aiy
Copy link

@aiy aiy commented May 5, 2015

It adds configuration option (--enable-openssl) to
enable OpenSSL library usage instead of NSS one.
OpenSSL support is provided mostly for FIPS compliance.

It was tested with following options:
1.
crypto_hash: sha256
crypto_cipher: aes128
2.
crypto_hash: none
crypto_cipher: none

Test code is not provided

@fabbione
Copy link
Member

Hi,

thanks for the submission. I have a few comments and questions before I am ready to merge this patch.

is it really necessary to select nss or openssl at build time? Is it possible to have the corosync binary link against both libraries and have the user select at runtime instead? crypto_model: nss|openssl (or whatever config keyword fits best).

If possible then we can re-use the ssl plugin selector from kronosnet project on github (the crypto implementation is shared with corosync but kronosnet one had a thin plugin layer to select crypto library like openssl or nss)

If not possible, since the openssl implementation and the nss one appears to be mutual exclusive, doesn't it make more sense to have the openssl implementation in another file and build/link the selected one instead?

Also, the test suite (cpgverify etc) implementation needs to be completed and I would like to see this tested against a wider range of crypto/hashing algorithms.

Once this patch is ready for merge, I would like the permission of the authors to reuse the code in the kronosnet project too (since the crypto code is pretty much shared with corosync).

Thanks
Fabio

@aiy
Copy link
Author

aiy commented May 19, 2015

Hi Fabio,

If user wants to have both nss and openssl built in then: 1) additional configuration option is required, 2) code (including original corosync transport code) must have a major change. It could be done in the future, 3) running both libraries (nss and openssl) could break FIPS compliance explained right after this answer.

As I mention earlier, the main reason for that code is FIPS compliance (http://en.wikipedia.org/wiki/Federal_Information_Processing_Standards). libnss is not compliant at the moment. openssl is compliant with FIPS. Usage of any additional layers could add possible security issues to the code.

I'll add test cases for other crypto and hashing algorithms.

Again, the code is simple and minimal change to make corosync FIPS compliant.

Also, corosync-keygen generates key which is too weak. 'openssl rand' like tools can generate more stronger key. Usage of corosync-keygen also makes corosync not compliant with FIPS.

Thanks,
Alexey

@fabbione
Copy link
Member

On 05/19/2015 04:25 AM, Alexey Yakimovich wrote:

Hi Fabio,

If user wants to have both nss and openssl built in then: 1) additional
configuration option is required,

Not necessarily, you can autodetect if a library is available or not.
But it's a minor detail.

  1. code (including original corosync
    transport code) must have a major change. It could be done in the
    future,

Not really not. I have done this already in another project and the
layer in between is minimal.

  1. running both libraries (nss and openssl) could break FIPS
    compliance explained right after this answer.

As I mention earlier, the main reason for that code is FIPS compliance
(http://en.wikipedia.org/wiki/Federal_Information_Processing_Standards).
libnss is not compliant at the moment.

http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140val-all.htm

says otherwise.

openssl is compliant with FIPS.
Usage of any additional layers could add possible security issues to the
code.

Hardly. The layer involves only a function redirection with no code or
data processing. Take this one for example:

https://github.com/fabbione/kronosnet/blob/master/libknet/crypto.c

The init simply generates a table of functions that are exported to
upper layers and map them to the implementation below. There is no
processing as you can see.

corosync transports require crypt_and_sign and verify_and_decryt
functions. Abstract those into a the thin layer (so no changes on above
layers) and the thin layer maps them to nss_ or ossl_ versions exported
from the totetmcrypto_nss.c or totemcrypto_ossl.c.

I'll add test cases for other crypto and hashing algorithms.

Super.

Again, the code is simple and minimal change to make corosync FIPS
compliant.

See above.

Also, corosync-keygen generates key which is too weak. 'openssl rand'
like tools can generate more stronger key. Usage of corosync-keygen also
makes corosync not compliant with FIPS.

Do you have some hard data on this specific issue?

Thanks
Fabio

Thanks,
Alexey


Reply to this email directly or view it on GitHub
#68 (comment).

@fabbione
Copy link
Member

On 05/19/2015 04:25 AM, Alexey Yakimovich wrote:

Hi Fabio,

If user wants to have both nss and openssl built in then: 1) additional
configuration option is required, 2) code (including original corosync
transport code) must have a major change. It could be done in the
future, 3) running both libraries (nss and openssl) could break FIPS
compliance explained right after this answer.

As I mention earlier, the main reason for that code is FIPS compliance
(http://en.wikipedia.org/wiki/Federal_Information_Processing_Standards).
libnss is not compliant at the moment. openssl is compliant with FIPS.
Usage of any additional layers could add possible security issues to the
code.

I'll add test cases for other crypto and hashing algorithms.

Again, the code is simple and minimal change to make corosync FIPS
compliant.

Also, corosync-keygen generates key which is too weak. 'openssl rand'
like tools can generate more stronger key. Usage of corosync-keygen also
makes corosync not compliant with FIPS.

Oh and just to make it clear, I am very happy about merging openssl
support in corosync. I just would like to see it merged properly. My
objections so far are only related to flexibility and to keep the
implementation with nss separated basically.

Also, could you please answer my question about permission to re-use the
code in the other project I mentioned before?

Thanks
Fabio

@jfriesse
Copy link
Member

In current master we no longer have encryption implementation because we get it for free by using knet. If this pull request still applies please consider port it to https://github.com/fabbione/kronosnet

@jfriesse jfriesse closed this Oct 17, 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.

None yet

3 participants