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

Upgrade rustls to v0.23 and add support for aws-lc-rs #1222

Merged
merged 4 commits into from
May 4, 2024

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Mar 2, 2024

@paolobarbolini paolobarbolini force-pushed the rustls-0.23 branch 2 times, most recently from b6e3371 to f376d5b Compare March 11, 2024 17:31
@paolobarbolini paolobarbolini changed the title Bump rustls to v0.23 Upgrade rustls to v0.23 and add support for aws-lc-rs Mar 11, 2024
@paolobarbolini paolobarbolini force-pushed the rustls-0.23 branch 3 times, most recently from 8c5422c to cbb296c Compare March 18, 2024 10:26
@paolobarbolini paolobarbolini marked this pull request as ready for review March 22, 2024 09:40
@Callum-A
Copy link

Hey is there any plans to merge this, there's a security advisory that depends on this version bump.

GHSA-6g7w-8wpp-frhj

@paolobarbolini
Copy link
Contributor Author

Hey is there any plans to merge this, there's a security advisory that depends on this version bump.

GHSA-6g7w-8wpp-frhj

To be fair this is not true. The patch was backported to previous versions of rustls. cargo update -p rustls is enough to fix the issue.

@Jarema
Copy link
Member

Jarema commented Apr 26, 2024

@Callum-A we are planning to merge this soon.

@Callum-A
Copy link

Callum-A commented Apr 26, 2024

Thanks all @paolobarbolini is correct. Ty for the info, the advisory we received on our end has been resolved :)

@Jarema
Copy link
Member

Jarema commented Apr 29, 2024

I wonder if it makes sense to default to ring, as that seems to be pain-free path for Windows users?
Though of course it's going opposite direction than the rustls crate itself...

@jadamcrain
Copy link
Contributor

Defaulting to ring is a good idea IMO. There's a lot of hate and discontent over in the rustls issue tracker ATM about their choice to default to aws-lc-rs.

Personally, I'm excited to have aws-lc-rs as an option because it's being maintained by a large company and not a solo maintainer, but the build issues are real.

@paolobarbolini
Copy link
Contributor Author

Fixed

@Jarema
Copy link
Member

Jarema commented Apr 30, 2024

@paolobarbolini thanks!
Though I think we still need nasm in CI for Windows :)

@paolobarbolini
Copy link
Contributor Author

I've switched the tests back to ring.

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -56,7 +58,10 @@ num = "0.4.1"


[features]
default = ["server_2_10"]
default = ["server_2_10", "aws_lc_rs"]
Copy link
Member

Choose a reason for hiding this comment

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

were you able to get any perf numbers comparing both crypto backends?
I would like to get some data before deciding to make a default switch.

@Jarema Jarema merged commit 276d61d into nats-io:main May 4, 2024
12 checks passed
Jarema added a commit that referenced this pull request May 6, 2024
# 0.35.0

## Overview

This release makes tls setup more flexible, leveraging rusls v0.23 and allowing to pick crypto backend:
* ring
* aws-lc-rs
* fips

Some other highlights:
* force reconnect via `force_reconnect` method
* explicit create/update consumer API

Thank you for all your contributions!

## Added
* Add `ToServerAddrs` impl for array/vector of strings by @mmalek in #1231
* Add public constructor for Acker by @AbstractiveNord in #1232
* Add force reconnect by @Jarema in #1240
* Add features check by @Jarema in #1247
* Add stream placement by @Jarema in #1250
* Add consumer action by @Jarema in #1254
* Add support for aws-lc-rs (rustls v0.23.0) by @paolobarbolini in #1222

## Fixed
* Use last header value for JetStream messages by @Jarema in #1239

## Changed
* Wrap inbox prefix in an `Arc` by @thomastaylor312 in #1236
* Document feature flags by @Jarema in #1246
* Don't force flush if write buffer isn't empty by @paolobarbolini in #1241

## New Contributors
* @mmalek made their first contribution in #1231

**Full Changelog**: async-nats/v0.34.0...async-nats/v0.35.0



Signed-off-by: Tomasz Pietrek <[email protected]>
Co-authored-by: Piotr Piotrowski <[email protected]>
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.

4 participants