Add mTLS authentication from auth-mtls branch
#773
Conversation
- implement mTLS validation and context JWT checks with dedicated errors - gate jwt-only GraphQL modules/tests for auth-mtls builds - add mTLS-focused tests and adjust feature-specific test setup Closes #768
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #773 +/- ##
==========================================
+ Coverage 75.27% 75.92% +0.65%
==========================================
Files 71 72 +1
Lines 19779 19935 +156
==========================================
+ Hits 14888 15135 +247
+ Misses 4891 4800 -91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| let client_verifier = rustls::server::WebPkiClientVerifier::builder(Arc::new(root_store)) | ||
| .allow_unauthenticated() |
There was a problem hiding this comment.
Is allow_unauthenticated adopted here as a temporary workaround? My understanding of the goal is to "enforce client certificate verification", and if that is true, I think we might want to remove allow_unauthenticated in the future.
There was a problem hiding this comment.
Rationale for allow_unauthenticated():
- Keep the loopback bypass working without extra ports or listeners.
- Allow the app layer to return explicit HTTP auth errors instead of failing the TLS handshake.
If we later remove the loopback bypass or move it to a separate port/listener, we could enforce TLS client auth here. That change is optional, not required.
tests/mtls_integration.rs
Outdated
| #[tokio::test] | ||
| async fn mtls_graphql_request_succeeds() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Would you be open to adding tests for non-successful scenarios? Since auth/mtls.rs defines specific ERR_* constants, I think we might benefit from verifying the rejection behavior for those cases. If adding them now causes is a delay, I think we can create a follow-up issue to track it.
There was a problem hiding this comment.
@sophie-cluml, what you pointed out is something I definitely missed. I appreciate it. I’ve added tests covering the rejection paths.
| if is_local(addr) { | ||
| return Ok(schema | ||
| .execute(request.data(RoleGuard::Local).data(addr)) | ||
| .await | ||
| .into()); | ||
| } |
There was a problem hiding this comment.
I think RoleGuard::Local in auth-jwt feature build served a good purpose, since the GraphQL API endpoint reset_admin_password was only for Local role. However, in auth-mtls feature build, I can see that the account module (and thus reset_admin_password in graphql/account.rs) is not part of the GraphQL API schema at all, so I think the original rationale for is_local() no longer applies for this auth-mtls feature build.
I think it rather has drawback. Currently graphql_handler checks for is_local(addr) and forces RoleGuard::Local. That means even a valid mTLS + Context JWT request from localhost comes in, the request gets downgraded to Local role and the request will be rejected by role guards of other GraphQL API endpoints.
Therefore, I think it might be worth considering to remove the if is_local(addr) branch in L414, and possibly remove the introduction of DISABLE_LOCAL_AUTH_BYPASS_ENV.
There was a problem hiding this comment.
Thanks for the thoughtful feedback—it’s a valid concern. I agree the Local bypass can have drawbacks in auth-mtls, especially when a valid mTLS + Context JWT request comes from localhost.
That said, I’d like to defer this decision until after we fully remove the auth-jwt feature. Once auth-jwt is gone, we can reassess whether the local bypass (and DISABLE_LOCAL_AUTH_BYPASS_ENV) still makes sense and adjust the behavior accordingly.
Closes #772
Closes #768
Closes #770