Skip to content

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

Closed
micolous wants to merge 1 commit into
kanidm:masterfrom
micolous:demo-optional-rustls
Closed

webauthn-rs-demo: make tls support optional#357
micolous wants to merge 1 commit into
kanidm:masterfrom
micolous:demo-optional-rustls

Conversation

@micolous

@micolous micolous commented Oct 3, 2023

Copy link
Copy Markdown
Collaborator

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

yaleman commented Oct 3, 2023

Copy link
Copy Markdown
Member

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

micolous commented Oct 3, 2023

Copy link
Copy Markdown
Collaborator Author

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

yaleman commented Oct 3, 2023

Copy link
Copy Markdown
Member

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

@micolous

micolous commented Oct 3, 2023

Copy link
Copy Markdown
Collaborator Author

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

micolous commented Oct 6, 2023

Copy link
Copy Markdown
Collaborator Author

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