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

webauthn-rs-demo: make tls support optional #357

Closed
wants to merge 1 commit into from

Conversation

micolous
Copy link
Collaborator

@micolous micolous commented Oct 3, 2023

rustls depends on ring 0.16, which doesn't build on aarch64-pc-windows-msvc, and has lots of other cross-compiling issues. Many of these issues have been fixed, but only on the ring 0.17 branch.

This PR makes webauthn-rs-demo TLS support optional (--features tls).

Longer term, I'd rather switch this over to using OpenSSL like everything else, but this at least unblocks building the entire workspace with default features on a system ring 0.16 doesn't support.

@yaleman
Copy link
Member

yaleman commented Oct 3, 2023

In keeping with the "do it right by default" can we make TLS a default feature and just disable it in the windows workspace builds?

@micolous
Copy link
Collaborator Author

micolous commented Oct 3, 2023

Can we make TLS a default feature and just disable it in the windows workspace builds?

There's no way to enable/disable this conditionally by platform: rust-lang/cargo#1197

A work-around would be to convert it to a library, and then use it from another package which has a [dependency] declaration by platform. However, it'd be simpler to move this away from rustls entirely so that the problem goes away.

As far as I can tell, the other tide example doesn't build in TLS support at all.

In keeping with the "do it right by default"...

This binary currently generates a self-signed certificate at start-up, and there's no way to provide one. As a result, you either need to:

  • configure client browsers to accept a self-signed certificate
  • put a HTTPS load balancer in front of it, and configure that to accept any certificate (which some public clouds do)

Because the certificate will change on start-up, there's marginal benefit to using HTTPS over plain HTTP.

I guess the open question is how is https://webauthn.firstyear.id.au configured, and will this break it?

@yaleman
Copy link
Member

yaleman commented Oct 3, 2023

I more meant add tls to features.default which you can disable on build.

@micolous
Copy link
Collaborator Author

micolous commented Oct 3, 2023

The trouble is that on an affected target, you'd need to apply --no-default-features as a workspace-level option, and then re-add all the other default features (which aren't broken) for every single package in the workspace, which will change over time.

TLS support in webauthn-rs-demo is not documented, nor is it enabled by default anyway. While this shifts the burden from (at minimum) an extremely niche platform (aarch64-pc-windows-gnu) to literally everyone else, the burden is to add --features tls for one package if they need TLS support; and there's no tests of any kind around it.

Once the demo is migrated from rustls to openssl, it could become a default (as openssl works just fine there). That migration would also mean that it doesn't ship two crypto libraries for a tiny demo app. 😄

@micolous micolous mentioned this pull request Oct 3, 2023
2 tasks
@micolous
Copy link
Collaborator Author

micolous commented Oct 6, 2023

Dropping this in favour of #360

@micolous micolous closed this Oct 6, 2023
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.

2 participants