Make use of sha1 crate an optional feature

[xnorpx]

For security reasons rust crypto might need to be disabled and one would want use pure OpenSSL.

This change adds a feature flag for sha1 crate.

• Allows users to not use sha1 and only use OpenSSL
• Allows users to use sha1 only (for ice/stun) and no OpenSSL
• Allows users to turn on sha1 and use Open SSL for rest

[xnorpx]

I will run this change through our CI and deployment and also figure out github

[algesten]

I think we should separate the idea of win crypto vs openssl and this. The sha1 crate is effectively an optimization regardless of which "base" crypto provider you select.

Ultimately the features would be like this:

default = ["openssl", "sha1"]
openssl = ["dep:openssl", "_base_crypto"]
wincng = ["dep:str0m-cng", "_base_crypto"]
sha1 = ["dep:sha1"]

Then the internal check will be that we at least have something provides _base_crypto feature.

sha1 just "takes over" from whatever is doing sha1 if that's not installed.

[xnorpx]

I think we should separate the idea of win crypto vs openssl and this. The sha1 crate is effectively an optimization regardless of which "base" crypto provider you select.

Ultimately the features would be like this:

default = ["openssl", "sha1"]
openssl = ["dep:openssl", "_base_crypto"]
wincng = ["dep:str0m-cng", "_base_crypto"]
sha1 = ["dep:sha1"]

Then the internal check will be that we at least have something provides _base_crypto feature.

sha1 just "takes over" from whatever is doing sha1 if that's not installed.

Ok I think I see what you mean, let me try add that.

[xnorpx]

I think we should separate the idea of win crypto vs openssl and this. The sha1 crate is effectively an optimization regardless of which "base" crypto provider you select.

Ultimately the features would be like this:

default = ["openssl", "sha1"]
openssl = ["dep:openssl", "_base_crypto"]
wincng = ["dep:str0m-cng", "_base_crypto"]
sha1 = ["dep:sha1"]

Then the internal check will be that we at least have something provides _base_crypto feature.

sha1 just "takes over" from whatever is doing sha1 if that's not installed.

So I enabled the sha1 by default so there should be no changes for current users. (except the ones that disables all features)

I think we can add the base_crypto check and features once we add the cng crate.

[xnorpx]

@algesten ok I tested it in our repo and end2end and seem to work fine. So ready for merge unless more comments.

[xnorpx]

@thomaseizinger you might need to add

"feature["sha1"]"

on your side after this change.

[thomaseizinger]

@thomaseizinger you might need to add

"feature["sha1"]"

on your side after this change.

That is a semver-breaking change then. Is the plan to make the corresponding version bump too?

[thomaseizinger]

FWIW, changing code paths with features can be problematic because features get aggregated by cargo. As a rule of thumb, features should only add new APIs, not modify existing code paths.

To select a "backend" for a certain functionality, I'd recommend to use regular cfgs instead. Those need to be set at compile-time by the final binary. The dalek folks do this for example: https://github.com/dalek-cryptography/curve25519-dalek/tree/main/curve25519-dalek#manual-backend-override

[xnorpx]

cfg feels a little bit much in this case, but whatever @algesten see fit.

[thomaseizinger]

cfg feels a little bit much in this case, but whatever @algesten see fit.

I tend to agree. The risk is more on your end because there is no way to disable the sha1 feature if it is enabled somehow.

[algesten]

That is a semver-breaking change then. Is the plan to make the corresponding version bump too?

Yeah, let's bump to 0.7.0.

FWIW, changing code paths with features can be problematic because features get aggregated by cargo. As a rule of thumb, features should only add new APIs, not modify existing code paths.

I hear ya. Got very deep into this problem in ureq wrt crypto provider. There I decided to require both a feature flag and a configuration change to enable another crypto backend. However for str0m/sha1 I reason like this:

• str0m should be optimized for maximum performance
• sha1 crate should be enabled by default
• That some people forbid random crates without analyzing the situation is not really str0m's problem. However we also want to encourage adoption and not take hard stances that preclude groups of users (or force forks) when we can avoid it
• Diamond dependencies with str0m is very unlikely (compared to ureq for instance)
• If you are forbidding the sha1 crate, you probably have tools that would detect a diamond dependency accidentally enabling the feature

[algesten]

Just testing to see if the asm feature works on windows now.

[thomaseizinger]

FWIW, changing code paths with features can be problematic because features get aggregated by cargo. As a rule of thumb, features should only add new APIs, not modify existing code paths.

I hear ya. Got very deep into this problem in ureq wrt crypto provider. There I decided to require both a feature flag and a configuration change to enable another crypto backend. However for str0m/sha1 I reason like this:

* str0m should be optimized for maximum performance

* sha1 crate should be enabled by default

* That some people forbid random crates without analyzing the situation is not really str0m's problem. However we also want to encourage adoption and not take hard stances that preclude groups of users (or force forks) when we can avoid it

* Diamond dependencies with str0m is very unlikely (compared to ureq for instance)

* If you are forbidding the sha1 crate, you probably have tools that would detect a diamond dependency accidentally enabling the feature

All sounds very reasonable to me!

[algesten]

The asm feature is still not working.

[xnorpx]

• If you are forbidding the sha1 crate, you probably have tools that would detect a diamond dependency accidentally enabling the feature

Yes, the prod build pipelines have checks that are injected. But I also added cargo tree | grep sha1 check in our own gate as well.

Thanks for quick review!