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

remove native-tls #2675

Merged
merged 21 commits into from
May 26, 2023
Merged

remove native-tls #2675

merged 21 commits into from
May 26, 2023

Conversation

82marbag
Copy link
Contributor

@82marbag 82marbag commented May 5, 2023

  • Show how to plug a connector
  • For TLS: keep rustls, only

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
rust-runtime/aws-smithy-client/Cargo.toml Show resolved Hide resolved
rust-runtime/aws-smithy-client/src/hyper_ext.rs Outdated Show resolved Hide resolved
Signed-off-by: Daniele Ahmed <[email protected]>
@hlbarber hlbarber added the breaking-change This will require a breaking change label May 23, 2023
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@smithy-lang smithy-lang deleted a comment from github-actions bot May 25, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot May 25, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot May 25, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot May 25, 2023
/// wrap it in a [hyper_ext::Adapter](crate::hyper_ext::Adapter).
pub type NativeTls = hyper_tls::HttpsConnector<hyper::client::HttpConnector>;

pub fn native_tls() -> NativeTls {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can do it post merge, but we should also include guidance for SDK customers

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo added this pull request to the merge queue May 26, 2023
Merged via the queue into main with commit d083c6f May 26, 2023
@drganjoo drganjoo deleted the remove-nativetls branch May 26, 2023 10:42
@stanal
Copy link

stanal commented Aug 24, 2023

why remove native-tls? can give some suggestions , I'm fusing on which to use between native-tls and rustls .

@jdisanti
Copy link
Collaborator

@stanal - The native-tls crate doesn't allow us to configure cipher suites, so if a vulnerability is discovered in one of the ones it uses, we don't have any ability to address it ourselves. We also have a requirement that this list of cipher suites needs to be used:
https://github.com/awslabs/smithy-rs/blob/778244f801b720bb2b2325a648378f0a51d7ecc6/rust-runtime/aws-smithy-client/src/conns.rs#L30-L40

github-merge-queue bot pushed a commit that referenced this pull request Sep 7, 2023
## Motivation and Context
~~**The PR will be a draft until
#2675 is merged into `main`.
Once that's merged, this PR can remove the change to `Cargo.toml` in
`aws-smithy-client`.**~~

**Now that it's passed tests in CI, ysaito1001 will merge this PR once
it has passed the internal tests**

Fixes #2500.

## Description
This PR updates the base image for ci-build's Dockerfile as the latest
image for [amazonlinux](https://gallery.ecr.aws/amazonlinux/amazonlinux)
has been updated to 2023.

As part of it, the changes to Dockerfile include
- Fixing conflicting curl's libraries by specifying `--allowerasing`
Without it, we'd get an error as follows:
```
package curl-minimal-7.88.1-1.amzn2023.0.1.x86_64 conflicts with curl provided by curl-7.87.0-2.amzn2023.0.2.x86_64
```
- Adding perl explicitly as a new version of openssl now requires it
Without `perl`, we'd get an error as follows:
```
Step 24/48 : RUN cargo +${rust_nightly_version} -Z sparse-registry install cargo-deny --locked --version ${cargo_deny_version}
 ---> Running in 3c3431881cfa
...
error: failed to run custom build command for `openssl-sys v0.9.76`
...
  --- stderr
  Can't locate FindBin.pm in @inc (you may need to install the FindBin module) (@inc contains: /usr/local/lib64/perl5/5.32 /usr/local/share/perl5/5.32 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at ./Configure line 15.
  BEGIN failed--compilation aborted at ./Configure line 15.
  thread 'main' panicked at '
```

Also, there is a change to `Cargo.toml` in `aws-smithy-client`:
- Forcing `aws-smithy-client` with `native-tls` feature to use a more
recent `openssl-sys` crate
Without it, `cargo minimal-versions check` would fail in
`aws-smithy-client` trying to build `openssl-sys` v0.9.39 where OpenSSL
in a container is more up-to-date, leading to a build failure (See [this
issue](sfackler/rust-openssl#1724) for more
details)

Finally, updating CI for `Run Tests on Windows`
- Manually installing `openssl` as
[suggested](sfackler/rust-openssl#1542 (comment)).
Without it, after introducing a more recent `openssl` 0.10.52
`dev-dependencies` in `aws-smithy-client`, we'd get this on Windows:
```
  --- stderr
  thread 'main' panicked at '

  Could not find directory of OpenSSL installation, and this `-sys` crate cannot
  proceed without this knowledge. If OpenSSL is installed and this crate had
  trouble finding it,  you can set the `OPENSSL_DIR` environment variable for the
  compilation process.

  Make sure you also have the development packages of openssl installed.
  For example, `libssl-dev` on Ubuntu or `openssl-devel` on Fedora.

  If you're in a situation where you think the directory *should* be found
  automatically, please open a bug at https://github.com/sfackler/rust-openssl
  and include information about your system as well as this message.

  $HOST = x86_64-pc-windows-msvc
  $TARGET = x86_64-pc-windows-msvc
  openssl-sys = 0.9.90
```

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: Yuki Saito <[email protected]>
Co-authored-by: Zelda Hessler <[email protected]>
@Velfi Velfi removed the needs-client-review Generic client label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants