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

(TK-497) Allow requiring authorization via config setting #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stelcodes
Copy link

@stelcodes stelcodes commented Sep 30, 2021

From my commit message:

This addresses PDB-5261 which documents a transient bug that allows
the metrics endpoint to be accessed without authorization when the
trapperkeeper-metrics bootstrap config contains the authorization service.
The root of this problem is likely a race condition within
trapperkeeper.

This patch adds a trapperkeeper-metrics config option
metrics.metrics-webservice.require-auth which defaults to true. In order
to use the trapperkeeper-metrics service without authorization, this
option must explicitly be set to false. When this option is set to true
and the authorization service is not loaded, an exception occurs and the
service does not start.

@stelcodes stelcodes requested a review from a team September 30, 2021 23:18
@austb
Copy link
Contributor

austb commented Oct 5, 2021

All the PE components use the authentication service, so this may need to be an X release for the library, but it shouldn't be a breaking change for us.

@@ -80,6 +80,10 @@ metrics: {
# Default is true.
enabled: false

# Enable or disable authorization for metrics endpoint
# Default is true.
require-auth: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be nested under the jolokia section?

Copy link
Author

@stelcodes stelcodes Oct 18, 2021

Choose a reason for hiding this comment

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

Oh nice catch! Would it actually make more sense to nest it under jolokia?

Comment on lines 87 to 92
;; When there should be authorization but there is no authorization service, fail fast
(when (and require-auth? (not auth-service))
(throw (ex-info (str "Config setting require-auth is set to true but authorization service was not found. "
"If you don't require authorization, set the trapperkeeper-metrics config setting "
"metrics.metrics-webservice.require-auth to false")
{})))
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of throwing here, should we use get-service instead of maybe-get-service above? Or are you wanting a more-specific error message than the one trapperkeeper would give you?

Copy link
Author

@stelcodes stelcodes Oct 18, 2021

Choose a reason for hiding this comment

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

Hey Molly, that makes sense to me. Were you thinking of something like this?

    (when (get-in-config [:metrics :metrics-webservice :jolokia :enabled] true)
      (let [config (->> (get-in-config [:metrics :metrics-webservice :jolokia :servlet-init-params] {})
                        jolokia/create-servlet-config)
            ;; NOTE: Normally, these route and server lookups would be done by
            ;; WebroutingService/add-servlet-handler, but that doesn't properly
            ;; mount sub-paths at the moment (TK-420). So we explicitly compute
            ;; these items and use WebserverService/add-servlet-handler instead.
            route (str (get-route this) "/v2")
            server (get-server this)
            options (if (nil? server)
                      {:servlet-init-params config}
                      {:servlet-init-params config :server-id (keyword server)})
            require-auth? (get-in-config [:metrics :metrics-webservice :require-auth] true)]
        (if require-auth?
          (let [auth-service (tk-services/get-service this :AuthorizationService)
                auth-check-fn (partial tk-auth/authorization-check auth-service)]
                (add-servlet-handler (jolokia/create-servlet auth-check-fn)
                                             route
                                             options))
          (add-servlet-handler (jolokia/create-servlet nil) route options))))

Comment on lines 249 to 250
(-> (merge metrics-service-config auth-config ssl-webserver-config)
(assoc-in [:metrics :metrics-webservice :require-auth] true)))))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you do this twice, might be good to put it in a let at the start of the test.

@mwaggett
Copy link
Contributor

mwaggett commented Oct 8, 2021

Would also be good to update the changelog when this is ready to go in and be released.

@stelcodes stelcodes force-pushed the main branch 2 times, most recently from 2875cfe to 858995f Compare October 20, 2021 19:20
This commit addresses PDB-5261 which documents a transient bug that
allows the metrics endpoint to be accessed without authorization when
the trapperkeeper-metrics bootstrap config contains the authorization
service. The root of this problem is likely a race condition within
trapperkeeper.

This patch adds a trapperkeeper-metrics config option
metrics.metrics-webservice.jolokia.require-auth which defaults to true.
In order to use the trapperkeeper-metrics service without authorization,
this option must explicitly be set to false. When this option is set to
true and the authorization service is not loaded, the
IllegalArgumentException thrown by trapper-keeper's get-service method
is purposefully unhandled and crashes the program. Helpful log messages
are printed when this occurs.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants