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

Allow to enable security for the Management interface without enabling basic authentication and document support for other mechanisms #44554

Conversation

michalvavrik
Copy link
Member

We use same HTTP authenticator for both main router and management interface, therefore same HTTP auth mechanisms can be supported. Difference is mainly that secuirty is not enabled by default when the Quarkus Security is present. Reason for this is backwards compatibility, users can use management interface on ports exposed only inside cluster etc. where external requests are not allowed. But still require authentication for the main router. The other difference is that only supported authorization method are HTTP perms. Using security annotations on custom health check beans etc. is possible, but less advisable as we cannot perform this eagerly.

This PR:

  • adds new configuration property that enables management interface authentication
  • adds test for OIDC with management interface
  • documents that other mechanisms can be used

I really wonder if we should not enable security by default. Thoughts about that?

Existing Management interface auth tests before this PR:

  • TLS/mTLS is tested in Vert.x HTTP extension unit tests
  • SmallRye JWT and basic for both management interface and main router, and path-specific management interface auth are tested in integration-tests/management-interface-auth

Copy link

github-actions bot commented Nov 17, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Thanks! That's awesome!

@sberyozkin
Copy link
Member

@michalvavrik Indeed, looks good.
What should happen here, https://github.com/quarkusio/quarkus/blob/main/integration-tests/management-interface-auth/src/main/resources/application.properties#L5, if instead of quarkus.management.auth.basic=true users type quarkus.management.auth.enabled=true ? Should basic authentication be activated by default for the management interface in this case ?

@michalvavrik
Copy link
Member Author

michalvavrik commented Nov 18, 2024

What should happen here, https://github.com/quarkusio/quarkus/blob/main/integration-tests/management-interface-auth/src/main/resources/application.properties#L5, if instead of quarkus.management.auth.basic=true users type quarkus.management.auth.enabled=true ?

I actually tested it because I didn't trust that config expression is expanded in @ConfigItem... annotation. IIRC (I don't have it checked out ATM):

  • quarkus.management.auth.basic=true -> pass
  • quarkus.management.auth=true and no ..basic=true -> fail
  • quarkus.management.auth=false and ..basic=true -> fail

Should basic authentication be activated by default for the management interface in this case ?

I don't think that is the case because there are other mechanisms like SR JWT? I think that mechanism is not a bulletproof, but I know what you mean. If you want, I can re-check that.

@sberyozkin
Copy link
Member

sberyozkin commented Nov 18, 2024

Thanks,

quarkus.management.auth=true and no ..basic=true -> fail - but in the authorization code flow test you added in this PR we don't do anything specific around enabling authorization code flow at the management interface and it still picks up the OIDC code flow mechanism set up to protect the main router... Also, on the main router, we do not have to enable basic authentication explicitly if it is the only authentication mechanism...

May be we should treat Basic authentication on the management router similarly to the way it is treated on the main router ? Or will it be too confusing ?

@michalvavrik
Copy link
Member Author

michalvavrik commented Nov 18, 2024

quarkus.management.auth=true and no ..basic=true -> fail - but in the authorization code flow test you added in this PR we don't do anything specific around enabling authorization code flow at the management interface and it still picks up the OIDC code flow mechanism set up to protect the main router...

There you added OIDC extension. If you add an extension that provides that mechanism, it's a good signal you want to use it. But basic auth is builtin and always present, so we can't guess if you want it or not.

Also, on the main router, we do not have to enable basic authentication explicitly if it is the only authentication mechanism...

I agree, but this is not the only mechanism in the IT/Management interface auth module.

May be we should treat Basic authentication on the management router similarly to the way it is treated on the main router ? Or will it be too confusing ?

I'll add another test to make sure this works as it does on the main router later today. I think it should work completely same because there, basic auth is also not implicitly enabled if other mechanisms are present.

@michalvavrik michalvavrik force-pushed the feature/enable-management-interface-auth-without-basic-auth branch from 101671d to dd0133a Compare November 18, 2024 15:15
@michalvavrik
Copy link
Member Author

@sberyozkin quick update as I had a look:

  • in IT/Management interface auth module basic is enabled anyway because is is required for the main router, so it doesn't fail there
  • I have added a new test that verifies you only need to set quarkus.management.auth.enabled=true. If no other auth mechanism is detected, then basic auth is enabled which is exactly what we do for the main router.

@michalvavrik michalvavrik force-pushed the feature/enable-management-interface-auth-without-basic-auth branch from dd0133a to 57bf2f8 Compare November 18, 2024 15:20
Copy link

quarkus-bot bot commented Nov 18, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 57bf2f8.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@michalvavrik
Copy link
Member Author

I believe native failure is not related:

2024-11-18T16:05:25.8067769Z Error: Detected a started Thread in the image heap. Thread name: main. Threads running in the image generator are no longer running at image runtime. If these objects should not be stored in the image heap, you can use 
2024-11-18T16:05:25.8070522Z ------------------------------------------------------------------------------------------------------------------------
2024-11-18T16:05:25.8075513Z 
2024-11-18T16:05:25.8076255Z                         3.9s (7.2% of total time) in 475 GCs | Peak RSS: 1.53GB | CPU load: 3.69
2024-11-18T16:05:25.8078833Z     '--trace-object-instantiation=java.lang.Thread'
2024-11-18T16:05:25.8079808Z ========================================================================================================================
2024-11-18T16:05:25.8081704Z 
2024-11-18T16:05:25.8082685Z Finished generating 'quarkus-integration-test-vertx-web-jackson-999-SNAPSHOT-runner' in 53.5s.
2024-11-18T16:05:25.8085896Z to find classes that instantiate these objects. Once you found such a class, you can mark it explicitly for run time initialization with 

@sberyozkin sberyozkin merged commit 05b92aa into quarkusio:main Nov 18, 2024
54 of 55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 18, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Nov 18, 2024
@michalvavrik michalvavrik deleted the feature/enable-management-interface-auth-without-basic-auth branch November 18, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to enable Management interface authentication without enabling basic authentication
3 participants