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 missing symbols for postgres #979

Merged
merged 2 commits into from
May 1, 2023

Conversation

samuel40791765
Copy link
Contributor

Issues:

Resolves CryptoAlg-1746 and CryptoAlg-1747

Description of changes:

Postgres has some "missing symbols" that we need to add back to support.

  1. cast had originally been removed along with decrepit, so this is just putting them back. The test file had not been removed, so we only need to add back cast.c.
  2. Postgres expects SSL_CTX_set_min_proto_version to be a macro, which was OpenSSL's way of wrapping around the SSL_CTX_ctrl functions. The simplest way around this is to redefine the SSL_CTX_set_min_proto_version as a macro at the end of ssl.h, like we've been doing for other instances of this.

Call-outs:

N/A

Testing:

Confirmed by building with Postgres.

./configure --with-openssl --enable-tap-tests --with-includes=$PWD/aws-lc/test_build_dir/include --with-libraries=$PWD/aws-lc/test_build_dir/lib --prefix=$PWD/build
make -j8 -C contrib all

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and
the ISC license.

@samuel40791765 samuel40791765 force-pushed the postgres-cast branch 4 times, most recently from 9b8e900 to 774eefa Compare April 25, 2023 22:47
Copy link
Contributor Author

@samuel40791765 samuel40791765 left a comment

Choose a reason for hiding this comment

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

I looked into some of our concerns for adding back Cast from decrepit and wanted to make the following points.

  1. This had already existed in AWS-LC when we were working on removing decrepit. During the work for that, we had the consensus that we would remove everything that did not have usage internally and incrementally add back functions if customers needed the functionality.
  2. We can consider influencing the Postgres upstream by submitting a PR to them to ifdef out the Cast ciphersuites. However, this change would only be added in new Postgres releases and won't be applicable to old releases. We can take a look at PostGres's versioning policy here: https://www.postgresql.org/support/versioning/
    PostgreSQL Versions 11-15 are currently supported, with each major version of PostgreSQL having a support timeline of 5 years. Any upstream influence we make now, won't truly be felt until PostgreSQL15 is deprecated in 2027 (assuming an upstream contribution makes it to Version 16).

Considering these factors, we should look to add minimal support for the Cast ciphersuites through EVP now, so we can gain adoption in existing versions of PostGreSQL. We can still make an upstream contribution to ifdef the CAST, blowfish, and des usage out of PostGres when building with AWS-LC, so that new versions of PostGres don't continue rely on this behavior.
We're making sure to only expose the minimal amount of symbols needed, which are the EVP_CIPHERs in this case. We're also adding the AWS_LC_DEPRECATED macro in front of the CAST ciphersuites to indicate that these function should not be relied on. This will intentionally cause current builds of PostGres to complain about deprecated symbols. We can make an upstream contribution to ifdef these deprecated symbols to silence these warnings.

@samuel40791765 samuel40791765 requested review from nebeid and removed request for andrewhop May 1, 2023 18:01
@samuel40791765 samuel40791765 merged commit d31f1c3 into aws:main May 1, 2023
@samuel40791765 samuel40791765 deleted the postgres-cast branch May 1, 2023 21:23
andrewhop pushed a commit to andrewhop/aws-lc that referenced this pull request Sep 11, 2023
This had already existed in AWS-LC when we were working on removing decrepit. 
During the work for that, we had the consensus that we would remove everything
that did not have usage internally and incrementally add back functions if
customers needed the functionality.
We can consider influencing the Postgres upstream by submitting a PR to them to 
ifdef out the Cast ciphersuites. However, this change would only be added in new
Postgres releases and won't be applicable to old releases. PostgreSQL Versions
11-15 are currently supported, with each major version of PostgreSQL having a
support timeline of 5 years. Any upstream influence we make now, won't truly be
felt until PostgreSQL15 is deprecated in 2027 (assuming an upstream contribution
makes it to Version 16).
Considering these factors, we should look to add minimal support for the Cast
ciphersuites through EVP now, so we can gain adoption in existing versions of 
PostGres. We can still make an upstream contribution to ifdef the CAST,
blowfish, and des usage out of PostGres when building with AWS-LC, so that new
versions of PostGres don't continue rely on this behavior.
We're making sure to only expose the minimal amount of symbols needed, which are
the EVP_CIPHERs in this case. We're also adding the AWS_LC_DEPRECATED macro in
front of the CAST ciphersuites to indicate that these function should not be
relied on. This will intentionally cause current builds of PostGres to complain
about deprecated symbols. We can make an upstream contribution to ifdef these
deprecated symbols to silence these warnings.
andrewhop pushed a commit to andrewhop/aws-lc that referenced this pull request Sep 12, 2023
This had already existed in AWS-LC when we were working on removing decrepit.
During the work for that, we had the consensus that we would remove everything
that did not have usage internally and incrementally add back functions if
customers needed the functionality.
We can consider influencing the Postgres upstream by submitting a PR to them to
ifdef out the Cast ciphersuites. However, this change would only be added in new
Postgres releases and won't be applicable to old releases. PostgreSQL Versions
11-15 are currently supported, with each major version of PostgreSQL having a
support timeline of 5 years. Any upstream influence we make now, won't truly be
felt until PostgreSQL15 is deprecated in 2027 (assuming an upstream contribution
makes it to Version 16).
Considering these factors, we should look to add minimal support for the Cast
ciphersuites through EVP now, so we can gain adoption in existing versions of
PostGres. We can still make an upstream contribution to ifdef the CAST,
blowfish, and des usage out of PostGres when building with AWS-LC, so that new
versions of PostGres don't continue rely on this behavior.
We're making sure to only expose the minimal amount of symbols needed, which are
the EVP_CIPHERs in this case. We're also adding the AWS_LC_DEPRECATED macro in
front of the CAST ciphersuites to indicate that these function should not be
relied on. This will intentionally cause current builds of PostGres to complain
about deprecated symbols. We can make an upstream contribution to ifdef these
deprecated symbols to silence these warnings.
andrewhop pushed a commit that referenced this pull request Sep 13, 2023
This had already existed in AWS-LC when we were working on removing decrepit.
During the work for that, we had the consensus that we would remove everything
that did not have usage internally and incrementally add back functions if
customers needed the functionality.
We can consider influencing the Postgres upstream by submitting a PR to them to
ifdef out the Cast ciphersuites. However, this change would only be added in new
Postgres releases and won't be applicable to old releases. PostgreSQL Versions
11-15 are currently supported, with each major version of PostgreSQL having a
support timeline of 5 years. Any upstream influence we make now, won't truly be
felt until PostgreSQL15 is deprecated in 2027 (assuming an upstream contribution
makes it to Version 16).
Considering these factors, we should look to add minimal support for the Cast
ciphersuites through EVP now, so we can gain adoption in existing versions of
PostGres. We can still make an upstream contribution to ifdef the CAST,
blowfish, and des usage out of PostGres when building with AWS-LC, so that new
versions of PostGres don't continue rely on this behavior.
We're making sure to only expose the minimal amount of symbols needed, which are
the EVP_CIPHERs in this case. We're also adding the AWS_LC_DEPRECATED macro in
front of the CAST ciphersuites to indicate that these function should not be
relied on. This will intentionally cause current builds of PostGres to complain
about deprecated symbols. We can make an upstream contribution to ifdef these
deprecated symbols to silence these warnings.
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.

3 participants