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

DAOSSDE-112 Security: Modifications to use of Cryptography #3923

Merged
merged 5 commits into from
Nov 24, 2020

Conversation

dpquigl
Copy link
Contributor

@dpquigl dpquigl commented Nov 21, 2020

After peer review the following modifications were requested.

  • Remove cipher suite AES128-GCM-SHA256 and just offer AES256-GCM-SHA384 as
    default.

  • Reduce certificates to one year lifetime (currently three years)

  • Replace PKCS Adding a .travis.yml file to trigger CI builds. #1 v1.2 with PKCS Adding a .travis.yml file to trigger CI builds. #1 v2.1/2 if possible. The golang core library
    is being used and may not support it yet. Team to verify if it does and, if so,
    will try to update.

  • Add to documentation recommending to customer to use the latest version of
    OpenSSL >= 1.1.1h. For this project, the customer is responsible for generating
    keys/certs.

  • Reduce key sizes to 3072 based on expected lifetime of this product

Signed-off-by: David Quigley [email protected]

After peer review the following modifications were requested.

* Remove cipher suite AES128-GCM-SHA256 and just offer AES256-GCM-SHA384 as
default.

* Reduce certificates to one year lifetime (currently three years)

* Replace PKCS #1 v1.2 with PKCS #1 v2.1/2 if possible. The golang core library
is being used and may not support it yet. Team to verify if it does and, if so,
will try to update.

* Add to documentation recommending to customer to use the latest version of
OpenSSL >= 1.1.1h. For this project, the customer is responsible for generating
keys/certs.

* Reduce key sizes to 3072 based on expected lifetime of this product

Signed-off-by: David Quigley <[email protected]>
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/2/execution/node/273/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on CentOS 7 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/2/execution/node/329/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/2/execution/node/280/log

@daosbuild1
Copy link
Collaborator

Copy link
Contributor

@mjmac mjmac left a comment

Choose a reason for hiding this comment

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

The switch to PSS makes sense, as does the removal of support for the smaller cipher suites. I'm wondering what the motivation is behind decreasing the key size and CA validity period, though.

@@ -115,10 +115,10 @@ extendedKeyUsage = clientAuth
function generate_ca_cert () {
echo "Generating Private CA Root Certificate"
# Generate Private key and set permissions
openssl genrsa -out "${PRIVATE}/daosCA.key" 4096
openssl genrsa -out "${PRIVATE}/daosCA.key" 3072
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the drop in key size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I'm not entirely sure. This was the guidance from the crypto team. I'd imagine its due to the fact that 3072 bit keys are computationally difficult enough to break and since its software not hardware that we can update the number if that turns out not to be true in the future.

Copy link
Contributor

@GitHuaKuang GitHuaKuang Nov 23, 2020

Choose a reason for hiding this comment

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

I think probably a balance between performance and security.
This is what I got on boro-59, there is no option for rsa 3072 size, but it shows the CPU time for the key size:

[boro-59:/home/huakuang]$ openssl speed rsa2048 rsa4096
sign verify sign/s verify/s
rsa 2048 bits 0.000791s 0.000033s 1264.8 30152.7
rsa 4096 bits 0.007899s 0.000121s 126.6 8276.1

chmod 0400 "${PRIVATE}/daosCA.key"
# Generate CA Certificate
openssl req -new -x509 -config "${CA_HOME}/ca.cnf" -days 1095 -sha512 \
openssl req -new -x509 -config "${CA_HOME}/ca.cnf" -days 365 -sha512 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the CA cert only valid for a year?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is considered best practice for internally run and managed CAs.

Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/3/execution/node/342/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/3/execution/node/287/log

@daosbuild1
Copy link
Collaborator

Copy link
Contributor

@GitHuaKuang GitHuaKuang left a comment

Choose a reason for hiding this comment

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

grep found some TLS_RSA_WITH_AES_128_GCM_SHA256 usage in src/control/vendor, wonder if we need to change these possibly "vendor" files?
./control/vendor/golang.org/x/net/http2/ciphers.go: cipher_TLS_RSA_WITH_AES_128_GCM_SHA256 uint16 = 0x009C
./control/vendor/golang.org/x/net/http2/ciphers.go: cipher_TLS_RSA_WITH_AES_128_GCM_SHA256,
./control/vendor/google.golang.org/grpc/credentials/tls.go: tls.TLS_RSA_WITH_AES_128_GCM_SHA256: TLS_RSA_WITH_AES_128_GCM_SHA256",

…ning hashes. This uses a seeded PRNG to generate the same salt every time.

Signed-off-by: David Quigley <[email protected]>
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/5/execution/node/299/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on CentOS 7 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/5/execution/node/270/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/5/execution/node/303/log

@dpquigl
Copy link
Contributor Author

dpquigl commented Nov 23, 2020

@GitHuaKuang We dont have to be concerned with what is in vendor. Those are just supported by those libraries but in our code we say we're only advertising that we support that one cipher suite.

Skip-func-test-leap15:true

signed-off-by: David Quigley <[email protected]>
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/6/execution/node/1062/log

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3923/6/execution/node/1137/log

@dpquigl dpquigl requested review from mjmac and GitHuaKuang November 24, 2020 17:28
@mjmac
Copy link
Contributor

mjmac commented Nov 24, 2020

Failures are not related to crypto changes.

@mjmac mjmac merged commit 5d7c051 into master Nov 24, 2020
@mjmac mjmac deleted the dpquigl/DAOSDE-112 branch November 24, 2020 17:35
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.

4 participants